[PATCH 0/2] btrfs: defrag: don't defrag extents already at their max capacity, then remove an ambiguous check

[PATCH 0/2] btrfs: defrag: don't defrag extents already at their max capacity, then remove an ambiguous check

[Qu Wenruo]

Thanks to the report from Filipe where he found a new bug in compressed
defrag, that we will try to defrag all compressed extents even it's
already at its max compacity.

This behavior is from the beginning of btrfs defrag.

The first patch is to fix the behavior, by just rejecting extents
which are already at their max capacity, nor allowing extents to be
merged with extents at their max capacity.

The second patch is to remove an ambiguous rejection condition.
The condition is believed to reject compressed extent, but it never
really works due to the check is > 128K, not >= 128K.

And the physically adajcent check may prevent users to reduce the number
of extents.

Qu Wenruo (2):
  btrfs: defrag: don't defrag extents which is already at its max
    capacity
  btrfs: defrag: remove an ambiguous condition for rejection

 fs/btrfs/ioctl.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

-- 
2.34.1

[PATCH 1/2] btrfs: defrag: don't defrag extents which is already at its max capacity

[Qu Wenruo]

[BUG]
For compressed extents, defrag ioctl will always try to defrag any
compressed extents, wasting not only IO but also CPU time to
compress/decompress:

   mkfs.btrfs -f $DEV
   mount -o compress $DEV $MNT
   xfs_io -f -c "pwrite -S 0xab 0 128K" $MNT/foobar
   sync
   xfs_io -f -c "pwrite -S 0xcd 128K 128K" $MNT/foobar
   sync
   echo "=== before ==="
   xfs_io -c "fiemap -v" $MNT/foobar
   btrfs filesystem defrag $MNT/foobar
   sync
   echo "=== after ==="
   xfs_io -c "fiemap -v" $MNT/foobar

Then it shows the 2 128K extents just get CoW for no extra benefit, with
extra IO/CPU spent:

    === before ===
    /mnt/btrfs/file1:
     EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
       0: [0..255]:        26624..26879       256   0x8
       1: [256..511]:      26632..26887       256   0x9
    === after ===
    /mnt/btrfs/file1:
     EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
       0: [0..255]:        26640..26895       256   0x8
       1: [256..511]:      26648..26903       256   0x9

This affects not only v5.16 (after the defrag rework), but also v5.15
(before the defrag rework).

[CAUSE]
From the very beginning, btrfs defrag never checks if one extent is
already at its max capacity (128K for compressed extents, 128M
otherwise).

And the default extent size threshold is 256K, which is already beyond
the compressed extent max size.

This means, by default btrfs defrag ioctl will mark all compressed
extent which is not adjacent to a hole/preallocated range for defrag.

[FIX]
Introduce a helper to grab the maximum extent size, and then in
defrag_collect_targets() and defrag_check_next_extent(), reject extents
which are already at their max capacity.

Reported-by: Filipe Manana <fdmanana@suse.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/ioctl.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 137d3732af33..a03c31e1ff18 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1049,6 +1049,13 @@ static struct extent_map *defrag_lookup_extent(struct inode *inode, u64 start,
 	return em;
 }
 
+static u32 get_extent_max_capacity(struct extent_map *em)
+{
+	if (test_bit(EXTENT_FLAG_COMPRESSED, &em->flags))
+		return BTRFS_MAX_COMPRESSED;
+	return BTRFS_MAX_EXTENT_SIZE;
+}
+
 static bool defrag_check_next_extent(struct inode *inode, struct extent_map *em,
 				     bool locked)
 {
@@ -1065,6 +1072,12 @@ static bool defrag_check_next_extent(struct inode *inode, struct extent_map *em,
 		goto out;
 	if (test_bit(EXTENT_FLAG_PREALLOC, &em->flags))
 		goto out;
+	/*
+	 * If the next extent is at its max capcity, defragging current extent
+	 * makes no sense, as the total number of extents won't change.
+	 */
+	if (next->len >= get_extent_max_capacity(em))
+		goto out;
 	/* Physically adjacent and large enough */
 	if ((em->block_start + em->block_len == next->block_start) &&
 	    (em->block_len > SZ_128K && next->block_len > SZ_128K))
@@ -1229,6 +1242,13 @@ static int defrag_collect_targets(struct btrfs_inode *inode,
 		if (em->len >= extent_thresh)
 			goto next;
 
+		/*
+		 * Skip extents already at its max capacity, this is mostly for
+		 * compressed extents, which max cap is only 128K.
+		 */
+		if (em->len >= get_extent_max_capacity(em))
+			goto next;
+
 		next_mergeable = defrag_check_next_extent(&inode->vfs_inode, em,
 							  locked);
 		if (!next_mergeable) {
-- 
2.34.1

Re: [PATCH 1/2] btrfs: defrag: don't defrag extents which is already at its max capacity

[Filipe Manana]

On Thu, Jan 27, 2022 at 01:24:42PM +0800, Qu Wenruo wrote:
> [BUG]
> For compressed extents, defrag ioctl will always try to defrag any
> compressed extents, wasting not only IO but also CPU time to
> compress/decompress:
> 
>    mkfs.btrfs -f $DEV
>    mount -o compress $DEV $MNT
>    xfs_io -f -c "pwrite -S 0xab 0 128K" $MNT/foobar
>    sync
>    xfs_io -f -c "pwrite -S 0xcd 128K 128K" $MNT/foobar
>    sync
>    echo "=== before ==="
>    xfs_io -c "fiemap -v" $MNT/foobar
>    btrfs filesystem defrag $MNT/foobar
>    sync
>    echo "=== after ==="
>    xfs_io -c "fiemap -v" $MNT/foobar
> 
> Then it shows the 2 128K extents just get CoW for no extra benefit, with
> extra IO/CPU spent:
> 
>     === before ===
>     /mnt/btrfs/file1:
>      EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
>        0: [0..255]:        26624..26879       256   0x8
>        1: [256..511]:      26632..26887       256   0x9
>     === after ===
>     /mnt/btrfs/file1:
>      EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
>        0: [0..255]:        26640..26895       256   0x8
>        1: [256..511]:      26648..26903       256   0x9
> 
> This affects not only v5.16 (after the defrag rework), but also v5.15
> (before the defrag rework).
> 
> [CAUSE]
> From the very beginning, btrfs defrag never checks if one extent is
> already at its max capacity (128K for compressed extents, 128M
> otherwise).
> 
> And the default extent size threshold is 256K, which is already beyond
> the compressed extent max size.
> 
> This means, by default btrfs defrag ioctl will mark all compressed
> extent which is not adjacent to a hole/preallocated range for defrag.
> 
> [FIX]
> Introduce a helper to grab the maximum extent size, and then in
> defrag_collect_targets() and defrag_check_next_extent(), reject extents
> which are already at their max capacity.
> 
> Reported-by: Filipe Manana <fdmanana@suse.com>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/ioctl.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 137d3732af33..a03c31e1ff18 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -1049,6 +1049,13 @@ static struct extent_map *defrag_lookup_extent(struct inode *inode, u64 start,
>  	return em;
>  }
>  
> +static u32 get_extent_max_capacity(struct extent_map *em)

Could be made const.

Minor thing apart, which can updated when the patch is picked,

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

Thanks.

> +{
> +	if (test_bit(EXTENT_FLAG_COMPRESSED, &em->flags))
> +		return BTRFS_MAX_COMPRESSED;
> +	return BTRFS_MAX_EXTENT_SIZE;
> +}
> +
>  static bool defrag_check_next_extent(struct inode *inode, struct extent_map *em,
>  				     bool locked)
>  {
> @@ -1065,6 +1072,12 @@ static bool defrag_check_next_extent(struct inode *inode, struct extent_map *em,
>  		goto out;
>  	if (test_bit(EXTENT_FLAG_PREALLOC, &em->flags))
>  		goto out;
> +	/*
> +	 * If the next extent is at its max capcity, defragging current extent
> +	 * makes no sense, as the total number of extents won't change.
> +	 */
> +	if (next->len >= get_extent_max_capacity(em))
> +		goto out;
>  	/* Physically adjacent and large enough */
>  	if ((em->block_start + em->block_len == next->block_start) &&
>  	    (em->block_len > SZ_128K && next->block_len > SZ_128K))
> @@ -1229,6 +1242,13 @@ static int defrag_collect_targets(struct btrfs_inode *inode,
>  		if (em->len >= extent_thresh)
>  			goto next;
>  
> +		/*
> +		 * Skip extents already at its max capacity, this is mostly for
> +		 * compressed extents, which max cap is only 128K.
> +		 */
> +		if (em->len >= get_extent_max_capacity(em))
> +			goto next;
> +
>  		next_mergeable = defrag_check_next_extent(&inode->vfs_inode, em,
>  							  locked);
>  		if (!next_mergeable) {
> -- 
> 2.34.1
> 

[PATCH 2/2] btrfs: defrag: remove an ambiguous condition for rejection

[Qu Wenruo]

From the very beginning of btrfs defrag, there is a check to reject
extents which meet both conditions:

- Physically adjacent

  We may want to defrag physically adjacent extents to reduce the number
  of extents or the size of subvolume tree.

- Larger than 128K

  This may be there for compressed extents, but unfortunately 128K is
  exactly the max capacity for compressed extents.
  And the check is > 128K, thus it never rejects compressed extents.

  Furthermore, the compressed extent capacity bug is fixed by previous
  patch, there is no reason for that check anymore.

The original check has a very small ranges to reject (the target extent
size is > 128K, and default extent threshold is 256K), and for
compressed extent it doesn't work at all.

So it's better just to remove the rejection, and allow us to defrag
physically adjacent extents.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/ioctl.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index a03c31e1ff18..af95e3b7aa72 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1078,10 +1078,6 @@ static bool defrag_check_next_extent(struct inode *inode, struct extent_map *em,
 	 */
 	if (next->len >= get_extent_max_capacity(em))
 		goto out;
-	/* Physically adjacent and large enough */
-	if ((em->block_start + em->block_len == next->block_start) &&
-	    (em->block_len > SZ_128K && next->block_len > SZ_128K))
-		goto out;
 	ret = true;
 out:
 	free_extent_map(next);
-- 
2.34.1

Re: [PATCH 2/2] btrfs: defrag: remove an ambiguous condition for rejection

[Filipe Manana]

On Thu, Jan 27, 2022 at 01:24:43PM +0800, Qu Wenruo wrote:
> From the very beginning of btrfs defrag, there is a check to reject
> extents which meet both conditions:
> 
> - Physically adjacent
> 
>   We may want to defrag physically adjacent extents to reduce the number
>   of extents or the size of subvolume tree.
> 
> - Larger than 128K
> 
>   This may be there for compressed extents, but unfortunately 128K is
>   exactly the max capacity for compressed extents.
>   And the check is > 128K, thus it never rejects compressed extents.
> 
>   Furthermore, the compressed extent capacity bug is fixed by previous
>   patch, there is no reason for that check anymore.
> 
> The original check has a very small ranges to reject (the target extent
> size is > 128K, and default extent threshold is 256K), and for
> compressed extent it doesn't work at all.
> 
> So it's better just to remove the rejection, and allow us to defrag
> physically adjacent extents.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

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

Looks good now, thanks.

> ---
>  fs/btrfs/ioctl.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index a03c31e1ff18..af95e3b7aa72 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -1078,10 +1078,6 @@ static bool defrag_check_next_extent(struct inode *inode, struct extent_map *em,
>  	 */
>  	if (next->len >= get_extent_max_capacity(em))
>  		goto out;
> -	/* Physically adjacent and large enough */
> -	if ((em->block_start + em->block_len == next->block_start) &&
> -	    (em->block_len > SZ_128K && next->block_len > SZ_128K))
> -		goto out;
>  	ret = true;
>  out:
>  	free_extent_map(next);
> -- 
> 2.34.1
> 

Re: [PATCH 0/2] btrfs: defrag: don't defrag extents already at their max capacity, then remove an ambiguous check

[Filipe Manana]

On Thu, Jan 27, 2022 at 01:24:41PM +0800, Qu Wenruo wrote:
> Thanks to the report from Filipe where he found a new bug in compressed
> defrag, that we will try to defrag all compressed extents even it's
> already at its max compacity.
> 
> This behavior is from the beginning of btrfs defrag.
> 
> The first patch is to fix the behavior, by just rejecting extents
> which are already at their max capacity, nor allowing extents to be
> merged with extents at their max capacity.
> 
> The second patch is to remove an ambiguous rejection condition.
> The condition is believed to reject compressed extent, but it never
> really works due to the check is > 128K, not >= 128K.
> 
> And the physically adajcent check may prevent users to reduce the number
> of extents.

With so many patches sent in such short periods of time, with some in series
and others not in properly formatted series, it's likely hard for other people
to follow what's going on.

So this patchset, applies on top of the following patch:

https://patchwork.kernel.org/project/linux-btrfs/patch/20220126005850.14729-1-wqu@suse.com/

That patch is part of another series consisting of 3 patches, but this
patchset here makes the patches 2/3 and 3/3 of that other patchset
obsolete.

Thanks.

> 
> Qu Wenruo (2):
>   btrfs: defrag: don't defrag extents which is already at its max
>     capacity
>   btrfs: defrag: remove an ambiguous condition for rejection
> 
>  fs/btrfs/ioctl.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> -- 
> 2.34.1
>