[PATCH v4 0/3] btrfs: fixes for defrag_check_next_extent()

[PATCH v4 0/3] btrfs: fixes for defrag_check_next_extent()

[Qu Wenruo]

That function is reused between older kernels (v5.15) and the refactored
defrag code (v5.16+).

However that function has one long existing bugs affecting defrag to
handle preallocated range.

And it can not handle compressed extent well neither.

Finally there is an ambiguous check which doesn't make much sense by
itself, and can be related by enhanced extent capacity check.

This series will fix all the 3 problem mentioned above.

Changelog:
v2:
- Use @extent_thresh from caller to replace the harded coded threshold
  Now caller has full control over the extent threshold value.

- Remove the old ambiguous check based on physical address
  The original check is too specific, only reject extents which are
  physically adjacent, AND too large.
  Since we have correct size check now, and the physically adjacent check
  is not always a win.
  So remove the old check completely.

v3:
- Split the @extent_thresh and physicall adjacent check into other
  patches

- Simplify the comment 

v4:
- Fix the @em usage which should be @next.
  As it will fail the submitted test case.

Qu Wenruo (3):
  btrfs: defrag: don't try to merge regular extents with preallocated
    extents
  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 | 35 ++++++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

-- 
2.34.1

[PATCH v4 1/3] btrfs: defrag: don't try to merge regular extents with preallocated extents

[Qu Wenruo]

[BUG]
With older kernels (before v5.16), btrfs will defrag preallocated extents.
While with newer kernels (v5.16 and newer) btrfs will not defrag
preallocated extents, but it will defrag the extent just before the
preallocated extent, even it's just a single sector.

This can be exposed by the following small script:

	mkfs.btrfs -f $dev > /dev/null

	mount $dev $mnt
	xfs_io -f -c "pwrite 0 4k" -c sync -c "falloc 4k 16K" $mnt/file
	xfs_io -c "fiemap -v" $mnt/file
	btrfs fi defrag $mnt/file
	sync
	xfs_io -c "fiemap -v" $mnt/file

The output looks like this on older kernels:

/mnt/btrfs/file:
 EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
   0: [0..7]:          26624..26631         8   0x0
   1: [8..39]:         26632..26663        32 0x801
/mnt/btrfs/file:
 EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
   0: [0..39]:         26664..26703        40   0x1

Which defrags the single sector along with the preallocated extent, and
replace them with an regular extent into a new location (caused by data
COW).
This wastes most of the data IO just for the preallocated range.

On the other hand, v5.16 is slightly better:

/mnt/btrfs/file:
 EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
   0: [0..7]:          26624..26631         8   0x0
   1: [8..39]:         26632..26663        32 0x801
/mnt/btrfs/file:
 EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
   0: [0..7]:          26664..26671         8   0x0
   1: [8..39]:         26632..26663        32 0x801

The preallocated range is not defragged, but the sector before it still
gets defragged, which has no need for it.

[CAUSE]
One of the function reused by the old and new behavior is
defrag_check_next_extent(), it will determine if we should defrag
current extent by checking the next one.

It only checks if the next extent is a hole or inlined, but it doesn't
check if it's preallocated.

On the other hand, out of the function, both old and new kernel will
reject preallocated extents.

Such inconsistent behavior causes above behavior.

[FIX]
- Also check if next extent is preallocated
  If so, don't defrag current extent.

- Add comments for each branch why we reject the extent

This will reduce the IO caused by defrag ioctl and autodefrag.

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

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 91ba2efe9792..a622b1ac0fec 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -1053,19 +1053,24 @@ static bool defrag_check_next_extent(struct inode *inode, struct extent_map *em,
 				     bool locked)
 {
 	struct extent_map *next;
-	bool ret = true;
+	bool ret = false;
 
 	/* this is the last extent */
 	if (em->start + em->len >= i_size_read(inode))
-		return false;
+		return ret;
 
 	next = defrag_lookup_extent(inode, em->start + em->len, locked);
+	/* No more em or hole */
 	if (!next || next->block_start >= EXTENT_MAP_LAST_BYTE)
-		ret = false;
-	else if ((em->block_start + em->block_len == next->block_start) &&
-		 (em->block_len > SZ_128K && next->block_len > SZ_128K))
-		ret = false;
-
+		goto out;
+	if (test_bit(EXTENT_FLAG_PREALLOC, &next->flags))
+		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);
 	return ret;
 }
-- 
2.34.1

[PATCH v4 2/3] 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>
Reviewed-by: Filipe Manana <fdmanana@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 a622b1ac0fec..74237f31d166 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(const 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, &next->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 v4 3/3] 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>
Reviewed-by: Filipe Manana <fdmanana@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 74237f31d166..3d3331dd0902 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 v4 1/3] btrfs: defrag: don't try to merge regular extents with preallocated extents

[Filipe Manana]

On Fri, Jan 28, 2022 at 03:21:20PM +0800, Qu Wenruo wrote:
> [BUG]
> With older kernels (before v5.16), btrfs will defrag preallocated extents.
> While with newer kernels (v5.16 and newer) btrfs will not defrag
> preallocated extents, but it will defrag the extent just before the
> preallocated extent, even it's just a single sector.
> 
> This can be exposed by the following small script:
> 
> 	mkfs.btrfs -f $dev > /dev/null
> 
> 	mount $dev $mnt
> 	xfs_io -f -c "pwrite 0 4k" -c sync -c "falloc 4k 16K" $mnt/file
> 	xfs_io -c "fiemap -v" $mnt/file
> 	btrfs fi defrag $mnt/file
> 	sync
> 	xfs_io -c "fiemap -v" $mnt/file
> 
> The output looks like this on older kernels:
> 
> /mnt/btrfs/file:
>  EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
>    0: [0..7]:          26624..26631         8   0x0
>    1: [8..39]:         26632..26663        32 0x801
> /mnt/btrfs/file:
>  EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
>    0: [0..39]:         26664..26703        40   0x1
> 
> Which defrags the single sector along with the preallocated extent, and
> replace them with an regular extent into a new location (caused by data
> COW).
> This wastes most of the data IO just for the preallocated range.
> 
> On the other hand, v5.16 is slightly better:
> 
> /mnt/btrfs/file:
>  EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
>    0: [0..7]:          26624..26631         8   0x0
>    1: [8..39]:         26632..26663        32 0x801
> /mnt/btrfs/file:
>  EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
>    0: [0..7]:          26664..26671         8   0x0
>    1: [8..39]:         26632..26663        32 0x801
> 
> The preallocated range is not defragged, but the sector before it still
> gets defragged, which has no need for it.
> 
> [CAUSE]
> One of the function reused by the old and new behavior is
> defrag_check_next_extent(), it will determine if we should defrag
> current extent by checking the next one.
> 
> It only checks if the next extent is a hole or inlined, but it doesn't
> check if it's preallocated.
> 
> On the other hand, out of the function, both old and new kernel will
> reject preallocated extents.
> 
> Such inconsistent behavior causes above behavior.
> 
> [FIX]
> - Also check if next extent is preallocated
>   If so, don't defrag current extent.
> 
> - Add comments for each branch why we reject the extent
> 
> This will reduce the IO caused by defrag ioctl and autodefrag.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

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

Thanks.

> ---
>  fs/btrfs/ioctl.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 91ba2efe9792..a622b1ac0fec 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -1053,19 +1053,24 @@ static bool defrag_check_next_extent(struct inode *inode, struct extent_map *em,
>  				     bool locked)
>  {
>  	struct extent_map *next;
> -	bool ret = true;
> +	bool ret = false;
>  
>  	/* this is the last extent */
>  	if (em->start + em->len >= i_size_read(inode))
> -		return false;
> +		return ret;
>  
>  	next = defrag_lookup_extent(inode, em->start + em->len, locked);
> +	/* No more em or hole */
>  	if (!next || next->block_start >= EXTENT_MAP_LAST_BYTE)
> -		ret = false;
> -	else if ((em->block_start + em->block_len == next->block_start) &&
> -		 (em->block_len > SZ_128K && next->block_len > SZ_128K))
> -		ret = false;
> -
> +		goto out;
> +	if (test_bit(EXTENT_FLAG_PREALLOC, &next->flags))
> +		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);
>  	return ret;
>  }
> -- 
> 2.34.1
> 

Re: [PATCH v4 0/3] btrfs: fixes for defrag_check_next_extent()

[Filipe Manana]

On Fri, Jan 28, 2022 at 03:21:19PM +0800, Qu Wenruo wrote:
> That function is reused between older kernels (v5.15) and the refactored
> defrag code (v5.16+).
> 
> However that function has one long existing bugs affecting defrag to
> handle preallocated range.
> 
> And it can not handle compressed extent well neither.
> 
> Finally there is an ambiguous check which doesn't make much sense by
> itself, and can be related by enhanced extent capacity check.
> 
> This series will fix all the 3 problem mentioned above.
> 
> Changelog:
> v2:
> - Use @extent_thresh from caller to replace the harded coded threshold
>   Now caller has full control over the extent threshold value.
> 
> - Remove the old ambiguous check based on physical address
>   The original check is too specific, only reject extents which are
>   physically adjacent, AND too large.
>   Since we have correct size check now, and the physically adjacent check
>   is not always a win.
>   So remove the old check completely.
> 
> v3:
> - Split the @extent_thresh and physicall adjacent check into other
>   patches
> 
> - Simplify the comment 
> 
> v4:
> - Fix the @em usage which should be @next.
>   As it will fail the submitted test case.
> 
> Qu Wenruo (3):
>   btrfs: defrag: don't try to merge regular extents with preallocated
>     extents
>   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 | 35 ++++++++++++++++++++++++++++-------
>  1 file changed, 28 insertions(+), 7 deletions(-)

There's something screwed up in the series:

$ b4 am cover.1643354254.git.wqu@suse.com
Looking up https://lore.kernel.org/r/cover.1643354254.git.wqu%40suse.com
Grabbing thread from lore.kernel.org/all/cover.1643354254.git.wqu%40suse.com/t.mbox.gz
Analyzing 5 messages in the thread
Checking attestation on all messages, may take a moment...
---
  [PATCH v4 1/3] btrfs: defrag: don't try to merge regular extents with preallocated extents
    + Reviewed-by: Filipe Manana <fdmanana@suse.com>
  [PATCH v4 2/3] btrfs: defrag: don't defrag extents which is already at its max capacity
  [PATCH v4 3/3] btrfs: defrag: remove an ambiguous condition for rejection
  ---
  NOTE: install dkimpy for DKIM signature verification
---
Total patches: 3
---
Cover: ./v4_20220128_wqu_btrfs_fixes_for_defrag_check_next_extent.cover
 Link: https://lore.kernel.org/r/cover.1643354254.git.wqu@suse.com
 Base: not specified
       git am ./v4_20220128_wqu_btrfs_fixes_for_defrag_check_next_extent.mbx

$ git am ./v4_20220128_wqu_btrfs_fixes_for_defrag_check_next_extent.mbx
Applying: btrfs: defrag: don't try to merge regular extents with preallocated extents
Applying: btrfs: defrag: don't defrag extents which is already at its max capacity
error: patch failed: fs/btrfs/ioctl.c:1229
error: fs/btrfs/ioctl.c: patch does not apply
Patch failed at 0002 btrfs: defrag: don't defrag extents which is already at its max capacity
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Trying to manually pick patches 1 by 1 from patchwork, also results in the
same failure when applying patch 2/3.

Not sure why it failed, but I was able to manually apply the diffs.

Thanks.

> 
> -- 
> 2.34.1
> 

Re: [PATCH v4 0/3] btrfs: fixes for defrag_check_next_extent()

[Qu Wenruo]

On 2022/1/28 20:17, Filipe Manana wrote:
> On Fri, Jan 28, 2022 at 03:21:19PM +0800, Qu Wenruo wrote:
>> That function is reused between older kernels (v5.15) and the refactored
>> defrag code (v5.16+).
>>
>> However that function has one long existing bugs affecting defrag to
>> handle preallocated range.
>>
>> And it can not handle compressed extent well neither.
>>
>> Finally there is an ambiguous check which doesn't make much sense by
>> itself, and can be related by enhanced extent capacity check.
>>
>> This series will fix all the 3 problem mentioned above.
>>
>> Changelog:
>> v2:
>> - Use @extent_thresh from caller to replace the harded coded threshold
>>    Now caller has full control over the extent threshold value.
>>
>> - Remove the old ambiguous check based on physical address
>>    The original check is too specific, only reject extents which are
>>    physically adjacent, AND too large.
>>    Since we have correct size check now, and the physically adjacent check
>>    is not always a win.
>>    So remove the old check completely.
>>
>> v3:
>> - Split the @extent_thresh and physicall adjacent check into other
>>    patches
>>
>> - Simplify the comment
>>
>> v4:
>> - Fix the @em usage which should be @next.
>>    As it will fail the submitted test case.
>>
>> Qu Wenruo (3):
>>    btrfs: defrag: don't try to merge regular extents with preallocated
>>      extents
>>    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 | 35 ++++++++++++++++++++++++++++-------
>>   1 file changed, 28 insertions(+), 7 deletions(-)
>
> There's something screwed up in the series:
>
> $ b4 am cover.1643354254.git.wqu@suse.com
> Looking up https://lore.kernel.org/r/cover.1643354254.git.wqu%40suse.com
> Grabbing thread from lore.kernel.org/all/cover.1643354254.git.wqu%40suse.com/t.mbox.gz
> Analyzing 5 messages in the thread
> Checking attestation on all messages, may take a moment...
> ---
>    [PATCH v4 1/3] btrfs: defrag: don't try to merge regular extents with preallocated extents
>      + Reviewed-by: Filipe Manana <fdmanana@suse.com>
>    [PATCH v4 2/3] btrfs: defrag: don't defrag extents which is already at its max capacity
>    [PATCH v4 3/3] btrfs: defrag: remove an ambiguous condition for rejection
>    ---
>    NOTE: install dkimpy for DKIM signature verification
> ---
> Total patches: 3
> ---
> Cover: ./v4_20220128_wqu_btrfs_fixes_for_defrag_check_next_extent.cover
>   Link: https://lore.kernel.org/r/cover.1643354254.git.wqu@suse.com
>   Base: not specified
>         git am ./v4_20220128_wqu_btrfs_fixes_for_defrag_check_next_extent.mbx
>
> $ git am ./v4_20220128_wqu_btrfs_fixes_for_defrag_check_next_extent.mbx
> Applying: btrfs: defrag: don't try to merge regular extents with preallocated extents
> Applying: btrfs: defrag: don't defrag extents which is already at its max capacity
> error: patch failed: fs/btrfs/ioctl.c:1229
> error: fs/btrfs/ioctl.c: patch does not apply
> Patch failed at 0002 btrfs: defrag: don't defrag extents which is already at its max capacity
> hint: Use 'git am --show-current-patch=diff' to see the failed patch
> When you have resolved this problem, run "git am --continue".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".
>
> Trying to manually pick patches 1 by 1 from patchwork, also results in the
> same failure when applying patch 2/3.

My bad, I'm still using the old branch where I did all my test, it lacks
patches which are already in misc-next.

The missing patch of my base is "btrfs: fix deadlock when reserving
space during defrag", that makes the new lines in
defrag_collect_targets() to have some differences.

As in my base, it's

	if (em->len >= extent_thresh)

But now in misc-next, it's

	if (range_len >= extent_thresh)


This also makes me wonder, should we compare range_len to extent_thresh
or em->len?

One workaround users in v5.15 may use is to pass "-t 128k" for btrfs fi
defrag, so extents at 128K will not be defragged.

Won't the modified range_len check cause us to defrag extents which is
already 128K but the cluster boundary just ends inside the compressed
extent, and at the next cluster, we will choose to defrag part of the
extent.

Thanks,
Qu



>
> Not sure why it failed, but I was able to manually apply the diffs.
>
> Thanks.
>
>>
>> --
>> 2.34.1
>>

Re: [PATCH v4 0/3] btrfs: fixes for defrag_check_next_extent()

[Filipe Manana]

On Fri, Jan 28, 2022 at 12:38 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>
>
>
> On 2022/1/28 20:17, Filipe Manana wrote:
> > On Fri, Jan 28, 2022 at 03:21:19PM +0800, Qu Wenruo wrote:
> >> That function is reused between older kernels (v5.15) and the refactored
> >> defrag code (v5.16+).
> >>
> >> However that function has one long existing bugs affecting defrag to
> >> handle preallocated range.
> >>
> >> And it can not handle compressed extent well neither.
> >>
> >> Finally there is an ambiguous check which doesn't make much sense by
> >> itself, and can be related by enhanced extent capacity check.
> >>
> >> This series will fix all the 3 problem mentioned above.
> >>
> >> Changelog:
> >> v2:
> >> - Use @extent_thresh from caller to replace the harded coded threshold
> >>    Now caller has full control over the extent threshold value.
> >>
> >> - Remove the old ambiguous check based on physical address
> >>    The original check is too specific, only reject extents which are
> >>    physically adjacent, AND too large.
> >>    Since we have correct size check now, and the physically adjacent check
> >>    is not always a win.
> >>    So remove the old check completely.
> >>
> >> v3:
> >> - Split the @extent_thresh and physicall adjacent check into other
> >>    patches
> >>
> >> - Simplify the comment
> >>
> >> v4:
> >> - Fix the @em usage which should be @next.
> >>    As it will fail the submitted test case.
> >>
> >> Qu Wenruo (3):
> >>    btrfs: defrag: don't try to merge regular extents with preallocated
> >>      extents
> >>    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 | 35 ++++++++++++++++++++++++++++-------
> >>   1 file changed, 28 insertions(+), 7 deletions(-)
> >
> > There's something screwed up in the series:
> >
> > $ b4 am cover.1643354254.git.wqu@suse.com
> > Looking up https://lore.kernel.org/r/cover.1643354254.git.wqu%40suse.com
> > Grabbing thread from lore.kernel.org/all/cover.1643354254.git.wqu%40suse.com/t.mbox.gz
> > Analyzing 5 messages in the thread
> > Checking attestation on all messages, may take a moment...
> > ---
> >    [PATCH v4 1/3] btrfs: defrag: don't try to merge regular extents with preallocated extents
> >      + Reviewed-by: Filipe Manana <fdmanana@suse.com>
> >    [PATCH v4 2/3] btrfs: defrag: don't defrag extents which is already at its max capacity
> >    [PATCH v4 3/3] btrfs: defrag: remove an ambiguous condition for rejection
> >    ---
> >    NOTE: install dkimpy for DKIM signature verification
> > ---
> > Total patches: 3
> > ---
> > Cover: ./v4_20220128_wqu_btrfs_fixes_for_defrag_check_next_extent.cover
> >   Link: https://lore.kernel.org/r/cover.1643354254.git.wqu@suse.com
> >   Base: not specified
> >         git am ./v4_20220128_wqu_btrfs_fixes_for_defrag_check_next_extent.mbx
> >
> > $ git am ./v4_20220128_wqu_btrfs_fixes_for_defrag_check_next_extent.mbx
> > Applying: btrfs: defrag: don't try to merge regular extents with preallocated extents
> > Applying: btrfs: defrag: don't defrag extents which is already at its max capacity
> > error: patch failed: fs/btrfs/ioctl.c:1229
> > error: fs/btrfs/ioctl.c: patch does not apply
> > Patch failed at 0002 btrfs: defrag: don't defrag extents which is already at its max capacity
> > hint: Use 'git am --show-current-patch=diff' to see the failed patch
> > When you have resolved this problem, run "git am --continue".
> > If you prefer to skip this patch, run "git am --skip" instead.
> > To restore the original branch and stop patching, run "git am --abort".
> >
> > Trying to manually pick patches 1 by 1 from patchwork, also results in the
> > same failure when applying patch 2/3.
>
> My bad, I'm still using the old branch where I did all my test, it lacks
> patches which are already in misc-next.
>
> The missing patch of my base is "btrfs: fix deadlock when reserving
> space during defrag", that makes the new lines in
> defrag_collect_targets() to have some differences.
>
> As in my base, it's
>
>         if (em->len >= extent_thresh)
>
> But now in misc-next, it's
>
>         if (range_len >= extent_thresh)
>
>
> This also makes me wonder, should we compare range_len to extent_thresh
> or em->len?

In this case I think em->len is fine.
Using range_len, which can be shorter for the first extent map in the
range, could trigger defrag of an extent at the maximum possible size.

Thanks.

>
> One workaround users in v5.15 may use is to pass "-t 128k" for btrfs fi
> defrag, so extents at 128K will not be defragged.
>
> Won't the modified range_len check cause us to defrag extents which is
> already 128K but the cluster boundary just ends inside the compressed
> extent, and at the next cluster, we will choose to defrag part of the
> extent.
>
> Thanks,
> Qu
>
>
>
> >
> > Not sure why it failed, but I was able to manually apply the diffs.
> >
> > Thanks.
> >
> >>
> >> --
> >> 2.34.1
> >>

Re: [PATCH v4 0/3] btrfs: fixes for defrag_check_next_extent()

[Qu Wenruo]

On 2022/1/28 20:43, Filipe Manana wrote:
> On Fri, Jan 28, 2022 at 12:38 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>>
>>
>>
>> On 2022/1/28 20:17, Filipe Manana wrote:
>>> On Fri, Jan 28, 2022 at 03:21:19PM +0800, Qu Wenruo wrote:
>>>> That function is reused between older kernels (v5.15) and the refactored
>>>> defrag code (v5.16+).
>>>>
>>>> However that function has one long existing bugs affecting defrag to
>>>> handle preallocated range.
>>>>
>>>> And it can not handle compressed extent well neither.
>>>>
>>>> Finally there is an ambiguous check which doesn't make much sense by
>>>> itself, and can be related by enhanced extent capacity check.
>>>>
>>>> This series will fix all the 3 problem mentioned above.
>>>>
>>>> Changelog:
>>>> v2:
>>>> - Use @extent_thresh from caller to replace the harded coded threshold
>>>>     Now caller has full control over the extent threshold value.
>>>>
>>>> - Remove the old ambiguous check based on physical address
>>>>     The original check is too specific, only reject extents which are
>>>>     physically adjacent, AND too large.
>>>>     Since we have correct size check now, and the physically adjacent check
>>>>     is not always a win.
>>>>     So remove the old check completely.
>>>>
>>>> v3:
>>>> - Split the @extent_thresh and physicall adjacent check into other
>>>>     patches
>>>>
>>>> - Simplify the comment
>>>>
>>>> v4:
>>>> - Fix the @em usage which should be @next.
>>>>     As it will fail the submitted test case.
>>>>
>>>> Qu Wenruo (3):
>>>>     btrfs: defrag: don't try to merge regular extents with preallocated
>>>>       extents
>>>>     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 | 35 ++++++++++++++++++++++++++++-------
>>>>    1 file changed, 28 insertions(+), 7 deletions(-)
>>>
>>> There's something screwed up in the series:
>>>
>>> $ b4 am cover.1643354254.git.wqu@suse.com
>>> Looking up https://lore.kernel.org/r/cover.1643354254.git.wqu%40suse.com
>>> Grabbing thread from lore.kernel.org/all/cover.1643354254.git.wqu%40suse.com/t.mbox.gz
>>> Analyzing 5 messages in the thread
>>> Checking attestation on all messages, may take a moment...
>>> ---
>>>     [PATCH v4 1/3] btrfs: defrag: don't try to merge regular extents with preallocated extents
>>>       + Reviewed-by: Filipe Manana <fdmanana@suse.com>
>>>     [PATCH v4 2/3] btrfs: defrag: don't defrag extents which is already at its max capacity
>>>     [PATCH v4 3/3] btrfs: defrag: remove an ambiguous condition for rejection
>>>     ---
>>>     NOTE: install dkimpy for DKIM signature verification
>>> ---
>>> Total patches: 3
>>> ---
>>> Cover: ./v4_20220128_wqu_btrfs_fixes_for_defrag_check_next_extent.cover
>>>    Link: https://lore.kernel.org/r/cover.1643354254.git.wqu@suse.com
>>>    Base: not specified
>>>          git am ./v4_20220128_wqu_btrfs_fixes_for_defrag_check_next_extent.mbx
>>>
>>> $ git am ./v4_20220128_wqu_btrfs_fixes_for_defrag_check_next_extent.mbx
>>> Applying: btrfs: defrag: don't try to merge regular extents with preallocated extents
>>> Applying: btrfs: defrag: don't defrag extents which is already at its max capacity
>>> error: patch failed: fs/btrfs/ioctl.c:1229
>>> error: fs/btrfs/ioctl.c: patch does not apply
>>> Patch failed at 0002 btrfs: defrag: don't defrag extents which is already at its max capacity
>>> hint: Use 'git am --show-current-patch=diff' to see the failed patch
>>> When you have resolved this problem, run "git am --continue".
>>> If you prefer to skip this patch, run "git am --skip" instead.
>>> To restore the original branch and stop patching, run "git am --abort".
>>>
>>> Trying to manually pick patches 1 by 1 from patchwork, also results in the
>>> same failure when applying patch 2/3.
>>
>> My bad, I'm still using the old branch where I did all my test, it lacks
>> patches which are already in misc-next.
>>
>> The missing patch of my base is "btrfs: fix deadlock when reserving
>> space during defrag", that makes the new lines in
>> defrag_collect_targets() to have some differences.
>>
>> As in my base, it's
>>
>>          if (em->len >= extent_thresh)
>>
>> But now in misc-next, it's
>>
>>          if (range_len >= extent_thresh)
>>
>>
>> This also makes me wonder, should we compare range_len to extent_thresh
>> or em->len?
>
> In this case I think em->len is fine.
> Using range_len, which can be shorter for the first extent map in the
> range, could trigger defrag of an extent at the maximum possible size.

Then the patch can be applied without any problem too.

Really a win-win case.

>
> Thanks.
>
>>
>> One workaround users in v5.15 may use is to pass "-t 128k" for btrfs fi
>> defrag, so extents at 128K will not be defragged.
>>
>> Won't the modified range_len check cause us to defrag extents which is
>> already 128K but the cluster boundary just ends inside the compressed
>> extent, and at the next cluster, we will choose to defrag part of the
>> extent.
>>
>> Thanks,
>> Qu
>>
>>
>>
>>>
>>> Not sure why it failed, but I was able to manually apply the diffs.
>>>
>>> Thanks.
>>>
>>>>
>>>> --
>>>> 2.34.1
>>>>

Re: [PATCH v4 0/3] btrfs: fixes for defrag_check_next_extent()

[David Sterba]

On Fri, Jan 28, 2022 at 03:21:19PM +0800, Qu Wenruo wrote:
> That function is reused between older kernels (v5.15) and the refactored
> defrag code (v5.16+).
> 
> However that function has one long existing bugs affecting defrag to
> handle preallocated range.
> 
> And it can not handle compressed extent well neither.
> 
> Finally there is an ambiguous check which doesn't make much sense by
> itself, and can be related by enhanced extent capacity check.
> 
> This series will fix all the 3 problem mentioned above.
> 
> Changelog:
> v2:
> - Use @extent_thresh from caller to replace the harded coded threshold
>   Now caller has full control over the extent threshold value.
> 
> - Remove the old ambiguous check based on physical address
>   The original check is too specific, only reject extents which are
>   physically adjacent, AND too large.
>   Since we have correct size check now, and the physically adjacent check
>   is not always a win.
>   So remove the old check completely.
> 
> v3:
> - Split the @extent_thresh and physicall adjacent check into other
>   patches
> 
> - Simplify the comment 
> 
> v4:
> - Fix the @em usage which should be @next.
>   As it will fail the submitted test case.
> 
> Qu Wenruo (3):
>   btrfs: defrag: don't try to merge regular extents with preallocated
>     extents
>   btrfs: defrag: don't defrag extents which is already at its max
>     capacity
>   btrfs: defrag: remove an ambiguous condition for rejection

Added as topic branch to for-next, thanks.

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

[David Sterba]

On Fri, Jan 28, 2022 at 03:21:21PM +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.

Is this wrong for all cases though? With your change compressed extents
would never get defragmented, while the defragmentation ioctl allows to
change the compression algorithm, so that's a loss of functionality.

And I think we can't do that even conditionally if the algorithm is
different, because what if we want to recompress a file with higher
level of the same algorithm? As the level not stored anywhere the defrag
ioctl can't decide which extents to skip to save work.

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

[Qu Wenruo]

On 2022/2/1 23:03, David Sterba wrote:
> On Fri, Jan 28, 2022 at 03:21:21PM +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.
>
> Is this wrong for all cases though? With your change compressed extents
> would never get defragmented, while the defragmentation ioctl allows to
> change the compression algorithm, so that's a loss of functionality.

Nope, firstly only 128K compressed extents will get skipped.
Smaller compressed extents will still be defragged.

Secondly, in defrag_collect_targets(), the check against max capacity is
after the check for compression conversion.
Meaning for the compression algo change, it will not take max capacity
into consideration anyway.

>
> And I think we can't do that even conditionally if the algorithm is
> different, because what if we want to recompress a file with higher
> level of the same algorithm? As the level not stored anywhere the defrag
> ioctl can't decide which extents to skip to save work.

The check is not based on compression algorithm, it's a flag in the
defrag ioctl parameter.
If that DEFRAG_RANGE_COMPRESS parameter is set, we won't bother a lot of
things (including max capacity, next mergeable, etc) but directly mark
the range for defrag.

Thanks,
Qu

Re: [PATCH v4 0/3] btrfs: fixes for defrag_check_next_extent()

[David Sterba]

On Fri, Jan 28, 2022 at 10:54:38PM +0100, David Sterba wrote:
> On Fri, Jan 28, 2022 at 03:21:19PM +0800, Qu Wenruo wrote:
> > That function is reused between older kernels (v5.15) and the refactored
> > defrag code (v5.16+).
> > 
> > However that function has one long existing bugs affecting defrag to
> > handle preallocated range.
> > 
> > And it can not handle compressed extent well neither.
> > 
> > Finally there is an ambiguous check which doesn't make much sense by
> > itself, and can be related by enhanced extent capacity check.
> > 
> > This series will fix all the 3 problem mentioned above.
> > 
> > Changelog:
> > v2:
> > - Use @extent_thresh from caller to replace the harded coded threshold
> >   Now caller has full control over the extent threshold value.
> > 
> > - Remove the old ambiguous check based on physical address
> >   The original check is too specific, only reject extents which are
> >   physically adjacent, AND too large.
> >   Since we have correct size check now, and the physically adjacent check
> >   is not always a win.
> >   So remove the old check completely.
> > 
> > v3:
> > - Split the @extent_thresh and physicall adjacent check into other
> >   patches
> > 
> > - Simplify the comment 
> > 
> > v4:
> > - Fix the @em usage which should be @next.
> >   As it will fail the submitted test case.
> > 
> > Qu Wenruo (3):
> >   btrfs: defrag: don't try to merge regular extents with preallocated
> >     extents
> >   btrfs: defrag: don't defrag extents which is already at its max
> >     capacity
> >   btrfs: defrag: remove an ambiguous condition for rejection
> 
> Added as topic branch to for-next, thanks.

Moved to misc-next.