| Message ID | CAGr1F2HwdCQyz=U5EkhYMA2WXMrwxPzftj3A_EmDRXdPPVBJpw@mail.gmail.com |
|---|---|
| State | Rejected, archived |
| Headers | show |
在 2012年2月15日 上午5:24，Aditya Kali <adityakali@google.com> 写道： > Hi Robin, > > > On Tue, Feb 14, 2012 at 12:01 AM, Robin Dong <hao.bigrat@gmail.com> wrote: >> >> 在 2012年2月10日 上午10:55，Ted Ts'o <tytso@mit.edu> 写道： >> > On Thu, Feb 02, 2012 at 03:48:49PM +0800, Robin Dong wrote: >> >> >> >> The reason is ext4_has_free_clusters reporting wrong >> >> result. Actually, the unit of argument "dirty_clusters" is block, so >> >> we should tranform the sbi->s_dirtyclusters_counter to block , just >> >> like "free_clusters". >> > >> > I've been looking at the the delayed allocation reservation code, and >> > I think it's a lot more confused that this. Some places we are >> > playing with fields as if they are clusters, and some times as if they >> > are blocks, and I've definitely found places where we're not handling >> > quota correctly with bigalloc (ext4_map_blocks is calling >> > ext4_da_update_reserve_space() with a value which is clearly in units >> > of blocks, but we are treating that vale as if it is clusters, etc.) >> > >> > So I think the problem is a lot larger than what you pointed out, and >> > we shouldn't fix it just by throwing in an EXT4_C2B call. If >> > s_dirtyclusters_counter should be denominated in blocks, then we need >> > to rename it. And there there's this comment which made me wince: >> > >> > * Note that in case of bigalloc, i_reserved_meta_blocks, >> > * i_reserved_data_blocks, etc. refer to number of clusters. >> > >> > Argh, no. If something is denominated in clusters, then it should be >> > appropriately named as such. >> > >> > Actually, I think the problem is a lot bigger than this, and it's a >> > conceptual one. When we try writing to a block which does not have a >> > mapping, and bigalloc is enabled, there are three cases: >> > >> > 1) This is the first time the cluster has ever been written to; hence, >> > even though we are dirtying a block, we need to reserve room for the >> > entire cluster. >> > >> > 2) While the block has not been mapped (and so the data on disk is >> > uninitialized) the cluster has indeed been allocated, so we don't need >> > to reserve any additional room for the block. >> > >> > 3) One or more blocks in the cluster have already been written to via >> > delayed allocation, but the cluster itself has not been selected >> > (allocated) yet. But since the space for the entire cluster was >> > reserved when the first block in the cluster was dirtied (case #1), we >> > don't need to reserve any more space. >> > >> > We aren't handling these cases correctly today, since we don't have >> > this logic here. These cases don't matter if you're only using >> > fallocate() and direct I/O, but if you are using buffered writes and >> > delayed allocation, they obviously matter a huge amount. >> > >> >> > And I don't think we're handling it correctly. Argh... >> > > > I thought we do handle these cases correctly after the 'ext4: attempt > to fix race in bigalloc code path' change. May be I missed something > or the code changed somewhere. If there is a testcase that fails, I > can take a look. > >> > - Ted >> >> Hi, Ted and Aditya >> >> I am trying to fix this "bigalloc quota reservation in >> delay-allocation" problem recently, and I feel confusion about these >> code below in ext4_ext_map_blocks(): >> >> if (flags & EXT4_GET_BLOCKS_DELALLOC_RESERVE) { >> unsigned int reserved_clusters; >> /* >> * Check how many clusters we had reserved this allocated range >> */ >> reserved_clusters = get_reserved_cluster_alloc(inode, >> map->m_lblk, allocated); >> if (map->m_flags & EXT4_MAP_FROM_CLUSTER) { >> if (reserved_clusters) { >> ...... >> ext4_da_update_reserve_space(inode, >> reserved_clusters, 0); >> } >> } else { >> BUG_ON(allocated_clusters < reserved_clusters); >> /* We will claim quota for all newly allocated blocks.*/ >> ext4_da_update_reserve_space(inode, allocated_clusters, >> 1); >> if (reserved_clusters < allocated_clusters) { >> struct ext4_inode_info *ei = EXT4_I(inode); >> int reservation = allocated_clusters - >> reserved_clusters; >> dquot_reserve_block(inode, >> EXT4_C2B(sbi, reservation)); >> spin_lock(&ei->i_block_reservation_lock); >> ei->i_reserved_data_blocks += reservation; >> spin_unlock(&ei->i_block_reservation_lock); >> } >> } >> } >> >> if we have allocated 3 clusters, why should we add "reservation" back >> to i_reservd_data_blocks ? > Because we will end up here again next time when we write blocks > [10-11] and try to claim space for them. At this time, there will be > no more reserved blocks left (unless you give them back) and you will > hit the warning in ext4_da_update_reserve_space(): > > if (unlikely(used > ei->i_reserved_data_blocks)) { > ext4_msg(inode->i_sb, KERN_NOTICE, "%s: ino %lu, used %d " > "with only %d reserved data blocks\n", > __func__, inode->i_ino, used, > ei->i_reserved_data_blocks); > WARN_ON(1); > used = ei->i_reserved_data_blocks; > } > > >> (Aditya Kali 's long comment dose not make >> sense to me) Maybe we don't need to do ext4_da_update_reserve_space >> when the new block is allocated from "implied_cluster"? Why not just >> change them to: >> >> if (flags & EXT4_GET_BLOCKS_DELALLOC_RESERVE) { >> if ( !(map->m_flags & EXT4_MAP_FROM_CLUSTER) ) >> /* We will claim quota for all newly allocated blocks.*/ >> ext4_da_update_reserve_space(inode, allocated_clusters, >> 1); >> } >> > > This won't work in all cases, like the one given in the long comment. > With above change, ext4_da_update_reserve_space() will get called > again the next time for some delayed allocated blocks in the same > cluster, but there isn't any reserved blocks left to claim. > The flag 'EXT4_MAP_FROM_CLUSTER' is not a sufficient check to > determine 'how many' clusters we need to claim. This is because > get_implied_cluster_alloc() only checks within already allocated > extents (and not in the page cache). So, I had to add the function > get_reserved_cluster_alloc() to check if there are any delayed > allocated blocks still in the page-cache. But I found the code in ext4_ext_map_blocks(): if ((sbi->s_cluster_ratio > 1) && ext4_find_delalloc_cluster(inode, map->m_lblk, 0)) map->m_flags |= EXT4_MAP_FROM_CLUSTER; thus, even the delayed allocated block in the page-cache (have not be allocated) already have been checked. Therefore in my opinion, the EXT4_MAP_FROM_CLUSTER is a sufficient check. Follow the case of your long comment :) , process will go like this in MY CODE ABOVE : [0-3], [4-7], [8-11] 1. delay-allocation write blocks 10&11 we reserve 1 cluster, the i_reserved_data_blocks is 1 now 2. delay-allocation write blocks 3 to 8 we reserve other 2 clusters, so 3 clusters totally, the i_reserved_data_blocks is 3 now 3. writeout the blocks 3 to 8 claim all 3 clusters, i_reserved_data_blocks is 0 now At this moment, we really have allocated 3 clusters, and the remain 10&11 block would never occupy another cluster (it would definitely go into the [8-11] cluster), so, why need we reserve one more cluster quota ? 4. writeout the 10&11 blocks the 10&11 blocks will be set EXT4_MAP_FROM_CLUSTER flag ( by get_implied_cluster_alloc as the 8~9 block have been allocated), and it will not call ext4_da_update_reserve_space any more As this, no warning, no reserve-and-release ping pong....at least in my imaging Could you please point out what I missed ? Thanks > You will also miss the other case (when EXT4_MAP_FROM_CLUSTER is set) > where we had reserved the cluster, but ended up not allocating it. > Since this cluster was not allocated, we need to update the reserved > space without claiming quota (ext4_da_update_reserve_space(inode, > reserved_clusters, 0) --- notice the last argument is '0'). > > I know its kinda messy, but I think it works. Also, since we don't > keep track of delayed extents, its harder to check in one go. > > As for your proposed fix earlier, the correct solution will be to not > convert 'free_clusters' value into blocks. > > diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c > index f9e2cd8..2c518f8 100644 > --- a/fs/ext4/balloc.c > +++ b/fs/ext4/balloc.c > @@ -426,7 +426,7 @@ static int ext4_has_free_clusters(struct ext4_sb_info *sbi, > > if (free_clusters - (nclusters + root_clusters + dirty_clusters) < > EXT4_FREECLUSTERS_WATERMARK) { > - free_clusters = EXT4_C2B(sbi, > percpu_counter_sum_positive(fcc)); > + free_clusters = percpu_counter_sum_positive(fcc); > dirty_clusters = percpu_counter_sum_positive(dcc); > } > /* Check whether we have space after accounting for current > > > We were incorrectly converting clusters to blocks and so estimating > that we have more free clusters available than what we actually do. > Hence the dmesg errors at write time. > >> >> -- >> -- >> Best Regard >> Robin Dong > > > Thanks, > Aditya
On Tue, Feb 14, 2012 at 7:19 PM, Robin Dong <hao.bigrat@gmail.com> wrote: > But I found the code in ext4_ext_map_blocks(): > > if ((sbi->s_cluster_ratio > 1) && > ext4_find_delalloc_cluster(inode, map->m_lblk, 0)) > map->m_flags |= EXT4_MAP_FROM_CLUSTER; > > thus, even the delayed allocated block in the page-cache (have not be > allocated) already have been checked. Therefore in my opinion, the > EXT4_MAP_FROM_CLUSTER is a sufficient check. > > Follow the case of your long comment :) , process will go like this > in MY CODE ABOVE : > > [0-3], [4-7], [8-11] > > 1. delay-allocation write blocks 10&11 > we reserve 1 cluster, the i_reserved_data_blocks is 1 now > 2. delay-allocation write blocks 3 to 8 > we reserve other 2 clusters, so 3 clusters totally, the > i_reserved_data_blocks is 3 now > 3. writeout the blocks 3 to 8 > claim all 3 clusters, i_reserved_data_blocks is 0 now > At this moment, we really have allocated 3 clusters, and the > remain 10&11 block would never occupy another cluster (it would > definitely go into the [8-11] cluster), so, why need we reserve one > more cluster quota ? > 4. writeout the 10&11 blocks > the 10&11 blocks will be set EXT4_MAP_FROM_CLUSTER flag ( by > get_implied_cluster_alloc as the 8~9 block have been allocated), and > it will not call ext4_da_update_reserve_space any more > > As this, no warning, no reserve-and-release ping pong....at least in my imaging > I couldn't look into this in detail, but your logic seems totally fine. But when I wrote this code, I remember being able to reproduce the quota warnings by running xfstests (some corner case that needed this code). > Could you please point out what I missed ? > > Thanks > -- Aditya -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c index f9e2cd8..2c518f8 100644 --- a/fs/ext4/balloc.c +++ b/fs/ext4/balloc.c @@ -426,7 +426,7 @@ static int ext4_has_free_clusters(struct ext4_sb_info *sbi, if (free_clusters - (nclusters + root_clusters + dirty_clusters) < EXT4_FREECLUSTERS_WATERMARK) { - free_clusters = EXT4_C2B(sbi, percpu_counter_sum_positive(fcc)); + free_clusters = percpu_counter_sum_positive(fcc); dirty_clusters = percpu_counter_sum_positive(dcc); }