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

[Qu Wenruo <quwenruo.btrfs@xxxxxxx>]

在 2024/9/6 20:27, Filipe Manana 写道:
> On Fri, Sep 6, 2024 at 11:39 AM Qu Wenruo <quwenruo.btrfs@xxxxxxx> wrote:
> > 在 2024/9/6 01:08, fdmanana@xxxxxxxxxx 写道:
> > > From: Filipe Manana <fdmanana@xxxxxxxx>
> > >
> > > 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@xxxxxxxx>
> >
> > Just one minor improvement to make debug a little easier.
> >
> > > Signed-off-by: Filipe Manana <fdmanana@xxxxxxxx>
> > > ---
> > >    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