| Message ID | 1374795726-28859-1-git-send-email-wenqing.lz@taobao.com |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 26, 2013 at 07:42:06AM +0800, Zheng Liu wrote: > From: Jan Kara <jack@suse.cz> > > Now in ext4_da_page_release_reservation() we remove the entry from es > tree if to_release != 0. But there are two issues. One is that it is > wrong when blocksize != pagesize, another is that we don't need to do > this if ->s_cluster_ratio == 1 because we will remove the entry in > ext4_truncate/ext4_punch_hole. Here we need to do this just because > when ->s_cluster_ratio > 1 we will determine whether we can release > the reserved space according to ext4_find_delalloc_cluster(). > > This commit tries to fix these problems. Now we remove the entry from > es tree only if ->s_cluster_ratio > 1. > > Signed-off-by: Jan Kara <jack@suse.cz> > Signed-off-by: Zheng Liu <wenqing.lz@taobao.com> Any comment? Thanks, - Zheng > --- > v2: > * fix a typo in comment. > > fs/ext4/inode.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > index ba33c67..2cc97a4 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -1387,7 +1387,15 @@ static void ext4_da_page_release_reservation(struct page *page, > curr_off = next_off; > } while ((bh = bh->b_this_page) != head); > > - if (to_release) { > + /* > + * Here we need to remove the entry from es tree because when bigalloc > + * is enabled we need to determine whether we can release the reserved > + * space according to the result of ext4_find_delalloc_cluster(). > + * > + * If bigalloc is disabled, we don't need to do this here because these > + * entries in es tree will be removed in ext4_truncate/ext4_punch_hole. > + */ > + if (sbi->s_cluster_ratio > 1 && to_release) { > lblk = page->index << (PAGE_CACHE_SHIFT - inode->i_blkbits); > ext4_es_remove_extent(inode, lblk, to_release); > } > -- > 1.7.9.7 > -- 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
On Fri, Jul 26, 2013 at 07:42:06AM +0800, Zheng Liu wrote: > From: Jan Kara <jack@suse.cz> > > Now in ext4_da_page_release_reservation() we remove the entry from es > tree if to_release != 0. But there are two issues. One is that it is > wrong when blocksize != pagesize, The commit description says that this is wrong, but I don't see anything in the patch which addresses this. And could you state what the impact is of this wrongness? Thanks, - Ted -- 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
On Mon 29-07-13 12:21:51, Ted Tso wrote: > On Fri, Jul 26, 2013 at 07:42:06AM +0800, Zheng Liu wrote: > > From: Jan Kara <jack@suse.cz> > > > > Now in ext4_da_page_release_reservation() we remove the entry from es > > tree if to_release != 0. But there are two issues. One is that it is > > wrong when blocksize != pagesize, > > The commit description says that this is wrong, but I don't see > anything in the patch which addresses this. And could you state what > the impact is of this wrongness? Well, this wrongness actually shouldn't have any real impact - for blocksize < pagesize the extent tree isn't used while truncate_pagecache() is running and ext4_truncate() then removes the whole truncated range from the tree again which hides any problems in ext4_da_page_release_reservation(). When bigalloc is used, we use the extent tree during truncate_pagecache() but to_release is always == 1 and thus the problem doesn't exist. Honza
On Mon, Jul 29, 2013 at 12:21:51PM -0400, Theodore Ts'o wrote: > On Fri, Jul 26, 2013 at 07:42:06AM +0800, Zheng Liu wrote: > > From: Jan Kara <jack@suse.cz> > > > > Now in ext4_da_page_release_reservation() we remove the entry from es > > tree if to_release != 0. But there are two issues. One is that it is > > wrong when blocksize != pagesize, > > The commit description says that this is wrong, but I don't see > anything in the patch which addresses this. And could you state what > the impact is of this wrongness? Sorry for my bad description. As Jan said, this patch doesn't address any issue. It just makes the code clearly. After applied this patch, I still get the warning messages from ext4_da_release_space running xfstests #74 when blocksize = 1k. Thanks, - Zheng -- 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
On Tue 30-07-13 07:44:25, Zheng Liu wrote: > Sorry for my bad description. As Jan said, this patch doesn't address > any issue. It just makes the code clearly. After applied this patch, > I still get the warning messages from ext4_da_release_space running > xfstests #74 when blocksize = 1k. Interesting. I never noticed those but now I see them as well. Dave Jones also reported this problem (although he didn't mention 1KB blocksize). I'll try to have a look into it. Honza
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index ba33c67..2cc97a4 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -1387,7 +1387,15 @@ static void ext4_da_page_release_reservation(struct page *page, curr_off = next_off; } while ((bh = bh->b_this_page) != head); - if (to_release) { + /* + * Here we need to remove the entry from es tree because when bigalloc + * is enabled we need to determine whether we can release the reserved + * space according to the result of ext4_find_delalloc_cluster(). + * + * If bigalloc is disabled, we don't need to do this here because these + * entries in es tree will be removed in ext4_truncate/ext4_punch_hole. + */ + if (sbi->s_cluster_ratio > 1 && to_release) { lblk = page->index << (PAGE_CACHE_SHIFT - inode->i_blkbits); ext4_es_remove_extent(inode, lblk, to_release); }