* [PATCH] ext4: Remove extent tree purging from ext4_da_page_release_reservation()
@ 2013-07-17 22:10 Jan Kara
2013-07-19 0:44 ` Zheng Liu
0 siblings, 1 reply; 6+ messages in thread
From: Jan Kara @ 2013-07-17 22:10 UTC (permalink / raw)
To: Ted Tso; +Cc: linux-ext4, Jan Kara, Zheng Liu
ext4_da_page_release_reservation() gets called from
ext4_da_invalidatepage(). This function is used when we are truncating
page cache for punch hole or truncate operations. In either case these
operations take care of removing extents from the extent tree. This is
more efficient and the code in ext4_da_page_release_reservation() is
actually buggy anyway. So just remove it.
CC: Zheng Liu <wenqing.lz@taobao.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
fs/ext4/inode.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 0188e65..98a9972 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1388,11 +1388,6 @@ static void ext4_da_page_release_reservation(struct page *page,
curr_off = next_off;
} while ((bh = bh->b_this_page) != head);
- if (to_release) {
- lblk = page->index << (PAGE_CACHE_SHIFT - inode->i_blkbits);
- ext4_es_remove_extent(inode, lblk, to_release);
- }
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: Remove extent tree purging from ext4_da_page_release_reservation()
2013-07-17 22:10 [PATCH] ext4: Remove extent tree purging from ext4_da_page_release_reservation() Jan Kara
@ 2013-07-19 0:44 ` Zheng Liu
2013-07-24 20:05 ` Jan Kara
0 siblings, 1 reply; 6+ messages in thread
From: Zheng Liu @ 2013-07-19 0:44 UTC (permalink / raw)
To: Jan Kara; +Cc: Ted Tso, linux-ext4, Zheng Liu
Hi Jan,
Sorry for the late reply.
On Thu, Jul 18, 2013 at 12:10:15AM +0200, Jan Kara wrote:
> ext4_da_page_release_reservation() gets called from
> ext4_da_invalidatepage(). This function is used when we are truncating
> page cache for punch hole or truncate operations. In either case these
> operations take care of removing extents from the extent tree. This is
> more efficient and the code in ext4_da_page_release_reservation() is
> actually buggy anyway. So just remove it.
I remember that I try to remove the entry from extent status tree here
because at the end of this function it tries to relase the reserved
space for delalloc. For 4k block we can simply release it because
->s_cluster_ratio == 1. But when bigalloc is enabled, we need to
determine whether we can release the reserved space according to the
result of ext4_find_delalloc_cluster() as the comment described. If we
don't remove the entry from extent status tree here, we could lost some
spaces that could be reused by other files. If I remember correctly, I
have hitted a warning message when I run xfstests to test it. These
days I try to trigger it using xfstests but I failed. Have you seen a
prblem that is caused by this code? Maybe we need to refactor out the
code and release the reserved space outside this function.
- Zheng
>
> CC: Zheng Liu <wenqing.lz@taobao.com>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
> fs/ext4/inode.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 0188e65..98a9972 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1388,11 +1388,6 @@ static void ext4_da_page_release_reservation(struct page *page,
> curr_off = next_off;
> } while ((bh = bh->b_this_page) != head);
>
> - if (to_release) {
> - lblk = page->index << (PAGE_CACHE_SHIFT - inode->i_blkbits);
> - ext4_es_remove_extent(inode, lblk, to_release);
> - }
> -
> /* If we have released all the blocks belonging to a cluster, then we
> * need to release the reserved space for that cluster. */
> num_clusters = EXT4_NUM_B2C(sbi, to_release);
> --
> 1.8.1.4
>
> --
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: Remove extent tree purging from ext4_da_page_release_reservation()
2013-07-19 0:44 ` Zheng Liu
@ 2013-07-24 20:05 ` Jan Kara
2013-07-25 11:52 ` Zheng Liu
0 siblings, 1 reply; 6+ messages in thread
From: Jan Kara @ 2013-07-24 20:05 UTC (permalink / raw)
To: Zheng Liu; +Cc: Jan Kara, Ted Tso, linux-ext4, Zheng Liu
Hi Zheng,
On Fri 19-07-13 08:44:39, Zheng Liu wrote:
> On Thu, Jul 18, 2013 at 12:10:15AM +0200, Jan Kara wrote:
> > ext4_da_page_release_reservation() gets called from
> > ext4_da_invalidatepage(). This function is used when we are truncating
> > page cache for punch hole or truncate operations. In either case these
> > operations take care of removing extents from the extent tree. This is
> > more efficient and the code in ext4_da_page_release_reservation() is
> > actually buggy anyway. So just remove it.
>
> I remember that I try to remove the entry from extent status tree here
> because at the end of this function it tries to relase the reserved
> space for delalloc. For 4k block we can simply release it because
> ->s_cluster_ratio == 1. But when bigalloc is enabled, we need to
> determine whether we can release the reserved space according to the
> result of ext4_find_delalloc_cluster() as the comment described. If we
> don't remove the entry from extent status tree here, we could lost some
> spaces that could be reused by other files. If I remember correctly, I
> have hitted a warning message when I run xfstests to test it. These
> days I try to trigger it using xfstests but I failed. Have you seen a
> prblem that is caused by this code? Maybe we need to refactor out the
> code and release the reserved space outside this function.
Ah, I see. No, I didn't observe any problem due to this code, I just
didn't understand why is it there. Also when blocksize < pagesize, the code
is wrong because delayed buffers to release need not be contiguous so
ext4_es_remove_extent(inode, lblk, to_release) may not free all the buffers
we want. But subsequent extent tree truncation in ext4_ext_truncate() hides
this problem.
So I think we might just change the condition:
if (to_release) {
to
if (to_release && sbi->s_cluster_ratio > 1) {
and add explanatory comment why cluster_ratio > 1 needs the truncation and
other cases don't. It will also save some needlessly burned CPU cycles
spent when manipulating extent tree.
Honza
--
Jan Kara <jack@suse.cz>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: Remove extent tree purging from ext4_da_page_release_reservation()
2013-07-24 20:05 ` Jan Kara
@ 2013-07-25 11:52 ` Zheng Liu
2013-07-25 14:05 ` Jan Kara
0 siblings, 1 reply; 6+ messages in thread
From: Zheng Liu @ 2013-07-25 11:52 UTC (permalink / raw)
To: Jan Kara; +Cc: Ted Tso, linux-ext4, Zheng Liu
On Wed, Jul 24, 2013 at 10:05:36PM +0200, Jan Kara wrote:
> Hi Zheng,
>
> On Fri 19-07-13 08:44:39, Zheng Liu wrote:
> > On Thu, Jul 18, 2013 at 12:10:15AM +0200, Jan Kara wrote:
> > > ext4_da_page_release_reservation() gets called from
> > > ext4_da_invalidatepage(). This function is used when we are truncating
> > > page cache for punch hole or truncate operations. In either case these
> > > operations take care of removing extents from the extent tree. This is
> > > more efficient and the code in ext4_da_page_release_reservation() is
> > > actually buggy anyway. So just remove it.
> >
> > I remember that I try to remove the entry from extent status tree here
> > because at the end of this function it tries to relase the reserved
> > space for delalloc. For 4k block we can simply release it because
> > ->s_cluster_ratio == 1. But when bigalloc is enabled, we need to
> > determine whether we can release the reserved space according to the
> > result of ext4_find_delalloc_cluster() as the comment described. If we
> > don't remove the entry from extent status tree here, we could lost some
> > spaces that could be reused by other files. If I remember correctly, I
> > have hitted a warning message when I run xfstests to test it. These
> > days I try to trigger it using xfstests but I failed. Have you seen a
> > prblem that is caused by this code? Maybe we need to refactor out the
> > code and release the reserved space outside this function.
> Ah, I see. No, I didn't observe any problem due to this code, I just
> didn't understand why is it there. Also when blocksize < pagesize, the code
> is wrong because delayed buffers to release need not be contiguous so
> ext4_es_remove_extent(inode, lblk, to_release) may not free all the buffers
> we want. But subsequent extent tree truncation in ext4_ext_truncate() hides
> this problem.
>
> So I think we might just change the condition:
>
> if (to_release) {
>
> to
>
> if (to_release && sbi->s_cluster_ratio > 1) {
>
> and add explanatory comment why cluster_ratio > 1 needs the truncation and
> other cases don't. It will also save some needlessly burned CPU cycles
> spent when manipulating extent tree.
Yes, thanks for pointing it out. I attach a patch below. Could you
please review it?
Thanks,
- Zheng
Subject: [PATCH 1/2] ext4: remove the entry from es tree when bigalloc is enabled
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>
---
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..e0c8623 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
+ * extries 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: Remove extent tree purging from ext4_da_page_release_reservation()
2013-07-25 11:52 ` Zheng Liu
@ 2013-07-25 14:05 ` Jan Kara
2013-07-25 23:36 ` Zheng Liu
0 siblings, 1 reply; 6+ messages in thread
From: Jan Kara @ 2013-07-25 14:05 UTC (permalink / raw)
To: Zheng Liu; +Cc: Jan Kara, Ted Tso, linux-ext4, Zheng Liu
On Thu 25-07-13 19:52:34, Zheng Liu wrote:
> On Wed, Jul 24, 2013 at 10:05:36PM +0200, Jan Kara wrote:
> > Hi Zheng,
> >
> > On Fri 19-07-13 08:44:39, Zheng Liu wrote:
> > > On Thu, Jul 18, 2013 at 12:10:15AM +0200, Jan Kara wrote:
> > > > ext4_da_page_release_reservation() gets called from
> > > > ext4_da_invalidatepage(). This function is used when we are truncating
> > > > page cache for punch hole or truncate operations. In either case these
> > > > operations take care of removing extents from the extent tree. This is
> > > > more efficient and the code in ext4_da_page_release_reservation() is
> > > > actually buggy anyway. So just remove it.
> > >
> > > I remember that I try to remove the entry from extent status tree here
> > > because at the end of this function it tries to relase the reserved
> > > space for delalloc. For 4k block we can simply release it because
> > > ->s_cluster_ratio == 1. But when bigalloc is enabled, we need to
> > > determine whether we can release the reserved space according to the
> > > result of ext4_find_delalloc_cluster() as the comment described. If we
> > > don't remove the entry from extent status tree here, we could lost some
> > > spaces that could be reused by other files. If I remember correctly, I
> > > have hitted a warning message when I run xfstests to test it. These
> > > days I try to trigger it using xfstests but I failed. Have you seen a
> > > prblem that is caused by this code? Maybe we need to refactor out the
> > > code and release the reserved space outside this function.
> > Ah, I see. No, I didn't observe any problem due to this code, I just
> > didn't understand why is it there. Also when blocksize < pagesize, the code
> > is wrong because delayed buffers to release need not be contiguous so
> > ext4_es_remove_extent(inode, lblk, to_release) may not free all the buffers
> > we want. But subsequent extent tree truncation in ext4_ext_truncate() hides
> > this problem.
> >
> > So I think we might just change the condition:
> >
> > if (to_release) {
> >
> > to
> >
> > if (to_release && sbi->s_cluster_ratio > 1) {
> >
> > and add explanatory comment why cluster_ratio > 1 needs the truncation and
> > other cases don't. It will also save some needlessly burned CPU cycles
> > spent when manipulating extent tree.
>
> Yes, thanks for pointing it out. I attach a patch below. Could you
> please review it?
Thanks for writing the patch! It looks good. BTW, feel free to take
authorship of it. I've just pointed out the problem. One typo correction
below:
> Subject: [PATCH 1/2] ext4: remove the entry from es tree when bigalloc is enabled
>
> 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>
> ---
> 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..e0c8623 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
> + * extries in es tree will be removed in ext4_truncate/ext4_punch_hole.
^^^ entries
> + */
> + 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);
> }
Honza
--
Jan Kara <jack@suse.cz>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ext4: Remove extent tree purging from ext4_da_page_release_reservation()
2013-07-25 14:05 ` Jan Kara
@ 2013-07-25 23:36 ` Zheng Liu
0 siblings, 0 replies; 6+ messages in thread
From: Zheng Liu @ 2013-07-25 23:36 UTC (permalink / raw)
To: Jan Kara; +Cc: Ted Tso, linux-ext4, Zheng Liu
On Thu, Jul 25, 2013 at 04:05:28PM +0200, Jan Kara wrote:
[...]
> > Yes, thanks for pointing it out. I attach a patch below. Could you
> > please review it?
> Thanks for writing the patch! It looks good. BTW, feel free to take
> authorship of it. I've just pointed out the problem. One typo correction
> below:
Thanks for your review. The latest patch will be sent out soon.
- Zheng
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-07-25 23:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-17 22:10 [PATCH] ext4: Remove extent tree purging from ext4_da_page_release_reservation() Jan Kara
2013-07-19 0:44 ` Zheng Liu
2013-07-24 20:05 ` Jan Kara
2013-07-25 11:52 ` Zheng Liu
2013-07-25 14:05 ` Jan Kara
2013-07-25 23:36 ` Zheng Liu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.