[PATCH v3 1/3] btrfs: test defrag with regular and preallocated extents

[PATCH v3 1/3] btrfs: test defrag with regular and preallocated extents

[Qu Wenruo]

Recent v5.16 has some regression around btrfs autodefrag mount option,
and the extra scrutiny around defrag code exposes some questionable
behavior from the old code.

One behavior is to defrag extents along with the next preallocated
extent.

This behavior will cause extra IO and convert all the preallocated
extent to regular zero filled extents, rendering the preallocated extent
useless.

The kernel fix is titled:

  btrfs: defrag: don't try to merge regular extents with preallocated extents

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
Changelog:
v2:
- Add _require_btrfs_support_sectorsize() helper
  And use it to make sure the platform supports 4k sectorsize

- Use $AWK_PROG to replace awk

v3:
- Move _get_file_extent_sector() into common/rc
---
 common/btrfs        | 16 ++++++++++
 common/rc           | 18 +++++++++++
 tests/btrfs/255     | 75 +++++++++++++++++++++++++++++++++++++++++++++
 tests/btrfs/255.out |  2 ++
 4 files changed, 111 insertions(+)
 create mode 100755 tests/btrfs/255
 create mode 100644 tests/btrfs/255.out

diff --git a/common/btrfs b/common/btrfs
index 4afe81eb..5de926dd 100644
--- a/common/btrfs
+++ b/common/btrfs
@@ -480,3 +480,19 @@ _btrfs_no_v1_cache_opt()
 	fi
 	echo -n "-onospace_cache"
 }
+
+# Require certain sectorsize support
+_require_btrfs_support_sectorsize()
+{
+	local sectorsize=$1
+
+	# PAGE_SIZE as sectorsize is always supported
+	if [ $sectorsize -eq $(get_page_size) ]; then
+		return
+	fi
+
+	test -f /sys/fs/btrfs/features/supported_sectorsizes || \
+		_notrun "no subpage support found"
+	grep -wq $sectorsize /sys/fs/btrfs/features/supported_sectorsizes || \
+		_notrun "sectorsize $sectorsize is not supported"
+}
diff --git a/common/rc b/common/rc
index b3289de9..8fbb32f8 100644
--- a/common/rc
+++ b/common/rc
@@ -3767,6 +3767,24 @@ _count_attr_extents()
 	$XFS_IO_PROG -c "fiemap -a" $1 | tail -n +2 | grep -v hole | wc -l
 }
 
+# Get the sector number of the extent at @offset of @file
+_get_file_extent_sector()
+{
+	local file=$1
+	local offset=$2
+	local result
+
+	result=$($XFS_IO_PROG -c "fiemap $offset" "$file" | \
+		 _filter_xfs_io_fiemap | head -n1 | $AWK_PROG '{print $3}')
+
+	# xfs_io fiemap will output nothing if there is only hole, so here
+	# to replace the empty string with "hole" instead
+	if [ -z "$result" ]; then
+		result="hole"
+	fi
+	echo "$result"
+}
+
 # arg 1 is dev to remove and is output of the below eg.
 # ls -l /sys/class/block/sdd | rev | cut -d "/" -f 3 | rev
 _devmgt_remove()
diff --git a/tests/btrfs/255 b/tests/btrfs/255
new file mode 100755
index 00000000..fb80359c
--- /dev/null
+++ b/tests/btrfs/255
@@ -0,0 +1,75 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2022 SUSE Linux Products GmbH. All Rights Reserved.
+#
+# FS QA Test 255
+#
+# Make sure btrfs doesn't defrag preallocated extents, nor lone extents
+# before preallocated extents.
+#
+
+. ./common/preamble
+_begin_fstest auto quick defrag
+
+# Override the default cleanup function.
+# _cleanup()
+# {
+# 	cd /
+# 	rm -r -f $tmp.*
+# }
+
+# Import common functions.
+. ./common/filter
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs btrfs
+_require_scratch
+
+# Needs 4K sectorsize
+_require_btrfs_support_sectorsize 4096
+
+_scratch_mkfs -s 4k >> $seqres.full 2>&1
+
+# Need datacow to make the defragged extents to have different bytenr
+_scratch_mount -o datacow
+
+# Create a file with the following layout:
+# 0       4K        8K        16K
+# |<- R ->|<-- Preallocated ->|
+# R is regular extents.
+#
+# In this case it makes no sense to defrag any extent.
+$XFS_IO_PROG -f -c "pwrite 0 4k" -c sync -c "falloc 4k 12k" \
+	"$SCRATCH_MNT/foobar" >> $seqres.full
+
+echo "=== Initial file extent layout ===" >> $seqres.full
+$XFS_IO_PROG -c "fiemap -v" "$SCRATCH_MNT/foobar" >> $seqres.full
+
+# Save the bytenr of both extents
+old_regular=$(_get_file_extent_sector "$SCRATCH_MNT/foobar" 0)
+old_prealloc=$(_get_file_extent_sector "$SCRATCH_MNT/foobar" 4096)
+
+# Now defrag and write the defragged range back to disk
+$BTRFS_UTIL_PROG filesystem defrag "$SCRATCH_MNT/foobar" >> $seqres.full
+sync
+
+echo "=== File extent layout after defrag ===" >> $seqres.full
+$XFS_IO_PROG -c "fiemap -v" "$SCRATCH_MNT/foobar" >> $seqres.full
+
+new_regular=$(_get_file_extent_sector "$SCRATCH_MNT/foobar" 0)
+new_prealloc=$(_get_file_extent_sector "$SCRATCH_MNT/foobar" 4096)
+
+if [ "$old_regular" -ne "$new_regular" ]; then
+	echo "the single lone sector get defragged"
+fi
+if [ "$old_prealloc" -ne "$new_prealloc" ]; then
+	echo "the preallocated extent get defragged"
+fi
+
+echo "Silence is golden"
+
+# success, all done
+status=0
+exit
diff --git a/tests/btrfs/255.out b/tests/btrfs/255.out
new file mode 100644
index 00000000..7eefb828
--- /dev/null
+++ b/tests/btrfs/255.out
@@ -0,0 +1,2 @@
+QA output created by 255
+Silence is golden
-- 
2.34.1

[PATCH v2 2/3] btrfs: test autodefrag with regular and hole extents

[Qu Wenruo]

In v5.11~v5.15 kernels, there is a regression in autodefrag that if a
cluster (up to 256K in size) has even a single hole, the whole cluster
will be rejected.

This will greatly reduce the efficiency of autodefrag.

The behavior is fixed in v5.16 by a full rework, although the rework
itself has other problems, it at least solves the problem.

Here we add a test case to reproduce the case, where we have a 128K
cluster, the first half is fragmented extents which can be defragged.
The second half is hole.

Make sure autodefrag can defrag the 64K part.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
Changelog:
v2:
- Use the previously define _get_file_extent_sector() helper
  This also removed some out-of-sync error messages

- Trigger autodefrag using commit=1 mount option
  No need for special purpose patch any more.

- Use xfs_io -s to skip several sync calls

- Shorten the subject of the commit
---
 tests/btrfs/256     | 80 +++++++++++++++++++++++++++++++++++++++++++++
 tests/btrfs/256.out |  2 ++
 2 files changed, 82 insertions(+)
 create mode 100755 tests/btrfs/256
 create mode 100644 tests/btrfs/256.out

diff --git a/tests/btrfs/256 b/tests/btrfs/256
new file mode 100755
index 00000000..def83a15
--- /dev/null
+++ b/tests/btrfs/256
@@ -0,0 +1,80 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2022 SUSE Linux Products GmbH. All Rights Reserved.
+#
+# FS QA Test 256
+#
+# Make sure btrfs auto defrag can properly defrag clusters which has hole
+# in the middle
+#
+. ./common/preamble
+_begin_fstest auto defrag quick
+
+. ./common/btrfs
+. ./common/filter
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs generic
+_require_scratch
+
+# Needs 4K sectorsize, as larger sectorsize can change the file layout.
+_require_btrfs_support_sectorsize 4096
+
+_scratch_mkfs >> $seqres.full
+
+# Need datacow to show which range is defragged, and we're testing
+# autodefrag
+_scratch_mount -o datacow,autodefrag
+
+# Create a layout where we have fragmented extents at [0, 64k) (sync write in
+# reserve order), then a hole at [64k, 128k)
+$XFS_IO_PROG -f -s -c "pwrite 48k 16k" -c "pwrite 32k 16k" \
+		-c "pwrite 16k 16k" -c "pwrite 0 16k" \
+		$SCRATCH_MNT/foobar >> $seqres.full
+truncate -s 128k $SCRATCH_MNT/foobar
+
+old_csum=$(_md5_checksum $SCRATCH_MNT/foobar)
+echo "=== File extent layout before autodefrag ===" >> $seqres.full
+$XFS_IO_PROG -c "fiemap -v" "$SCRATCH_MNT/foobar" >> $seqres.full
+echo "old md5=$old_csum" >> $seqres.full
+
+old_regular=$(_get_file_extent_sector "$SCRATCH_MNT/foobar" 0)
+old_hole=$(_get_file_extent_sector "$SCRATCH_MNT/foobar" 64k)
+
+# Now trigger autodefrag, autodefrag is triggered in the cleaner thread,
+# which will be woken up by commit thread
+_scratch_remount commit=1
+sleep 3
+sync
+
+new_csum=$(_md5_checksum $SCRATCH_MNT/foobar)
+new_regular=$(_get_file_extent_sector "$SCRATCH_MNT/foobar" 0)
+new_hole=$(_get_file_extent_sector "$SCRATCH_MNT/foobar" 64k)
+
+echo "=== File extent layout after autodefrag ===" >> $seqres.full
+$XFS_IO_PROG -c "fiemap -v" "$SCRATCH_MNT/foobar" >> $seqres.full
+echo "new md5=$new_csum" >> $seqres.full
+
+# In v5.11~v5.15 kernels, regular extents won't get defragged, and would trigger
+# the following output
+if [ $new_regular == $old_regular ]; then
+	echo "regular extents didn't get defragged"
+fi
+
+# In v5.10 and earlier kernel, autodefrag may choose to defrag holes,
+# which should be avoided.
+if [ "$new_hole" != "$old_hole" ]; then
+	echo "hole extents got defragged"
+fi
+
+# Defrag should not change file content
+if [ "$new_csum" != "$old_csum" ]; then
+	echo "file content changed"
+fi
+
+echo "Silence is golden"
+# success, all done
+status=0
+exit
diff --git a/tests/btrfs/256.out b/tests/btrfs/256.out
new file mode 100644
index 00000000..7ee8e2e5
--- /dev/null
+++ b/tests/btrfs/256.out
@@ -0,0 +1,2 @@
+QA output created by 256
+Silence is golden
-- 
2.34.1

Re: [PATCH v2 2/3] btrfs: test autodefrag with regular and hole extents

[Filipe Manana]

On Fri, Jan 28, 2022 at 08:27:00AM +0800, Qu Wenruo wrote:
> In v5.11~v5.15 kernels, there is a regression in autodefrag that if a
> cluster (up to 256K in size) has even a single hole, the whole cluster
> will be rejected.
> 
> This will greatly reduce the efficiency of autodefrag.
> 
> The behavior is fixed in v5.16 by a full rework, although the rework
> itself has other problems, it at least solves the problem.
> 
> Here we add a test case to reproduce the case, where we have a 128K
> cluster, the first half is fragmented extents which can be defragged.
> The second half is hole.
> 
> Make sure autodefrag can defrag the 64K part.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Tried the test, and it succeeds here without the kernel fix applied.
I've ran it 10 times, and it always passed without the kernel fix.

Thanks.

> ---
> Changelog:
> v2:
> - Use the previously define _get_file_extent_sector() helper
>   This also removed some out-of-sync error messages
> 
> - Trigger autodefrag using commit=1 mount option
>   No need for special purpose patch any more.
> 
> - Use xfs_io -s to skip several sync calls
> 
> - Shorten the subject of the commit
> ---
>  tests/btrfs/256     | 80 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/btrfs/256.out |  2 ++
>  2 files changed, 82 insertions(+)
>  create mode 100755 tests/btrfs/256
>  create mode 100644 tests/btrfs/256.out
> 
> diff --git a/tests/btrfs/256 b/tests/btrfs/256
> new file mode 100755
> index 00000000..def83a15
> --- /dev/null
> +++ b/tests/btrfs/256
> @@ -0,0 +1,80 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (C) 2022 SUSE Linux Products GmbH. All Rights Reserved.
> +#
> +# FS QA Test 256
> +#
> +# Make sure btrfs auto defrag can properly defrag clusters which has hole
> +# in the middle
> +#
> +. ./common/preamble
> +_begin_fstest auto defrag quick
> +
> +. ./common/btrfs
> +. ./common/filter
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs generic
> +_require_scratch
> +
> +# Needs 4K sectorsize, as larger sectorsize can change the file layout.
> +_require_btrfs_support_sectorsize 4096
> +
> +_scratch_mkfs >> $seqres.full
> +
> +# Need datacow to show which range is defragged, and we're testing
> +# autodefrag
> +_scratch_mount -o datacow,autodefrag
> +
> +# Create a layout where we have fragmented extents at [0, 64k) (sync write in
> +# reserve order), then a hole at [64k, 128k)
> +$XFS_IO_PROG -f -s -c "pwrite 48k 16k" -c "pwrite 32k 16k" \
> +		-c "pwrite 16k 16k" -c "pwrite 0 16k" \
> +		$SCRATCH_MNT/foobar >> $seqres.full
> +truncate -s 128k $SCRATCH_MNT/foobar
> +
> +old_csum=$(_md5_checksum $SCRATCH_MNT/foobar)
> +echo "=== File extent layout before autodefrag ===" >> $seqres.full
> +$XFS_IO_PROG -c "fiemap -v" "$SCRATCH_MNT/foobar" >> $seqres.full
> +echo "old md5=$old_csum" >> $seqres.full
> +
> +old_regular=$(_get_file_extent_sector "$SCRATCH_MNT/foobar" 0)
> +old_hole=$(_get_file_extent_sector "$SCRATCH_MNT/foobar" 64k)
> +
> +# Now trigger autodefrag, autodefrag is triggered in the cleaner thread,
> +# which will be woken up by commit thread
> +_scratch_remount commit=1
> +sleep 3
> +sync
> +
> +new_csum=$(_md5_checksum $SCRATCH_MNT/foobar)
> +new_regular=$(_get_file_extent_sector "$SCRATCH_MNT/foobar" 0)
> +new_hole=$(_get_file_extent_sector "$SCRATCH_MNT/foobar" 64k)
> +
> +echo "=== File extent layout after autodefrag ===" >> $seqres.full
> +$XFS_IO_PROG -c "fiemap -v" "$SCRATCH_MNT/foobar" >> $seqres.full
> +echo "new md5=$new_csum" >> $seqres.full
> +
> +# In v5.11~v5.15 kernels, regular extents won't get defragged, and would trigger
> +# the following output
> +if [ $new_regular == $old_regular ]; then
> +	echo "regular extents didn't get defragged"
> +fi
> +
> +# In v5.10 and earlier kernel, autodefrag may choose to defrag holes,
> +# which should be avoided.
> +if [ "$new_hole" != "$old_hole" ]; then
> +	echo "hole extents got defragged"
> +fi
> +
> +# Defrag should not change file content
> +if [ "$new_csum" != "$old_csum" ]; then
> +	echo "file content changed"
> +fi
> +
> +echo "Silence is golden"
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/btrfs/256.out b/tests/btrfs/256.out
> new file mode 100644
> index 00000000..7ee8e2e5
> --- /dev/null
> +++ b/tests/btrfs/256.out
> @@ -0,0 +1,2 @@
> +QA output created by 256
> +Silence is golden
> -- 
> 2.34.1
> 

Re: [PATCH v2 2/3] btrfs: test autodefrag with regular and hole extents

[Qu Wenruo]

On 2022/1/28 19:57, Filipe Manana wrote:
> On Fri, Jan 28, 2022 at 08:27:00AM +0800, Qu Wenruo wrote:
>> In v5.11~v5.15 kernels, there is a regression in autodefrag that if a
>> cluster (up to 256K in size) has even a single hole, the whole cluster
>> will be rejected.
>>
>> This will greatly reduce the efficiency of autodefrag.
>>
>> The behavior is fixed in v5.16 by a full rework, although the rework
>> itself has other problems, it at least solves the problem.
>>
>> Here we add a test case to reproduce the case, where we have a 128K
>> cluster, the first half is fragmented extents which can be defragged.
>> The second half is hole.
>>
>> Make sure autodefrag can defrag the 64K part.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>
> Tried the test, and it succeeds here without the kernel fix applied.
> I've ran it 10 times, and it always passed without the kernel fix.

Have you tried v5.15 kernels? As v5.16 has the reworked code and will
always pass.

Or you can try to revert those 3 commits:

c22a3572cbaf ("btrfs: defrag: enable defrag for subpage case")
c635757365c3 ("btrfs: defrag: remove the old infrastructure")
7b508037d4ca ("btrfs: defrag: use defrag_one_cluster() to implement
btrfs_defrag_file()")

That's how I tested the case with the same kernel base without going
back to v5.15.

And with those 3 commits reverted (aka, my baseline to emulate v5.15),
it fails like this:

btrfs/256 4s ... - output mismatch (see
/home/adam/xfstests-dev/results//btrfs/256.out.bad)
     --- tests/btrfs/256.out	2022-01-28 08:19:11.123333302 +0800
     +++ /home/adam/xfstests-dev/results//btrfs/256.out.bad	2022-01-28
20:10:18.416666746 +0800
     @@ -1,2 +1,3 @@
      QA output created by 256
     +regular extents didn't get defragged
      Silence is golden
     ...
     (Run 'diff -u /home/adam/xfstests-dev/tests/btrfs/256.out
/home/adam/xfstests-dev/results//btrfs/256.out.bad'  to see the entire diff)
Ran: btrfs/256

With the fiemap result in the full log:
=== File extent layout before autodefrag ===
/mnt/scratch/foobar:
  EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
    0: [0..31]:         26720..26751        32   0x0
    1: [32..63]:        26688..26719        32   0x0
    2: [64..95]:        26656..26687        32   0x0
    3: [96..127]:       26624..26655        32   0x1
    4: [128..255]:      hole               128
old md5=9ef8ace32f3b9890cff4fd43699bbd81
=== File extent layout after autodefrag ===
/mnt/scratch/foobar:
  EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
    0: [0..31]:         26720..26751        32   0x0
    1: [32..63]:        26688..26719        32   0x0
    2: [64..95]:        26656..26687        32   0x0
    3: [96..127]:       26624..26655        32   0x1
    4: [128..255]:      hole               128
new md5=9ef8ace32f3b9890cff4fd43699bbd81

With my POC patch for v5.15 or v5.16 kernel it passes as expected.

Thus that's why there is no fix mentioned in the commit message.

Thanks,
Qu



>
> Thanks.
>
>> ---
>> Changelog:
>> v2:
>> - Use the previously define _get_file_extent_sector() helper
>>    This also removed some out-of-sync error messages
>>
>> - Trigger autodefrag using commit=1 mount option
>>    No need for special purpose patch any more.
>>
>> - Use xfs_io -s to skip several sync calls
>>
>> - Shorten the subject of the commit
>> ---
>>   tests/btrfs/256     | 80 +++++++++++++++++++++++++++++++++++++++++++++
>>   tests/btrfs/256.out |  2 ++
>>   2 files changed, 82 insertions(+)
>>   create mode 100755 tests/btrfs/256
>>   create mode 100644 tests/btrfs/256.out
>>
>> diff --git a/tests/btrfs/256 b/tests/btrfs/256
>> new file mode 100755
>> index 00000000..def83a15
>> --- /dev/null
>> +++ b/tests/btrfs/256
>> @@ -0,0 +1,80 @@
>> +#! /bin/bash
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (C) 2022 SUSE Linux Products GmbH. All Rights Reserved.
>> +#
>> +# FS QA Test 256
>> +#
>> +# Make sure btrfs auto defrag can properly defrag clusters which has hole
>> +# in the middle
>> +#
>> +. ./common/preamble
>> +_begin_fstest auto defrag quick
>> +
>> +. ./common/btrfs
>> +. ./common/filter
>> +
>> +# real QA test starts here
>> +
>> +# Modify as appropriate.
>> +_supported_fs generic
>> +_require_scratch
>> +
>> +# Needs 4K sectorsize, as larger sectorsize can change the file layout.
>> +_require_btrfs_support_sectorsize 4096
>> +
>> +_scratch_mkfs >> $seqres.full
>> +
>> +# Need datacow to show which range is defragged, and we're testing
>> +# autodefrag
>> +_scratch_mount -o datacow,autodefrag
>> +
>> +# Create a layout where we have fragmented extents at [0, 64k) (sync write in
>> +# reserve order), then a hole at [64k, 128k)
>> +$XFS_IO_PROG -f -s -c "pwrite 48k 16k" -c "pwrite 32k 16k" \
>> +		-c "pwrite 16k 16k" -c "pwrite 0 16k" \
>> +		$SCRATCH_MNT/foobar >> $seqres.full
>> +truncate -s 128k $SCRATCH_MNT/foobar
>> +
>> +old_csum=$(_md5_checksum $SCRATCH_MNT/foobar)
>> +echo "=== File extent layout before autodefrag ===" >> $seqres.full
>> +$XFS_IO_PROG -c "fiemap -v" "$SCRATCH_MNT/foobar" >> $seqres.full
>> +echo "old md5=$old_csum" >> $seqres.full
>> +
>> +old_regular=$(_get_file_extent_sector "$SCRATCH_MNT/foobar" 0)
>> +old_hole=$(_get_file_extent_sector "$SCRATCH_MNT/foobar" 64k)
>> +
>> +# Now trigger autodefrag, autodefrag is triggered in the cleaner thread,
>> +# which will be woken up by commit thread
>> +_scratch_remount commit=1
>> +sleep 3
>> +sync
>> +
>> +new_csum=$(_md5_checksum $SCRATCH_MNT/foobar)
>> +new_regular=$(_get_file_extent_sector "$SCRATCH_MNT/foobar" 0)
>> +new_hole=$(_get_file_extent_sector "$SCRATCH_MNT/foobar" 64k)
>> +
>> +echo "=== File extent layout after autodefrag ===" >> $seqres.full
>> +$XFS_IO_PROG -c "fiemap -v" "$SCRATCH_MNT/foobar" >> $seqres.full
>> +echo "new md5=$new_csum" >> $seqres.full
>> +
>> +# In v5.11~v5.15 kernels, regular extents won't get defragged, and would trigger
>> +# the following output
>> +if [ $new_regular == $old_regular ]; then
>> +	echo "regular extents didn't get defragged"
>> +fi
>> +
>> +# In v5.10 and earlier kernel, autodefrag may choose to defrag holes,
>> +# which should be avoided.
>> +if [ "$new_hole" != "$old_hole" ]; then
>> +	echo "hole extents got defragged"
>> +fi
>> +
>> +# Defrag should not change file content
>> +if [ "$new_csum" != "$old_csum" ]; then
>> +	echo "file content changed"
>> +fi
>> +
>> +echo "Silence is golden"
>> +# success, all done
>> +status=0
>> +exit
>> diff --git a/tests/btrfs/256.out b/tests/btrfs/256.out
>> new file mode 100644
>> index 00000000..7ee8e2e5
>> --- /dev/null
>> +++ b/tests/btrfs/256.out
>> @@ -0,0 +1,2 @@
>> +QA output created by 256
>> +Silence is golden
>> --
>> 2.34.1
>>

Re: [PATCH v2 2/3] btrfs: test autodefrag with regular and hole extents

[Filipe Manana]

On Fri, Jan 28, 2022 at 12:13 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>
>
>
> On 2022/1/28 19:57, Filipe Manana wrote:
> > On Fri, Jan 28, 2022 at 08:27:00AM +0800, Qu Wenruo wrote:
> >> In v5.11~v5.15 kernels, there is a regression in autodefrag that if a
> >> cluster (up to 256K in size) has even a single hole, the whole cluster
> >> will be rejected.
> >>
> >> This will greatly reduce the efficiency of autodefrag.
> >>
> >> The behavior is fixed in v5.16 by a full rework, although the rework
> >> itself has other problems, it at least solves the problem.
> >>
> >> Here we add a test case to reproduce the case, where we have a 128K
> >> cluster, the first half is fragmented extents which can be defragged.
> >> The second half is hole.
> >>
> >> Make sure autodefrag can defrag the 64K part.
> >>
> >> Signed-off-by: Qu Wenruo <wqu@suse.com>
> >
> > Tried the test, and it succeeds here without the kernel fix applied.
> > I've ran it 10 times, and it always passed without the kernel fix.
>
> Have you tried v5.15 kernels? As v5.16 has the reworked code and will
> always pass.
>
> Or you can try to revert those 3 commits:
>
> c22a3572cbaf ("btrfs: defrag: enable defrag for subpage case")
> c635757365c3 ("btrfs: defrag: remove the old infrastructure")
> 7b508037d4ca ("btrfs: defrag: use defrag_one_cluster() to implement
> btrfs_defrag_file()")

With all those reverted, plus all the recent fixes that went to Linus'
tree, it fails as expected.
I got lost somewhere while navigating the sea of so many recent defrag
patches and versions.

So:

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

Thanks.

>
> That's how I tested the case with the same kernel base without going
> back to v5.15.
>
> And with those 3 commits reverted (aka, my baseline to emulate v5.15),
> it fails like this:
>
> btrfs/256 4s ... - output mismatch (see
> /home/adam/xfstests-dev/results//btrfs/256.out.bad)
>      --- tests/btrfs/256.out    2022-01-28 08:19:11.123333302 +0800
>      +++ /home/adam/xfstests-dev/results//btrfs/256.out.bad     2022-01-28
> 20:10:18.416666746 +0800
>      @@ -1,2 +1,3 @@
>       QA output created by 256
>      +regular extents didn't get defragged
>       Silence is golden
>      ...
>      (Run 'diff -u /home/adam/xfstests-dev/tests/btrfs/256.out
> /home/adam/xfstests-dev/results//btrfs/256.out.bad'  to see the entire diff)
> Ran: btrfs/256
>
> With the fiemap result in the full log:
> === File extent layout before autodefrag ===
> /mnt/scratch/foobar:
>   EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
>     0: [0..31]:         26720..26751        32   0x0
>     1: [32..63]:        26688..26719        32   0x0
>     2: [64..95]:        26656..26687        32   0x0
>     3: [96..127]:       26624..26655        32   0x1
>     4: [128..255]:      hole               128
> old md5=9ef8ace32f3b9890cff4fd43699bbd81
> === File extent layout after autodefrag ===
> /mnt/scratch/foobar:
>   EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
>     0: [0..31]:         26720..26751        32   0x0
>     1: [32..63]:        26688..26719        32   0x0
>     2: [64..95]:        26656..26687        32   0x0
>     3: [96..127]:       26624..26655        32   0x1
>     4: [128..255]:      hole               128
> new md5=9ef8ace32f3b9890cff4fd43699bbd81
>
> With my POC patch for v5.15 or v5.16 kernel it passes as expected.
>
> Thus that's why there is no fix mentioned in the commit message.
>
> Thanks,
> Qu
>
>
>
> >
> > Thanks.
> >
> >> ---
> >> Changelog:
> >> v2:
> >> - Use the previously define _get_file_extent_sector() helper
> >>    This also removed some out-of-sync error messages
> >>
> >> - Trigger autodefrag using commit=1 mount option
> >>    No need for special purpose patch any more.
> >>
> >> - Use xfs_io -s to skip several sync calls
> >>
> >> - Shorten the subject of the commit
> >> ---
> >>   tests/btrfs/256     | 80 +++++++++++++++++++++++++++++++++++++++++++++
> >>   tests/btrfs/256.out |  2 ++
> >>   2 files changed, 82 insertions(+)
> >>   create mode 100755 tests/btrfs/256
> >>   create mode 100644 tests/btrfs/256.out
> >>
> >> diff --git a/tests/btrfs/256 b/tests/btrfs/256
> >> new file mode 100755
> >> index 00000000..def83a15
> >> --- /dev/null
> >> +++ b/tests/btrfs/256
> >> @@ -0,0 +1,80 @@
> >> +#! /bin/bash
> >> +# SPDX-License-Identifier: GPL-2.0
> >> +# Copyright (C) 2022 SUSE Linux Products GmbH. All Rights Reserved.
> >> +#
> >> +# FS QA Test 256
> >> +#
> >> +# Make sure btrfs auto defrag can properly defrag clusters which has hole
> >> +# in the middle
> >> +#
> >> +. ./common/preamble
> >> +_begin_fstest auto defrag quick
> >> +
> >> +. ./common/btrfs
> >> +. ./common/filter
> >> +
> >> +# real QA test starts here
> >> +
> >> +# Modify as appropriate.
> >> +_supported_fs generic
> >> +_require_scratch
> >> +
> >> +# Needs 4K sectorsize, as larger sectorsize can change the file layout.
> >> +_require_btrfs_support_sectorsize 4096
> >> +
> >> +_scratch_mkfs >> $seqres.full
> >> +
> >> +# Need datacow to show which range is defragged, and we're testing
> >> +# autodefrag
> >> +_scratch_mount -o datacow,autodefrag
> >> +
> >> +# Create a layout where we have fragmented extents at [0, 64k) (sync write in
> >> +# reserve order), then a hole at [64k, 128k)
> >> +$XFS_IO_PROG -f -s -c "pwrite 48k 16k" -c "pwrite 32k 16k" \
> >> +            -c "pwrite 16k 16k" -c "pwrite 0 16k" \
> >> +            $SCRATCH_MNT/foobar >> $seqres.full
> >> +truncate -s 128k $SCRATCH_MNT/foobar
> >> +
> >> +old_csum=$(_md5_checksum $SCRATCH_MNT/foobar)
> >> +echo "=== File extent layout before autodefrag ===" >> $seqres.full
> >> +$XFS_IO_PROG -c "fiemap -v" "$SCRATCH_MNT/foobar" >> $seqres.full
> >> +echo "old md5=$old_csum" >> $seqres.full
> >> +
> >> +old_regular=$(_get_file_extent_sector "$SCRATCH_MNT/foobar" 0)
> >> +old_hole=$(_get_file_extent_sector "$SCRATCH_MNT/foobar" 64k)
> >> +
> >> +# Now trigger autodefrag, autodefrag is triggered in the cleaner thread,
> >> +# which will be woken up by commit thread
> >> +_scratch_remount commit=1
> >> +sleep 3
> >> +sync
> >> +
> >> +new_csum=$(_md5_checksum $SCRATCH_MNT/foobar)
> >> +new_regular=$(_get_file_extent_sector "$SCRATCH_MNT/foobar" 0)
> >> +new_hole=$(_get_file_extent_sector "$SCRATCH_MNT/foobar" 64k)
> >> +
> >> +echo "=== File extent layout after autodefrag ===" >> $seqres.full
> >> +$XFS_IO_PROG -c "fiemap -v" "$SCRATCH_MNT/foobar" >> $seqres.full
> >> +echo "new md5=$new_csum" >> $seqres.full
> >> +
> >> +# In v5.11~v5.15 kernels, regular extents won't get defragged, and would trigger
> >> +# the following output
> >> +if [ $new_regular == $old_regular ]; then
> >> +    echo "regular extents didn't get defragged"
> >> +fi
> >> +
> >> +# In v5.10 and earlier kernel, autodefrag may choose to defrag holes,
> >> +# which should be avoided.
> >> +if [ "$new_hole" != "$old_hole" ]; then
> >> +    echo "hole extents got defragged"
> >> +fi
> >> +
> >> +# Defrag should not change file content
> >> +if [ "$new_csum" != "$old_csum" ]; then
> >> +    echo "file content changed"
> >> +fi
> >> +
> >> +echo "Silence is golden"
> >> +# success, all done
> >> +status=0
> >> +exit
> >> diff --git a/tests/btrfs/256.out b/tests/btrfs/256.out
> >> new file mode 100644
> >> index 00000000..7ee8e2e5
> >> --- /dev/null
> >> +++ b/tests/btrfs/256.out
> >> @@ -0,0 +1,2 @@
> >> +QA output created by 256
> >> +Silence is golden
> >> --
> >> 2.34.1
> >>

[PATCH 3/3] btrfs: test defrag with compressed extents

[Qu Wenruo]

There is a long existing bug in btrfs defrag code that it will always
try to defrag compressed extents, even they are already at max capacity.

This will not reduce the number of extents, but only waste IO/CPU.

The kernel fix is titled:

  btrfs: defrag: don't defrag extents which is already at its max capacity

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
Changelog:
v2:
- Use fiemap output to compare the difference
  Now no need to use _get_file_extent_sector() helper at all.

- Remove unnecessary mount options

- Enlarge the write size to 16M
  To be future proof

- Shorten the subject
---
 tests/btrfs/257     | 57 +++++++++++++++++++++++++++++++++++++++++++++
 tests/btrfs/257.out |  2 ++
 2 files changed, 59 insertions(+)
 create mode 100755 tests/btrfs/257
 create mode 100644 tests/btrfs/257.out

diff --git a/tests/btrfs/257 b/tests/btrfs/257
new file mode 100755
index 00000000..bacd0c23
--- /dev/null
+++ b/tests/btrfs/257
@@ -0,0 +1,57 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2022 SUSE Linux Products GmbH. All Rights Reserved.
+#
+# FS QA Test 257
+#
+# Make sure btrfs defrag ioctl won't defrag compressed extents which are already
+# at their max capacity.
+#
+. ./common/preamble
+_begin_fstest auto quick defrag
+
+# Import common functions.
+. ./common/filter
+. ./common/btrfs
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs btrfs
+_require_scratch
+
+_scratch_mkfs >> $seqres.full
+
+_scratch_mount -o compress
+
+# Btrfs uses 128K as max extent size for compressed extents, this would result
+# several compressed extents all at their max size
+$XFS_IO_PROG -f -c "pwrite -S 0xee 0 16m" -c sync \
+	$SCRATCH_MNT/foobar >> $seqres.full
+
+old_csum=$(_md5_checksum $SCRATCH_MNT/foobar)
+
+echo "=== File extent layout before defrag ===" >> $seqres.full
+$XFS_IO_PROG -c "fiemap -v" "$SCRATCH_MNT/foobar" >> $seqres.full
+$XFS_IO_PROG -c "fiemap -v" "$SCRATCH_MNT/foobar" > $tmp.before
+
+$BTRFS_UTIL_PROG filesystem defrag "$SCRATCH_MNT/foobar" >> $seqres.full
+sync
+
+new_csum=$(_md5_checksum $SCRATCH_MNT/foobar)
+
+echo "=== File extent layout before defrag ===" >> $seqres.full
+$XFS_IO_PROG -c "fiemap -v" "$SCRATCH_MNT/foobar" >> $seqres.full
+$XFS_IO_PROG -c "fiemap -v" "$SCRATCH_MNT/foobar" > $tmp.after
+
+if [ $new_csum != $old_csum ]; then
+	echo "file content changed"
+fi
+
+diff -q $tmp.before $tmp.after || echo "compressed extents get defragged"
+
+echo "Silence is golden"
+
+# success, all done
+status=0
+exit
diff --git a/tests/btrfs/257.out b/tests/btrfs/257.out
new file mode 100644
index 00000000..cc3693f3
--- /dev/null
+++ b/tests/btrfs/257.out
@@ -0,0 +1,2 @@
+QA output created by 257
+Silence is golden
-- 
2.34.1

Re: [PATCH 3/3] btrfs: test defrag with compressed extents

[Filipe Manana]

On Fri, Jan 28, 2022 at 08:27:01AM +0800, Qu Wenruo wrote:
> There is a long existing bug in btrfs defrag code that it will always
> try to defrag compressed extents, even they are already at max capacity.
> 
> This will not reduce the number of extents, but only waste IO/CPU.
> 
> The kernel fix is titled:
> 
>   btrfs: defrag: don't defrag extents which is already at its max capacity
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
> Changelog:
> v2:
> - Use fiemap output to compare the difference
>   Now no need to use _get_file_extent_sector() helper at all.
> 
> - Remove unnecessary mount options
> 
> - Enlarge the write size to 16M
>   To be future proof
> 
> - Shorten the subject
> ---
>  tests/btrfs/257     | 57 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/btrfs/257.out |  2 ++
>  2 files changed, 59 insertions(+)
>  create mode 100755 tests/btrfs/257
>  create mode 100644 tests/btrfs/257.out
> 
> diff --git a/tests/btrfs/257 b/tests/btrfs/257
> new file mode 100755
> index 00000000..bacd0c23
> --- /dev/null
> +++ b/tests/btrfs/257
> @@ -0,0 +1,57 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (C) 2022 SUSE Linux Products GmbH. All Rights Reserved.
> +#
> +# FS QA Test 257
> +#
> +# Make sure btrfs defrag ioctl won't defrag compressed extents which are already
> +# at their max capacity.
> +#
> +. ./common/preamble
> +_begin_fstest auto quick defrag

Still missing the 'compress' group.

> +
> +# Import common functions.
> +. ./common/filter
> +. ./common/btrfs
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs btrfs
> +_require_scratch
> +
> +_scratch_mkfs >> $seqres.full
> +
> +_scratch_mount -o compress
> +
> +# Btrfs uses 128K as max extent size for compressed extents, this would result
> +# several compressed extents all at their max size
> +$XFS_IO_PROG -f -c "pwrite -S 0xee 0 16m" -c sync \
> +	$SCRATCH_MNT/foobar >> $seqres.full
> +
> +old_csum=$(_md5_checksum $SCRATCH_MNT/foobar)
> +
> +echo "=== File extent layout before defrag ===" >> $seqres.full
> +$XFS_IO_PROG -c "fiemap -v" "$SCRATCH_MNT/foobar" >> $seqres.full
> +$XFS_IO_PROG -c "fiemap -v" "$SCRATCH_MNT/foobar" > $tmp.before
> +
> +$BTRFS_UTIL_PROG filesystem defrag "$SCRATCH_MNT/foobar" >> $seqres.full
> +sync
> +
> +new_csum=$(_md5_checksum $SCRATCH_MNT/foobar)
> +
> +echo "=== File extent layout before defrag ===" >> $seqres.full
> +$XFS_IO_PROG -c "fiemap -v" "$SCRATCH_MNT/foobar" >> $seqres.full
> +$XFS_IO_PROG -c "fiemap -v" "$SCRATCH_MNT/foobar" > $tmp.after
> +
> +if [ $new_csum != $old_csum ]; then
> +	echo "file content changed"
> +fi
> +
> +diff -q $tmp.before $tmp.after || echo "compressed extents get defragged"

get -> got

Like patch 1/3, it can probably be fixed by Eryu when he picks it.

Minor things apart, it looks good, and it fails without the kernel patch
and passes with it, as expected.

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

Thanks.

> +
> +echo "Silence is golden"
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/btrfs/257.out b/tests/btrfs/257.out
> new file mode 100644
> index 00000000..cc3693f3
> --- /dev/null
> +++ b/tests/btrfs/257.out
> @@ -0,0 +1,2 @@
> +QA output created by 257
> +Silence is golden
> -- 
> 2.34.1
> 

Re: [PATCH v3 1/3] btrfs: test defrag with regular and preallocated extents

[Filipe Manana]

On Fri, Jan 28, 2022 at 08:26:59AM +0800, Qu Wenruo wrote:
> Recent v5.16 has some regression around btrfs autodefrag mount option,
> and the extra scrutiny around defrag code exposes some questionable
> behavior from the old code.
> 
> One behavior is to defrag extents along with the next preallocated
> extent.
> 
> This behavior will cause extra IO and convert all the preallocated
> extent to regular zero filled extents, rendering the preallocated extent
> useless.
> 
> The kernel fix is titled:
> 
>   btrfs: defrag: don't try to merge regular extents with preallocated extents
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
> Changelog:
> v2:
> - Add _require_btrfs_support_sectorsize() helper
>   And use it to make sure the platform supports 4k sectorsize
> 
> - Use $AWK_PROG to replace awk
> 
> v3:
> - Move _get_file_extent_sector() into common/rc
> ---
>  common/btrfs        | 16 ++++++++++
>  common/rc           | 18 +++++++++++
>  tests/btrfs/255     | 75 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/btrfs/255.out |  2 ++
>  4 files changed, 111 insertions(+)
>  create mode 100755 tests/btrfs/255
>  create mode 100644 tests/btrfs/255.out
> 
> diff --git a/common/btrfs b/common/btrfs
> index 4afe81eb..5de926dd 100644
> --- a/common/btrfs
> +++ b/common/btrfs
> @@ -480,3 +480,19 @@ _btrfs_no_v1_cache_opt()
>  	fi
>  	echo -n "-onospace_cache"
>  }
> +
> +# Require certain sectorsize support
> +_require_btrfs_support_sectorsize()
> +{
> +	local sectorsize=$1
> +
> +	# PAGE_SIZE as sectorsize is always supported
> +	if [ $sectorsize -eq $(get_page_size) ]; then
> +		return
> +	fi
> +
> +	test -f /sys/fs/btrfs/features/supported_sectorsizes || \
> +		_notrun "no subpage support found"
> +	grep -wq $sectorsize /sys/fs/btrfs/features/supported_sectorsizes || \
> +		_notrun "sectorsize $sectorsize is not supported"
> +}
> diff --git a/common/rc b/common/rc
> index b3289de9..8fbb32f8 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -3767,6 +3767,24 @@ _count_attr_extents()
>  	$XFS_IO_PROG -c "fiemap -a" $1 | tail -n +2 | grep -v hole | wc -l
>  }
>  
> +# Get the sector number of the extent at @offset of @file
> +_get_file_extent_sector()
> +{
> +	local file=$1
> +	local offset=$2
> +	local result
> +
> +	result=$($XFS_IO_PROG -c "fiemap $offset" "$file" | \
> +		 _filter_xfs_io_fiemap | head -n1 | $AWK_PROG '{print $3}')
> +
> +	# xfs_io fiemap will output nothing if there is only hole, so here
> +	# to replace the empty string with "hole" instead
> +	if [ -z "$result" ]; then
> +		result="hole"
> +	fi
> +	echo "$result"
> +}
> +
>  # arg 1 is dev to remove and is output of the below eg.
>  # ls -l /sys/class/block/sdd | rev | cut -d "/" -f 3 | rev
>  _devmgt_remove()
> diff --git a/tests/btrfs/255 b/tests/btrfs/255
> new file mode 100755
> index 00000000..fb80359c
> --- /dev/null
> +++ b/tests/btrfs/255
> @@ -0,0 +1,75 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (C) 2022 SUSE Linux Products GmbH. All Rights Reserved.
> +#
> +# FS QA Test 255
> +#
> +# Make sure btrfs doesn't defrag preallocated extents, nor lone extents
> +# before preallocated extents.
> +#
> +
> +. ./common/preamble
> +_begin_fstest auto quick defrag

Missing the 'prealloc' group.

> +
> +# Override the default cleanup function.
> +# _cleanup()
> +# {
> +# 	cd /
> +# 	rm -r -f $tmp.*
> +# }
> +
> +# Import common functions.
> +. ./common/filter
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs btrfs
> +_require_scratch
> +
> +# Needs 4K sectorsize
> +_require_btrfs_support_sectorsize 4096

We use the falloc command of xfs_io, so missing:

_require_xfs_io_command "falloc"

> +
> +_scratch_mkfs -s 4k >> $seqres.full 2>&1
> +
> +# Need datacow to make the defragged extents to have different bytenr
> +_scratch_mount -o datacow
> +
> +# Create a file with the following layout:
> +# 0       4K        8K        16K
> +# |<- R ->|<-- Preallocated ->|
> +# R is regular extents.
> +#
> +# In this case it makes no sense to defrag any extent.
> +$XFS_IO_PROG -f -c "pwrite 0 4k" -c sync -c "falloc 4k 12k" \
> +	"$SCRATCH_MNT/foobar" >> $seqres.full
> +
> +echo "=== Initial file extent layout ===" >> $seqres.full
> +$XFS_IO_PROG -c "fiemap -v" "$SCRATCH_MNT/foobar" >> $seqres.full
> +
> +# Save the bytenr of both extents
> +old_regular=$(_get_file_extent_sector "$SCRATCH_MNT/foobar" 0)
> +old_prealloc=$(_get_file_extent_sector "$SCRATCH_MNT/foobar" 4096)
> +
> +# Now defrag and write the defragged range back to disk
> +$BTRFS_UTIL_PROG filesystem defrag "$SCRATCH_MNT/foobar" >> $seqres.full
> +sync
> +
> +echo "=== File extent layout after defrag ===" >> $seqres.full
> +$XFS_IO_PROG -c "fiemap -v" "$SCRATCH_MNT/foobar" >> $seqres.full
> +
> +new_regular=$(_get_file_extent_sector "$SCRATCH_MNT/foobar" 0)
> +new_prealloc=$(_get_file_extent_sector "$SCRATCH_MNT/foobar" 4096)
> +
> +if [ "$old_regular" -ne "$new_regular" ]; then
> +	echo "the single lone sector get defragged"

get -> got

> +fi
> +if [ "$old_prealloc" -ne "$new_prealloc" ]; then
> +	echo "the preallocated extent get defragged"

get -> got

I don't want to make you send yet another patch version, and those
are minor things that Eryu can probably change when picks the patch,
so:

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

The test fails as expected without the btrfs fix, and passes with
it with.

Thanks.

> +fi
> +
> +echo "Silence is golden"
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/btrfs/255.out b/tests/btrfs/255.out
> new file mode 100644
> index 00000000..7eefb828
> --- /dev/null
> +++ b/tests/btrfs/255.out
> @@ -0,0 +1,2 @@
> +QA output created by 255
> +Silence is golden
> -- 
> 2.34.1
>