[PATCH] btrfs: defrag: properly update range->start for autodefrag

[PATCH] btrfs: defrag: properly update range->start for autodefrag

[Qu Wenruo]

[BUG]
After commit 7b508037d4ca ("btrfs: defrag: use defrag_one_cluster() to
implement btrfs_defrag_file()") autodefrag no longer properly re-defrag
the file from previously finished location.

[CAUSE]
The recent refactor on defrag only focuses on defrag ioctl subpage
support, doesn't take auto defrag into consideration.

There are two problems involved which prevents autodefrag to restart its
scan:

- No range.start update
  Previously when one defrag target is found, range->start will be
  updated to indicate where next search should start from.

  But now btrfs_defrag_file() doesn't update it anymore, making all
  autodefrag to rescan from file offset 0.

  This would also make autodefrag to mark the same range dirty again and
  again, causing extra IO.

- No proper quick exit for defrag_one_cluster()
  Currently if we reached or exceed @max_sectors limit, we just exit
  defrag_one_cluster(), and let next defrag_one_cluster() call to do a
  quick exit.
  This makes @cur increase, thus no way to properly know which range is
  defragged and which range is skipped.

[FIX]
The fix involves two modifications:

- Update range->start to next cluster start
  This is a little different from the old behavior.
  Previously range->start is updated to the next defrag target.

  But in the end, the behavior should still be pretty much the same,
  as now we skip to next defrag target inside btrfs_defrag_file().

  Thus if auto-defrag determines to re-scan, then we still do the skip,
  just at a different timing.

- Make defrag_one_cluster() to return >0 to indicate a quick exit
  So that btrfs_defrag_file() can also do a quick exit, without
  increasing @cur to the range end, and re-use @cur to update
  @range->start.

- Add comment for btrfs_defrag_file() to mention the range->start update
  Currently only autodefrag utilize this behavior, as defrag ioctl won't
  set @max_to_defrag parameter, thus unless interrupted it will always
  try to defrag the whole range.

Reported-by: Filipe Manana <fdmanana@suse.com>
Fixes: 7b508037d4ca ("btrfs: defrag: use defrag_one_cluster() to implement btrfs_defrag_file()")
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/ioctl.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 2a77273d91fe..91ba2efe9792 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1443,8 +1443,10 @@ static int defrag_one_cluster(struct btrfs_inode *inode,
 		u32 range_len = entry->len;
 
 		/* Reached or beyond the limit */
-		if (max_sectors && *sectors_defragged >= max_sectors)
+		if (max_sectors && *sectors_defragged >= max_sectors) {
+			ret = 1;
 			break;
+		}
 
 		if (max_sectors)
 			range_len = min_t(u32, range_len,
@@ -1487,7 +1489,10 @@ static int defrag_one_cluster(struct btrfs_inode *inode,
  *		   will be defragged.
  *
  * Return <0 for error.
- * Return >=0 for the number of sectors defragged.
+ * Return >=0 for the number of sectors defragged, and range->start will be updated
+ * to indicate the file offset where next defrag should be started at.
+ * (Mostly for autodefrag, which sets @max_to_defrag thus we may exit early without
+ *  defragging all the range).
  */
 int btrfs_defrag_file(struct inode *inode, struct file_ra_state *ra,
 		      struct btrfs_ioctl_defrag_range_args *range,
@@ -1575,10 +1580,19 @@ int btrfs_defrag_file(struct inode *inode, struct file_ra_state *ra,
 		if (ret < 0)
 			break;
 		cur = cluster_end + 1;
+		if (ret > 0) {
+			ret = 0;
+			break;
+		}
 	}
 
 	if (ra_allocated)
 		kfree(ra);
+	/*
+	 * Update range.start for autodefrag, this will indicate where to start
+	 * in next run.
+	 */
+	range->start = cur;
 	if (sectors_defragged) {
 		/*
 		 * We have defragged some sectors, for compression case they
-- 
2.34.1

Re: [PATCH] btrfs: defrag: properly update range->start for autodefrag

[Filipe Manana]

On Tue, Jan 18, 2022 at 07:53:52PM +0800, Qu Wenruo wrote:
> [BUG]
> After commit 7b508037d4ca ("btrfs: defrag: use defrag_one_cluster() to
> implement btrfs_defrag_file()") autodefrag no longer properly re-defrag
> the file from previously finished location.
> 
> [CAUSE]
> The recent refactor on defrag only focuses on defrag ioctl subpage
> support, doesn't take auto defrag into consideration.
> 
> There are two problems involved which prevents autodefrag to restart its
> scan:
> 
> - No range.start update
>   Previously when one defrag target is found, range->start will be
>   updated to indicate where next search should start from.
> 
>   But now btrfs_defrag_file() doesn't update it anymore, making all
>   autodefrag to rescan from file offset 0.
> 
>   This would also make autodefrag to mark the same range dirty again and
>   again, causing extra IO.
> 
> - No proper quick exit for defrag_one_cluster()
>   Currently if we reached or exceed @max_sectors limit, we just exit
>   defrag_one_cluster(), and let next defrag_one_cluster() call to do a
>   quick exit.
>   This makes @cur increase, thus no way to properly know which range is
>   defragged and which range is skipped.
> 
> [FIX]
> The fix involves two modifications:
> 
> - Update range->start to next cluster start
>   This is a little different from the old behavior.
>   Previously range->start is updated to the next defrag target.
> 
>   But in the end, the behavior should still be pretty much the same,
>   as now we skip to next defrag target inside btrfs_defrag_file().
> 
>   Thus if auto-defrag determines to re-scan, then we still do the skip,
>   just at a different timing.
> 
> - Make defrag_one_cluster() to return >0 to indicate a quick exit
>   So that btrfs_defrag_file() can also do a quick exit, without
>   increasing @cur to the range end, and re-use @cur to update
>   @range->start.
> 
> - Add comment for btrfs_defrag_file() to mention the range->start update
>   Currently only autodefrag utilize this behavior, as defrag ioctl won't
>   set @max_to_defrag parameter, thus unless interrupted it will always
>   try to defrag the whole range.
> 
> Reported-by: Filipe Manana <fdmanana@suse.com>
> Fixes: 7b508037d4ca ("btrfs: defrag: use defrag_one_cluster() to implement btrfs_defrag_file()")
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Looks good, thanks.

Reviewed-by: Filipe Manana <fdmanana@suse.com>

As mentioned in the other patch, please add a Link tag to the user's report:

Link: https://lore.kernel.org/linux-btrfs/0a269612-e43f-da22-c5bc-b34b1b56ebe8@mailbox.org/

> ---
>  fs/btrfs/ioctl.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 2a77273d91fe..91ba2efe9792 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -1443,8 +1443,10 @@ static int defrag_one_cluster(struct btrfs_inode *inode,
>  		u32 range_len = entry->len;
>  
>  		/* Reached or beyond the limit */
> -		if (max_sectors && *sectors_defragged >= max_sectors)
> +		if (max_sectors && *sectors_defragged >= max_sectors) {
> +			ret = 1;
>  			break;
> +		}
>  
>  		if (max_sectors)
>  			range_len = min_t(u32, range_len,
> @@ -1487,7 +1489,10 @@ static int defrag_one_cluster(struct btrfs_inode *inode,
>   *		   will be defragged.
>   *
>   * Return <0 for error.
> - * Return >=0 for the number of sectors defragged.
> + * Return >=0 for the number of sectors defragged, and range->start will be updated
> + * to indicate the file offset where next defrag should be started at.
> + * (Mostly for autodefrag, which sets @max_to_defrag thus we may exit early without
> + *  defragging all the range).
>   */
>  int btrfs_defrag_file(struct inode *inode, struct file_ra_state *ra,
>  		      struct btrfs_ioctl_defrag_range_args *range,
> @@ -1575,10 +1580,19 @@ int btrfs_defrag_file(struct inode *inode, struct file_ra_state *ra,
>  		if (ret < 0)
>  			break;
>  		cur = cluster_end + 1;
> +		if (ret > 0) {
> +			ret = 0;
> +			break;
> +		}
>  	}
>  
>  	if (ra_allocated)
>  		kfree(ra);
> +	/*
> +	 * Update range.start for autodefrag, this will indicate where to start
> +	 * in next run.
> +	 */
> +	range->start = cur;
>  	if (sectors_defragged) {
>  		/*
>  		 * We have defragged some sectors, for compression case they
> -- 
> 2.34.1
> 

Re: [PATCH] btrfs: defrag: properly update range->start for autodefrag

[David Sterba]

On Tue, Jan 18, 2022 at 07:53:52PM +0800, Qu Wenruo wrote:
> [BUG]
> After commit 7b508037d4ca ("btrfs: defrag: use defrag_one_cluster() to
> implement btrfs_defrag_file()") autodefrag no longer properly re-defrag
> the file from previously finished location.
> 
> [CAUSE]
> The recent refactor on defrag only focuses on defrag ioctl subpage
> support, doesn't take auto defrag into consideration.
> 
> There are two problems involved which prevents autodefrag to restart its
> scan:
> 
> - No range.start update
>   Previously when one defrag target is found, range->start will be
>   updated to indicate where next search should start from.
> 
>   But now btrfs_defrag_file() doesn't update it anymore, making all
>   autodefrag to rescan from file offset 0.
> 
>   This would also make autodefrag to mark the same range dirty again and
>   again, causing extra IO.
> 
> - No proper quick exit for defrag_one_cluster()
>   Currently if we reached or exceed @max_sectors limit, we just exit
>   defrag_one_cluster(), and let next defrag_one_cluster() call to do a
>   quick exit.
>   This makes @cur increase, thus no way to properly know which range is
>   defragged and which range is skipped.
> 
> [FIX]
> The fix involves two modifications:
> 
> - Update range->start to next cluster start
>   This is a little different from the old behavior.
>   Previously range->start is updated to the next defrag target.
> 
>   But in the end, the behavior should still be pretty much the same,
>   as now we skip to next defrag target inside btrfs_defrag_file().
> 
>   Thus if auto-defrag determines to re-scan, then we still do the skip,
>   just at a different timing.
> 
> - Make defrag_one_cluster() to return >0 to indicate a quick exit
>   So that btrfs_defrag_file() can also do a quick exit, without
>   increasing @cur to the range end, and re-use @cur to update
>   @range->start.
> 
> - Add comment for btrfs_defrag_file() to mention the range->start update
>   Currently only autodefrag utilize this behavior, as defrag ioctl won't
>   set @max_to_defrag parameter, thus unless interrupted it will always
>   try to defrag the whole range.
> 
> Reported-by: Filipe Manana <fdmanana@suse.com>
> Fixes: 7b508037d4ca ("btrfs: defrag: use defrag_one_cluster() to implement btrfs_defrag_file()")
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Added to misc-next, thanks.