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

From: Filipe Manana <fdmanana@kernel.org>
To: Qu Wenruo <wqu@suse.com>
Cc: fstests@vger.kernel.org, linux-btrfs@vger.kernel.org
Subject: Re: [PATCH 3/3] btrfs: test defrag with compressed extents
Date: Fri, 28 Jan 2022 12:20:17 +0000	[thread overview]
Message-ID: <YfPfgaWLtKNJcVO3@debian9.Home> (raw)
In-Reply-To: <20220128002701.11971-3-wqu@suse.com>

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
> 