Re: [PATCH 2/3] btrfs: test that fiemap reports extent as not shared after deleting file

[Zorro Lang <zlang@redhat.com>]

On Mon, Oct 31, 2022 at 04:51:06PM +0000, Filipe Manana wrote:
> On Mon, Oct 31, 2022 at 4:41 PM Zorro Lang <zlang@redhat.com> wrote:
> >
> > On Mon, Oct 31, 2022 at 11:11:20AM +0000, fdmanana@kernel.org wrote:
> > > From: Filipe Manana <fdmanana@suse.com>
> > >
> > > Test that if we have two files with shared extents, after removing one of
> > > the files, if we do a fiemap against the other file, it does not report
> > > extents as shared anymore.
> > >
> > > This exercises the processing of delayed references for data extents in
> > > the backref walking code, used by fiemap to determine if an extent is
> > > shared.
> > >
> > > This used to fail until very recently and was fixed by the following
> > > kernel commit that landed in 6.1-rc2:
> > >
> > >   4fc7b5722824 (""btrfs: fix processing of delayed data refs during backref walking")
> > >
> > > Signed-off-by: Filipe Manana <fdmanana@suse.com>
> > > ---
> >
> > Looks good to me. As this's not a genericcase, I'm not sure if you need
> > _require_congruent_file_oplen helper.
> 
> Hum? Why would it be needed?
> That helper was introduced because of xfs's realtime config.
> On btrfs reflinking always works for any multiple of the sector size.

Sure, I was not asking you to use that, as it's not a generic case, so I just
tried to double check with you, to make sure btrfs doesn't need it. Thanks
for you confirm that, this patchset looks good to me.

Reviewed-by: Zorro Lang <zlang@redhat.com>

Thanks,
Zorro

> 
> >
> > Thanks,
> > Zorro
> >
> > >  tests/btrfs/279     | 82 +++++++++++++++++++++++++++++++++++++++++++++
> > >  tests/btrfs/279.out | 39 +++++++++++++++++++++
> > >  2 files changed, 121 insertions(+)
> > >  create mode 100755 tests/btrfs/279
> > >  create mode 100644 tests/btrfs/279.out
> > >
> > > diff --git a/tests/btrfs/279 b/tests/btrfs/279
> > > new file mode 100755
> > > index 00000000..5b5824fd
> > > --- /dev/null
> > > +++ b/tests/btrfs/279
> > > @@ -0,0 +1,82 @@
> > > +#! /bin/bash
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +# Copyright (C) 2022 SUSE Linux Products GmbH. All Rights Reserved.
> > > +#
> > > +# FS QA Test 279
> > > +#
> > > +# Test that if we have two files with shared extents, after removing one of the
> > > +# files, if we do a fiemap against the other file, it does not report extents as
> > > +# shared anymore.
> > > +#
> > > +# This exercises the processing of delayed references for data extents in the
> > > +# backref walking code, used by fiemap to determine if an extent is shared.
> > > +#
> > > +. ./common/preamble
> > > +_begin_fstest auto quick subvol fiemap clone
> > > +
> > > +. ./common/filter
> > > +. ./common/reflink
> > > +. ./common/punch # for _filter_fiemap_flags
> > > +
> > > +_supported_fs btrfs
> > > +_require_scratch_reflink
> > > +_require_cp_reflink
> > > +_require_xfs_io_command "fiemap"
> > > +
> > > +_fixed_by_kernel_commit 4fc7b5722824 \
> > > +     "btrfs: fix processing of delayed data refs during backref walking"
> > > +
> > > +run_test()
> > > +{
> > > +     local file_path_1=$1
> > > +     local file_path_2=$2
> > > +     local do_sync=$3
> > > +
> > > +     $XFS_IO_PROG -f -c "pwrite 0 64K" $file_path_1 | _filter_xfs_io
> > > +     _cp_reflink $file_path_1 $file_path_2
> > > +
> > > +     if [ $do_sync -eq 1 ]; then
> > > +             sync
> > > +     fi
> > > +
> > > +     echo "Fiemap of $file_path_1 before deleting $file_path_2:" | \
> > > +             _filter_scratch
> > > +     $XFS_IO_PROG -c "fiemap -v" $file_path_1 | _filter_fiemap_flags
> > > +
> > > +     rm -f $file_path_2
> > > +
> > > +     echo "Fiemap of $file_path_1 after deleting $file_path_2:" | \
> > > +             _filter_scratch
> > > +     $XFS_IO_PROG -c "fiemap -v" $file_path_1 | _filter_fiemap_flags
> > > +}
> > > +
> > > +_scratch_mkfs >> $seqres.full 2>&1
> > > +_scratch_mount
> > > +
> > > +# Create two test subvolumes, we'll reflink files between them.
> > > +$BTRFS_UTIL_PROG subvolume create $SCRATCH_MNT/subv1 | _filter_scratch
> > > +$BTRFS_UTIL_PROG subvolume create $SCRATCH_MNT/subv2 | _filter_scratch
> > > +
> > > +echo
> > > +echo "Testing with same subvolume and without transaction commit"
> > > +echo
> > > +run_test "$SCRATCH_MNT/subv1/f1" "$SCRATCH_MNT/subv1/f2" 0
> > > +
> > > +echo
> > > +echo "Testing with same subvolume and with transaction commit"
> > > +echo
> > > +run_test "$SCRATCH_MNT/subv1/f3" "$SCRATCH_MNT/subv1/f4" 1
> > > +
> > > +echo
> > > +echo "Testing with different subvolumes and without transaction commit"
> > > +echo
> > > +run_test "$SCRATCH_MNT/subv1/f5" "$SCRATCH_MNT/subv2/f6" 0
> > > +
> > > +echo
> > > +echo "Testing with different subvolumes and with transaction commit"
> > > +echo
> > > +run_test "$SCRATCH_MNT/subv1/f7" "$SCRATCH_MNT/subv2/f8" 1
> > > +
> > > +# success, all done
> > > +status=0
> > > +exit
> > > diff --git a/tests/btrfs/279.out b/tests/btrfs/279.out
> > > new file mode 100644
> > > index 00000000..a959a86d
> > > --- /dev/null
> > > +++ b/tests/btrfs/279.out
> > > @@ -0,0 +1,39 @@
> > > +QA output created by 279
> > > +Create subvolume 'SCRATCH_MNT/subv1'
> > > +Create subvolume 'SCRATCH_MNT/subv2'
> > > +
> > > +Testing with same subvolume and without transaction commit
> > > +
> > > +wrote 65536/65536 bytes at offset 0
> > > +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> > > +Fiemap of SCRATCH_MNT/subv1/f1 before deleting SCRATCH_MNT/subv1/f2:
> > > +0: [0..127]: shared|last
> > > +Fiemap of SCRATCH_MNT/subv1/f1 after deleting SCRATCH_MNT/subv1/f2:
> > > +0: [0..127]: last
> > > +
> > > +Testing with same subvolume and with transaction commit
> > > +
> > > +wrote 65536/65536 bytes at offset 0
> > > +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> > > +Fiemap of SCRATCH_MNT/subv1/f3 before deleting SCRATCH_MNT/subv1/f4:
> > > +0: [0..127]: shared|last
> > > +Fiemap of SCRATCH_MNT/subv1/f3 after deleting SCRATCH_MNT/subv1/f4:
> > > +0: [0..127]: last
> > > +
> > > +Testing with different subvolumes and without transaction commit
> > > +
> > > +wrote 65536/65536 bytes at offset 0
> > > +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> > > +Fiemap of SCRATCH_MNT/subv1/f5 before deleting SCRATCH_MNT/subv2/f6:
> > > +0: [0..127]: shared|last
> > > +Fiemap of SCRATCH_MNT/subv1/f5 after deleting SCRATCH_MNT/subv2/f6:
> > > +0: [0..127]: last
> > > +
> > > +Testing with different subvolumes and with transaction commit
> > > +
> > > +wrote 65536/65536 bytes at offset 0
> > > +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> > > +Fiemap of SCRATCH_MNT/subv1/f7 before deleting SCRATCH_MNT/subv2/f8:
> > > +0: [0..127]: shared|last
> > > +Fiemap of SCRATCH_MNT/subv1/f7 after deleting SCRATCH_MNT/subv2/f8:
> > > +0: [0..127]: last
> > > --
> > > 2.35.1
> > >
> >
>