Hi Filipe,
Thank you for the review and the verification toward unpatched kernel.
I added these two small changes in patch v3, but I also fixed the
_scratch_mkfs output and removed _cleanup() as Zorro suggested.
Since I did other changes in patch v3, could you take a look on it and
reply the Reviewed-by tag? Thanks.
Filipe Manana <fdmanana@xxxxxxxxxx> 於 2022年8月22日 週一 晚上7:24寫道：
>
> On Mon, Aug 22, 2022 at 08:49:32AM +0800, bingjingc wrote:
> > From: BingJing Chang <bingjingc@xxxxxxxxxxxx>
> >
> > Normally btrfs stores file paths in an array of ref items. However, items
> > for the same parent directory can not exceed the size of a leaf. So btrfs
> > also store the rest of them in extended ref items alternatively.
> >
> > In this test, it creates a large number of links under a directory causing
> > the file paths stored in these two ways to be the parent snapshot. And it
> > deletes and recreates just an amount of them that can be stored within an
> > array of ref items to be the send snapshot. Test that an incremental send
> > operation correctly issues link/unlink operations only against new/deleted
> > paths, or the receive operation will fail due to a link on an existed path.
> >
> > This currently fails on btrfs but is fixed by a kernel patch with the
> > commit 3aa5bd367fa5a3 ("btrfs: send: fix sending link commands for
> > existing file paths")
> >
> > Signed-off-by: BingJing Chang <bingjingc@xxxxxxxxxxxx>
> > ---
> > tests/btrfs/272 | 97 +++++++++++++++++++++++++++++++++++++++++++++
> > tests/btrfs/272.out | 3 ++
> > 2 files changed, 100 insertions(+)
> > create mode 100755 tests/btrfs/272
> > create mode 100644 tests/btrfs/272.out
> >
> > diff --git a/tests/btrfs/272 b/tests/btrfs/272
> > new file mode 100755
> > index 00000000..e1986de9
> > --- /dev/null
> > +++ b/tests/btrfs/272
> > @@ -0,0 +1,97 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2022 BingJing Chang.
> > +#
> > +# FS QA Test No. btrfs/272
> > +#
> > +# Regression test for btrfs incremental send issue where a link instruction
> > +# is sent against an existing path, causing btrfs receive to fail.
> > +#
> > +# This issue is fixed by the following linux kernel btrfs patch:
> > +#
> > +# commit 3aa5bd367fa5a3 ("btrfs: send: fix sending link commands for
> > +# existing file paths")
> > +#
> > +. ./common/preamble
> > +_begin_fstest auto quick send
> > +
> > +# Override the default cleanup function.
> > +_cleanup()
> > +{
> > + cd /
> > + rm -fr $send_files_dir
> > + rm -f $tmp.*
> > +}
> > +
> > +# Import common functions.
> > +. ./common/filter
> > +
> > +# real QA test starts here
> > +_supported_fs btrfs
>
> I didn't tell you before, but I wasn't aware back then, that we now have
> an annotation to specify kernel commits, se here we should add:
>
> _fixed_by_kernel_commit 3aa5bd367fa5a3 \
> "btrfs: send: fix sending link commands for existing file paths"
>
>
> > +_require_test
> > +_require_scratch
> > +_require_fssum
> > +
> > +send_files_dir=$TEST_DIR/btrfs-test-$seq
> > +
> > +rm -fr $send_files_dir
> > +mkdir $send_files_dir
> > +
> > +_scratch_mkfs > /dev/null 2>&1
> > +_scratch_mount
> > +
> > +# Create a file and 2000 hard links to the same inode
> > +_run_btrfs_util_prog subvolume create $SCRATCH_MNT/vol
> > +touch $SCRATCH_MNT/vol/foo
> > +for i in {1..2000}; do
> > + link $SCRATCH_MNT/vol/foo $SCRATCH_MNT/vol/$i
> > +done
> > +
> > +# Create a snapshot for a full send operation
> > +_run_btrfs_util_prog subvolume snapshot -r $SCRATCH_MNT/vol $SCRATCH_MNT/snap1
> > +_run_btrfs_util_prog send -f $send_files_dir/1.snap $SCRATCH_MNT/snap1
> > +
> > +# Remove 2000 hard links and re-create the last 1000 links
> > +for i in {1..2000}; do
> > + rm $SCRATCH_MNT/vol/$i
> > +done
> > +for i in {1001..2000}; do
> > + link $SCRATCH_MNT/vol/foo $SCRATCH_MNT/vol/$i
> > +done
> > +
> > +# Create another snapshot for an incremental send operation
> > +_run_btrfs_util_prog subvolume snapshot -r $SCRATCH_MNT $SCRATCH_MNT/snap2
>
> So I ran the test on an unpatched kernel and it didn't fail!
>
> The reason is that that command is taking a snapshot of $SCRATCH_MNT, when it
> should be $SCRATCH_MNT/vol. So it wasn't testing what we were supposed to test.
>
> > +_run_btrfs_util_prog send -p $SCRATCH_MNT/snap1 -f $send_files_dir/2.snap \
> > + $SCRATCH_MNT/snap2
> > +
> > +$FSSUM_PROG -A -f -w $send_files_dir/1.fssum $SCRATCH_MNT/snap1
> > +$FSSUM_PROG -A -f -w $send_files_dir/2.fssum \
> > + -x $SCRATCH_MNT/snap2/snap1 $SCRATCH_MNT/snap2
> > +
> > +# Recreate the filesystem by receiving both send streams and verify we get
> > +# the same content that the original filesystem had.
> > +_scratch_unmount
> > +_scratch_mkfs >>$seqres.full 2>&1
> > +_scratch_mount
> > +
> > +# Add the first snapshot to the new filesystem by applying the first send
> > +# stream.
> > +_run_btrfs_util_prog receive -f $send_files_dir/1.snap $SCRATCH_MNT
> > +
> > +# The incremental receive operation below used to fail with the following
> > +# error:
> > +#
> > +# ERROR: link 1238 -> foo failed: File exists
> > +#
> > +# This is because the path "1238" was stored as an extended ref item in the
> > +# original snapshot but as a normal ref item in the next snapshot. The send
> > +# operation cannot handle the duplicated paths, which are stored in
> > +# different ways, well, so it decides to issue a link operation for the
> > +# existing path. This results in the receiver to fail with the above error.
> > +_run_btrfs_util_prog receive -f $send_files_dir/2.snap $SCRATCH_MNT
>
> Thanks for following the style of btrfs/241 and putting here an explanation
> of why it failed!
>
> Btw, this patch didn't reach the btrfs mailing list, you typed the address as
> "linux-btrfs@vger.kernel.orgto", an extra "to" at the end.
>
> So I'm adding the list to cc.
>
> Anyway, with those two small changes, the patch will look good to me, then
> you can add:
>
> Reviewed-by: Filipe Manana <fdmanana@xxxxxxxx>
>
> Thanks for doing this!
>
> > +
> > +$FSSUM_PROG -r $send_files_dir/1.fssum $SCRATCH_MNT/snap1
> > +$FSSUM_PROG -r $send_files_dir/2.fssum $SCRATCH_MNT/snap2
> > +
> > +status=0
> > +exit
> > diff --git a/tests/btrfs/272.out b/tests/btrfs/272.out
> > new file mode 100644
> > index 00000000..b009b87a
> > --- /dev/null
> > +++ b/tests/btrfs/272.out
> > @@ -0,0 +1,3 @@
> > +QA output created by 272
> > +OK
> > +OK
> > --
> > 2.37.1
> >
