Re: [PATCH] btrfs/319: make the test work when compression is used

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

在 2024/9/6 20:27, Filipe Manana 写道:
> On Fri, Sep 6, 2024 at 11:39 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>>
>>
>>
>> 在 2024/9/6 01:08, fdmanana@kernel.org 写道:
>>> From: Filipe Manana <fdmanana@suse.com>
>>>
>>> Currently btrfs/319 assumes there is no compression and that the files
>>> get a single extent (1 fiemap line) with a size of 1048581 bytes. However
>>> when testing with compression, for example by passing "-o compress" to
>>> MOUNT_OPTIONS environment variable, we get several extents and two lines
>>> of fiemap output, which makes the test fail since it hardcodes the fiemap
>>> output:
>>>
>>>     $ MOUNT_OPTIONS="-o compress" ./check btrfs/319
>>>     FSTYP         -- btrfs
>>>     PLATFORM      -- Linux/x86_64 debian0 6.11.0-rc6-btrfs-next-173+ #1 SMP PREEMPT_DYNAMIC Tue Sep  3 17:40:24 WEST 2024
>>>     MKFS_OPTIONS  -- /dev/sdc
>>>     MOUNT_OPTIONS -- -o compress /dev/sdc /home/fdmanana/btrfs-tests/scratch_1
>>>
>>>     btrfs/319 1s ... - output mismatch (see /home/fdmanana/git/hub/xfstests/results//btrfs/319.out.bad)
>>>         --- tests/btrfs/319.out        2024-08-12 14:16:55.653383284 +0100
>>>         +++ /home/fdmanana/git/hub/xfstests/results//btrfs/319.out.bad 2024-09-05 15:24:53.323076548 +0100
>>>         @@ -6,11 +6,13 @@
>>>          e61178ee0288ebe3fa36a3c975b02c94  SCRATCH_MNT/snap/foo
>>>          e61178ee0288ebe3fa36a3c975b02c94  SCRATCH_MNT/snap/bar
>>>          File bar fiemap in the original filesystem:
>>>         -0: [0..2055]: shared|last
>>>         +0: [0..2047]: shared
>>>         +1: [2048..2055]: shared|last
>>>          Creating a new filesystem to receive the send stream...
>>>         ...
>>>         (Run 'diff -u /home/fdmanana/git/hub/xfstests/tests/btrfs/319.out /home/fdmanana/git/hub/xfstests/results//btrfs/319.out.bad'  to see the entire diff)
>>>
>>>     HINT: You _MAY_ be missing kernel fix:
>>>           46a6e10a1ab1 btrfs: send: allow cloning non-aligned extent if it ends at i_size
>>>
>>>     Ran: btrfs/319
>>>     Failures: btrfs/319
>>>     Failed 1 of 1 tests
>>>
>>> So change the test to not rely on the fiemap output in its golden output
>>> and instead just check if all the extents reported by fiemap have the
>>> shared flag set (failing if there are any without the shared flag).
>>
>> Looks good to me.
>>
>> Reviewed-by: Qu Wenruo <wqu@suse.com>
>>
>> Just one minor improvement to make debug a little easier.
>>
>>>
>>> Signed-off-by: Filipe Manana <fdmanana@suse.com>
>>> ---
>>>    tests/btrfs/319     | 19 +++++++++++++++----
>>>    tests/btrfs/319.out |  4 ----
>>>    2 files changed, 15 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/tests/btrfs/319 b/tests/btrfs/319
>>> index 975c1497..7cfd3d00 100755
>>> --- a/tests/btrfs/319
>>> +++ b/tests/btrfs/319
>>> @@ -32,6 +32,19 @@ _require_odirect
>>>    _fixed_by_kernel_commit 46a6e10a1ab1 \
>>>        "btrfs: send: allow cloning non-aligned extent if it ends at i_size"
>>>
>>> +check_all_extents_shared()
>>> +{
>>> +     local file=$1
>>> +     local fiemap_output
>>> +
>>> +     fiemap_output=$($XFS_IO_PROG -r -c "fiemap -v" $file | _filter_fiemap_flags)
>>
>> Maybe also save the full unfiltered output to seqres.full?
>
> Hum, why?
>
> Do you think looking at the flags in hexadecimal values instead of
> symbolic names like "shared", "encoded", "last", etc, is easier to
> read?

My bad, I thought that filter only shows the flags, but it turns out
that keeps the filepos and length.

So the filtered one is much better in all aspects.

Thanks,
Qu

>
> Thanks.
>
>>
>> Thanks,
>> Qu
>>
>>> +     echo "$fiemap_output" | grep -qv 'shared'
>>> +     if [ $? -eq 0 ]; then
>>> +             echo -e "Found non-shared extents for file $file:\n"
>>> +             echo "$fiemap_output"
>>> +     fi
>>> +}
>>> +
>>>    send_files_dir=$TEST_DIR/btrfs-test-$seq
>>>    send_stream=$send_files_dir/snap.stream
>>>
>>> @@ -58,8 +71,7 @@ echo "File digests in the original filesystem:"
>>>    md5sum $SCRATCH_MNT/snap/foo | _filter_scratch
>>>    md5sum $SCRATCH_MNT/snap/bar | _filter_scratch
>>>
>>> -echo "File bar fiemap in the original filesystem:"
>>> -$XFS_IO_PROG -r -c "fiemap -v" $SCRATCH_MNT/snap/bar | _filter_fiemap_flags
>>> +check_all_extents_shared "$SCRATCH_MNT/snap/bar"
>>>
>>>    echo "Creating a new filesystem to receive the send stream..."
>>>    _scratch_unmount
>>> @@ -72,8 +84,7 @@ echo "File digests in the new filesystem:"
>>>    md5sum $SCRATCH_MNT/snap/foo | _filter_scratch
>>>    md5sum $SCRATCH_MNT/snap/bar | _filter_scratch
>>>
>>> -echo "File bar fiemap in the new filesystem:"
>>> -$XFS_IO_PROG -r -c "fiemap -v" $SCRATCH_MNT/snap/bar | _filter_fiemap_flags
>>> +check_all_extents_shared "$SCRATCH_MNT/snap/bar"
>>>
>>>    # success, all done
>>>    status=0
>>> diff --git a/tests/btrfs/319.out b/tests/btrfs/319.out
>>> index 14079dbe..18a50ff8 100644
>>> --- a/tests/btrfs/319.out
>>> +++ b/tests/btrfs/319.out
>>> @@ -5,12 +5,8 @@ Creating snapshot and a send stream for it...
>>>    File digests in the original filesystem:
>>>    e61178ee0288ebe3fa36a3c975b02c94  SCRATCH_MNT/snap/foo
>>>    e61178ee0288ebe3fa36a3c975b02c94  SCRATCH_MNT/snap/bar
>>> -File bar fiemap in the original filesystem:
>>> -0: [0..2055]: shared|last
>>>    Creating a new filesystem to receive the send stream...
>>>    At subvol snap
>>>    File digests in the new filesystem:
>>>    e61178ee0288ebe3fa36a3c975b02c94  SCRATCH_MNT/snap/foo
>>>    e61178ee0288ebe3fa36a3c975b02c94  SCRATCH_MNT/snap/bar
>>> -File bar fiemap in the new filesystem:
>>> -0: [0..2055]: shared|last
>