From: Filipe Manana <fdmanana@kernel.org>
To: Qu Wenruo <wqu@suse.com>
Cc: linux-btrfs@vger.kernel.org, fstests@vger.kernel.org
Subject: Re: [PATCH] fstests: btrfs: make sure defrag doesn't increase space usage
Date: Wed, 7 Feb 2024 15:00:59 +0000 [thread overview]
Message-ID: <CAL3q7H5fBLhnsp2iVEXDWvM-V-0=VnRhwRvL0O74sf9+xyXQXg@mail.gmail.com> (raw)
In-Reply-To: <20240206233024.35399-1-wqu@suse.com>
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
>
>
prev parent reply other threads:[~2024-02-07 15:01 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-06 23:30 [PATCH] fstests: btrfs: make sure defrag doesn't increase space usage Qu Wenruo
2024-02-07 15:00 ` Filipe Manana [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAL3q7H5fBLhnsp2iVEXDWvM-V-0=VnRhwRvL0O74sf9+xyXQXg@mail.gmail.com' \
--to=fdmanana@kernel.org \
--cc=fstests@vger.kernel.org \
--cc=linux-btrfs@vger.kernel.org \
--cc=wqu@suse.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).