[PATCH] fstests: btrfs: make sure defrag doesn't increase space usage

[PATCH] fstests: btrfs: make sure defrag doesn't increase space usage

[Qu Wenruo]

[BUG]
There is a bug report that, the following simple file layout would make
btrfs defrag to wrongly defrag part of the file, and results in
increased space usage:

 # mkfs.btrfs -f $dev
 # mount $dev $mnt
 # xfs_io -f -c "pwrite 0 40m" $mnt/foobar
 # sync
 # xfs_io -f -c "pwrite 40m 64k" $mnt/foobar.
 # sync
 # btrfs filesystem defrag $mnt/foobar
 # sync

[CAUSE]
It's a bug in the defrag decision part, where we use the length to the
end of the extent to determine if it meets our extent size threshold.

That cause us to do different defrag decision inside the same file
extent, and such defrag would cause extra space caused by btrfs data
CoW.

[TEST CASE]
The test case would just use the above workload as the core, and use
qgroups to properly record the data usage of the fs tree, to make sure
the defrag at least won't cause extra space usage in this particular
case.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 tests/btrfs/310     | 63 +++++++++++++++++++++++++++++++++++++++++++++
 tests/btrfs/310.out |  2 ++
 2 files changed, 65 insertions(+)
 create mode 100755 tests/btrfs/310
 create mode 100644 tests/btrfs/310.out

diff --git a/tests/btrfs/310 b/tests/btrfs/310
new file mode 100755
index 00000000..ca535f99
--- /dev/null
+++ b/tests/btrfs/310
@@ -0,0 +1,63 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2024 YOUR NAME HERE.  All Rights Reserved.
+#
+# FS QA Test 310
+#
+# what am I here for?
+#
+. ./common/preamble
+_begin_fstest auto quick defrag qgroup 
+
+# Modify as appropriate.
+_supported_fs btrfs
+_require_scratch
+_require_btrfs_no_nodatacow
+_fixed_by_kernel_commit XXXXXXXXXXXX \
+	"btrfs: btrfs: defrag: avoid unnecessary defrag caused by incorrect extent size"
+
+_scratch_mkfs >> $seqres.full
+
+# We require no compression and enable datacow.
+# As we rely on qgroup to give us an accurate number of used space,
+# which is based on on-disk extent size, thus we have to disable compression.
+#
+# And we rely COW to cause wasted space on unpatched kernels, thus data cow
+# is required.
+_scratch_mount -o compress=no,datacow
+
+# Enable quota to account the wasted bookend space.
+$BTRFS_UTIL_PROG quota enable $SCRATCH_MNT 2>> $seqres.full &
+_qgroup_rescan $SCRATCH_MNT >> $seqres.full
+
+# Create the following layout
+# [0, 40M)		A regular uncompressed extent
+# [40M, 40M+64K)	A small regular extent allowing merging
+$XFS_IO_PROG -f -c "pwrite 0 40M" -c sync "$SCRATCH_MNT/foobar" >> $seqres.full
+$XFS_IO_PROG -f -c "pwrite 40M 64K" -c sync "$SCRATCH_MNT/foobar" >> $seqres.full
+
+# Then record the current qgroup number, which should be 40M + 64K + nodesize
+qgroup_before=$($BTRFS_UTIL_PROG qgroup show --sync --raw "$SCRATCH_MNT" | tail -n1 | $AWK_PROG '{print $2}')
+echo "qgroup number before defrag: $qgroup_before" >> $seqres.full
+
+# Now defrag the file with the default 32M extent size threshold.
+$BTRFS_UTIL_PROG filesystem defrag -t 32M "$SCRATCH_MNT/foobar" >> $seqres.full
+
+# Write back the re-dirtied content of defrag and update qgroup.
+sync
+
+# Now check the newer qgroup numbers
+qgroup_after=$($BTRFS_UTIL_PROG qgroup show --sync --raw "$SCRATCH_MNT" | tail -n1 | $AWK_PROG '{print $2}')
+echo "qgroup number after defrag: $qgroup_after" >> $seqres.full
+
+# The new number should not exceed the old one, or the defrag itself is
+# doing more damage.
+if [ "$qgroup_after" -gt "$qgroup_before" ]; then
+	echo "defrag caused more space usage: before=$qgroup_before after=$qgroup_after"
+fi
+
+echo "Silence is golden"
+
+# success, all done
+status=0
+exit
diff --git a/tests/btrfs/310.out b/tests/btrfs/310.out
new file mode 100644
index 00000000..7b9eaf78
--- /dev/null
+++ b/tests/btrfs/310.out
@@ -0,0 +1,2 @@
+QA output created by 310
+Silence is golden
-- 
2.42.0

Re: [PATCH] fstests: btrfs: make sure defrag doesn't increase space usage

[Filipe Manana]

On Tue, Feb 6, 2024 at 11:30 PM Qu Wenruo <wqu@suse.com> wrote:
>
> [BUG]
> There is a bug report that, the following simple file layout would make
> btrfs defrag to wrongly defrag part of the file, and results in
> increased space usage:
>
>  # mkfs.btrfs -f $dev
>  # mount $dev $mnt
>  # xfs_io -f -c "pwrite 0 40m" $mnt/foobar
>  # sync
>  # xfs_io -f -c "pwrite 40m 64k" $mnt/foobar.
>  # sync
>  # btrfs filesystem defrag $mnt/foobar
>  # sync
>
> [CAUSE]
> It's a bug in the defrag decision part, where we use the length to the
> end of the extent to determine if it meets our extent size threshold.
>
> That cause us to do different defrag decision inside the same file
> extent, and such defrag would cause extra space caused by btrfs data
> CoW.
>
> [TEST CASE]
> The test case would just use the above workload as the core, and use
> qgroups to properly record the data usage of the fs tree, to make sure
> the defrag at least won't cause extra space usage in this particular
> case.
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  tests/btrfs/310     | 63 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/btrfs/310.out |  2 ++
>  2 files changed, 65 insertions(+)
>  create mode 100755 tests/btrfs/310
>  create mode 100644 tests/btrfs/310.out
>
> diff --git a/tests/btrfs/310 b/tests/btrfs/310
> new file mode 100755
> index 00000000..ca535f99
> --- /dev/null
> +++ b/tests/btrfs/310
> @@ -0,0 +1,63 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2024 YOUR NAME HERE.  All Rights Reserved.

Don't forget to update this.

> +#
> +# FS QA Test 310
> +#
> +# what am I here for?

And this too.

> +#
> +. ./common/preamble
> +_begin_fstest auto quick defrag qgroup
> +
> +# Modify as appropriate.
> +_supported_fs btrfs
> +_require_scratch
> +_require_btrfs_no_nodatacow
> +_fixed_by_kernel_commit XXXXXXXXXXXX \
> +       "btrfs: btrfs: defrag: avoid unnecessary defrag caused by incorrect extent size"
> +
> +_scratch_mkfs >> $seqres.full
> +
> +# We require no compression and enable datacow.
> +# As we rely on qgroup to give us an accurate number of used space,
> +# which is based on on-disk extent size, thus we have to disable compression.
> +#
> +# And we rely COW to cause wasted space on unpatched kernels, thus data cow
> +# is required.
> +_scratch_mount -o compress=no,datacow

datacow is redundant here, _require_btrfs_no_nodatacow was already called above.

Should also use  _require_btrfs_no_compress()

> +
> +# Enable quota to account the wasted bookend space.
> +$BTRFS_UTIL_PROG quota enable $SCRATCH_MNT 2>> $seqres.full &
> +_qgroup_rescan $SCRATCH_MNT >> $seqres.full
> +
> +# Create the following layout
> +# [0, 40M)             A regular uncompressed extent
> +# [40M, 40M+64K)       A small regular extent allowing merging
> +$XFS_IO_PROG -f -c "pwrite 0 40M" -c sync "$SCRATCH_MNT/foobar" >> $seqres.full
> +$XFS_IO_PROG -f -c "pwrite 40M 64K" -c sync "$SCRATCH_MNT/foobar" >> $seqres.full
> +
> +# Then record the current qgroup number, which should be 40M + 64K + nodesize
> +qgroup_before=$($BTRFS_UTIL_PROG qgroup show --sync --raw "$SCRATCH_MNT" | tail -n1 | $AWK_PROG '{print $2}')
> +echo "qgroup number before defrag: $qgroup_before" >> $seqres.full
> +
> +# Now defrag the file with the default 32M extent size threshold.
> +$BTRFS_UTIL_PROG filesystem defrag -t 32M "$SCRATCH_MNT/foobar" >> $seqres.full
> +
> +# Write back the re-dirtied content of defrag and update qgroup.
> +sync
> +
> +# Now check the newer qgroup numbers
> +qgroup_after=$($BTRFS_UTIL_PROG qgroup show --sync --raw "$SCRATCH_MNT" | tail -n1 | $AWK_PROG '{print $2}')
> +echo "qgroup number after defrag: $qgroup_after" >> $seqres.full
> +
> +# The new number should not exceed the old one, or the defrag itself is
> +# doing more damage.

Damage is not exactly the proper wording here, I would say wasting
space, as damage I would think of something like corruption, data
loss, etc.

Otherwise it looks fine.
Thanks.

> +if [ "$qgroup_after" -gt "$qgroup_before" ]; then
> +       echo "defrag caused more space usage: before=$qgroup_before after=$qgroup_after"
> +fi
> +
> +echo "Silence is golden"
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/btrfs/310.out b/tests/btrfs/310.out
> new file mode 100644
> index 00000000..7b9eaf78
> --- /dev/null
> +++ b/tests/btrfs/310.out
> @@ -0,0 +1,2 @@
> +QA output created by 310
> +Silence is golden
> --
> 2.42.0
>
>