[PATCH] btrfs: update writeback index when starting defrag

[PATCH] btrfs: update writeback index when starting defrag

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

When starting a defrag, we should update the writeback index of the
inode's mapping in case it currently has a value beyond the start of the
range we are defragging. This can help performance and often result in
getting less extents after writeback - for e.g., if the current value
of the writeback index sits somewhere in the middle of a range that
gets dirty by the defrag, then after writeback we can get two smaller
extents instead of a single, larger extent.

We used to have this before the refactoring in 5.16, but it was removed
without any reason to do so. Orginally it was added in kernel 3.1, by
commit 2a0f7f5769992b ("Btrfs: fix recursive auto-defrag"), in order to
fix a loop with autodefrag resulting in dirtying and writing pages over
and over, but some testing on current code did not show that happening,
at least with the test described in that commit.

So add back the behaviour, as at the very least it is a nice to have
optimization.

Fixes: 7b508037d4cac3 ("btrfs: defrag: use defrag_one_cluster() to implement btrfs_defrag_file()")
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/ioctl.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index bfe5ed65e92b..95d0e210f063 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1535,6 +1535,7 @@ int btrfs_defrag_file(struct inode *inode, struct file_ra_state *ra,
 	int compress_type = BTRFS_COMPRESS_ZLIB;
 	int ret = 0;
 	u32 extent_thresh = range->extent_thresh;
+	pgoff_t start_index;
 
 	if (isize == 0)
 		return 0;
@@ -1576,6 +1577,14 @@ int btrfs_defrag_file(struct inode *inode, struct file_ra_state *ra,
 			file_ra_state_init(ra, inode->i_mapping);
 	}
 
+	/*
+	 * Make writeback start from the beginning of the range, so that the
+	 * defrag range can be written sequentially.
+	 */
+	start_index = cur >> PAGE_SHIFT;
+	if (start_index < inode->i_mapping->writeback_index)
+		inode->i_mapping->writeback_index = start_index;
+
 	while (cur < last_byte) {
 		const unsigned long prev_sectors_defragged = sectors_defragged;
 		u64 cluster_end;
-- 
2.33.0

Re: [PATCH] btrfs: update writeback index when starting defrag

[Qu Wenruo]

On 2022/1/21 01:41, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
>
> When starting a defrag, we should update the writeback index of the
> inode's mapping in case it currently has a value beyond the start of the
> range we are defragging. This can help performance and often result in
> getting less extents after writeback - for e.g., if the current value
> of the writeback index sits somewhere in the middle of a range that
> gets dirty by the defrag, then after writeback we can get two smaller
> extents instead of a single, larger extent.
>
> We used to have this before the refactoring in 5.16, but it was removed
> without any reason to do so. Orginally it was added in kernel 3.1, by
> commit 2a0f7f5769992b ("Btrfs: fix recursive auto-defrag"), in order to
> fix a loop with autodefrag resulting in dirtying and writing pages over
> and over, but some testing on current code did not show that happening,
> at least with the test described in that commit.
>
> So add back the behaviour, as at the very least it is a nice to have
> optimization.

Writeback_index is always one mystery to me.

In fact just re-checking the writeback_index usage, I found the metadata
writeback is reading that value just like data writeback.
But we don't have any call sites to set writeback_index for btree inode.

Is there any better doc for the proper behavior for writeback_index?

Thanks,
Qu

>
> Fixes: 7b508037d4cac3 ("btrfs: defrag: use defrag_one_cluster() to implement btrfs_defrag_file()")
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
>   fs/btrfs/ioctl.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
>
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index bfe5ed65e92b..95d0e210f063 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -1535,6 +1535,7 @@ int btrfs_defrag_file(struct inode *inode, struct file_ra_state *ra,
>   	int compress_type = BTRFS_COMPRESS_ZLIB;
>   	int ret = 0;
>   	u32 extent_thresh = range->extent_thresh;
> +	pgoff_t start_index;
>
>   	if (isize == 0)
>   		return 0;
> @@ -1576,6 +1577,14 @@ int btrfs_defrag_file(struct inode *inode, struct file_ra_state *ra,
>   			file_ra_state_init(ra, inode->i_mapping);
>   	}
>
> +	/*
> +	 * Make writeback start from the beginning of the range, so that the
> +	 * defrag range can be written sequentially.
> +	 */
> +	start_index = cur >> PAGE_SHIFT;
> +	if (start_index < inode->i_mapping->writeback_index)
> +		inode->i_mapping->writeback_index = start_index;
> +
>   	while (cur < last_byte) {
>   		const unsigned long prev_sectors_defragged = sectors_defragged;
>   		u64 cluster_end;

Re: [PATCH] btrfs: update writeback index when starting defrag

[Filipe Manana]

On Fri, Jan 21, 2022 at 12:03 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>
>
>
> On 2022/1/21 01:41, fdmanana@kernel.org wrote:
> > From: Filipe Manana <fdmanana@suse.com>
> >
> > When starting a defrag, we should update the writeback index of the
> > inode's mapping in case it currently has a value beyond the start of the
> > range we are defragging. This can help performance and often result in
> > getting less extents after writeback - for e.g., if the current value
> > of the writeback index sits somewhere in the middle of a range that
> > gets dirty by the defrag, then after writeback we can get two smaller
> > extents instead of a single, larger extent.
> >
> > We used to have this before the refactoring in 5.16, but it was removed
> > without any reason to do so. Orginally it was added in kernel 3.1, by
> > commit 2a0f7f5769992b ("Btrfs: fix recursive auto-defrag"), in order to
> > fix a loop with autodefrag resulting in dirtying and writing pages over
> > and over, but some testing on current code did not show that happening,
> > at least with the test described in that commit.
> >
> > So add back the behaviour, as at the very least it is a nice to have
> > optimization.
>
> Writeback_index is always one mystery to me.
>
> In fact just re-checking the writeback_index usage, I found the metadata
> writeback is reading that value just like data writeback.
> But we don't have any call sites to set writeback_index for btree inode.
>
> Is there any better doc for the proper behavior for writeback_index?

I don't know... maybe somewhere in the generic writeback code there
are comments.

>
> Thanks,
> Qu
>
> >
> > Fixes: 7b508037d4cac3 ("btrfs: defrag: use defrag_one_cluster() to implement btrfs_defrag_file()")
> > Signed-off-by: Filipe Manana <fdmanana@suse.com>
> > ---
> >   fs/btrfs/ioctl.c | 9 +++++++++
> >   1 file changed, 9 insertions(+)
> >
> > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> > index bfe5ed65e92b..95d0e210f063 100644
> > --- a/fs/btrfs/ioctl.c
> > +++ b/fs/btrfs/ioctl.c
> > @@ -1535,6 +1535,7 @@ int btrfs_defrag_file(struct inode *inode, struct file_ra_state *ra,
> >       int compress_type = BTRFS_COMPRESS_ZLIB;
> >       int ret = 0;
> >       u32 extent_thresh = range->extent_thresh;
> > +     pgoff_t start_index;
> >
> >       if (isize == 0)
> >               return 0;
> > @@ -1576,6 +1577,14 @@ int btrfs_defrag_file(struct inode *inode, struct file_ra_state *ra,
> >                       file_ra_state_init(ra, inode->i_mapping);
> >       }
> >
> > +     /*
> > +      * Make writeback start from the beginning of the range, so that the
> > +      * defrag range can be written sequentially.
> > +      */
> > +     start_index = cur >> PAGE_SHIFT;
> > +     if (start_index < inode->i_mapping->writeback_index)
> > +             inode->i_mapping->writeback_index = start_index;
> > +
> >       while (cur < last_byte) {
> >               const unsigned long prev_sectors_defragged = sectors_defragged;
> >               u64 cluster_end;

Re: [PATCH] btrfs: update writeback index when starting defrag

[David Sterba]

On Thu, Jan 20, 2022 at 05:41:17PM +0000, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> When starting a defrag, we should update the writeback index of the
> inode's mapping in case it currently has a value beyond the start of the
> range we are defragging. This can help performance and often result in
> getting less extents after writeback - for e.g., if the current value
> of the writeback index sits somewhere in the middle of a range that
> gets dirty by the defrag, then after writeback we can get two smaller
> extents instead of a single, larger extent.
> 
> We used to have this before the refactoring in 5.16, but it was removed
> without any reason to do so. Orginally it was added in kernel 3.1, by
> commit 2a0f7f5769992b ("Btrfs: fix recursive auto-defrag"), in order to
> fix a loop with autodefrag resulting in dirtying and writing pages over
> and over, but some testing on current code did not show that happening,
> at least with the test described in that commit.
> 
> So add back the behaviour, as at the very least it is a nice to have
> optimization.
> 
> Fixes: 7b508037d4cac3 ("btrfs: defrag: use defrag_one_cluster() to implement btrfs_defrag_file()")
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Added to misc-next, thank.