On Tue, Feb 6, 2024 at 11:30 PM Qu Wenruo <wqu@xxxxxxxx> 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@xxxxxxxx>
> ---
> 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
>
>
