Re: [PATCH] fstests: btrfs: test incremental send for changed reference paths

[bingjing chang <bxxxjxxg@gmail.com>]

Hi Filipe,

Thank you for your review comments and the clear example (btrfs/241) for me.
I've revised and submitted the patch v2 using fssum utility.

If there're still problems, please reply to that mail to let me know. Thanks!

Filipe Manana <fdmanana@kernel.org> 於 2022年8月8日 週一 下午5:30寫道：

>
> On Sat, Aug 6, 2022 at 3:35 PM bingjingc <bingjingc@synology.com> wrote:
> >
> > From: BingJing Chang <bingjingc@synology.com>
> >
> > Normally btrfs stores reference 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 reference paths stored in these two ways as the parent
> > snapshot. And it deletes and recreates just an amount of them that can be
> > stored within an array of ref items as the send snapshot. Test that an
> > incremental send operation correctly issues link/unlink operations only
> > against new/deleted reference 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
> > following subject:
>
> Thanks for sending the test BingJing!
> Some comments below.
>
> >
> >   "btrfs: send: fix sending link commands for existing file paths"
>
> Since the patch already landed in Linus' tree last week, the preferred
> format here is:
>
> commit 3aa5bd367fa5a3 ("btrfs: send: fix sending link commands for
> existing file paths")
>
> >
> > Signed-off-by: BingJing Chang <bingjingc@synology.com>
> > ---
> >  tests/btrfs/272     | 65 +++++++++++++++++++++++++++++++++++++++++++++
> >  tests/btrfs/272.out |  2 ++
> >  2 files changed, 67 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..a362d943
> > --- /dev/null
> > +++ b/tests/btrfs/272
> > @@ -0,0 +1,65 @@
> > +#! /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 existed file, causing btrfs receive to fail.
>
> existed file -> existing path
>
> > +#
> > +# This issue is fixed by the following linux kernel btrfs patch:
> > +#
> > +#   btrfs: send: fix sending link commands for existing file paths
>
> Same here.
>
> > +#
> > +. ./common/preamble
> > +_begin_fstest auto quick send
> > +
> > +tmp=`mktemp -d`
>
> Overriding $tmp, which is set by the fstests framework is not a good idea.
> It's expected to be a file and not a directory.
>
> If you need a directory to store temporary files, you can use the test device.
> Take a look at btrfs/241 for example.
>
> > +
> > +# Override the default cleanup function.
> > +_cleanup()
> > +{
> > +       rm -rf $tmp
>
> Then here leave the standard "rm -f $tmp.*" followed by a rm -rf of
> the temporary directory in the test mount point.
>
> > +}
> > +
> > +# Import common functions.
> > +. ./common/filter
> > +
> > +# real QA test starts here
> > +_supported_fs btrfs
> > +_require_test
> > +_require_scratch
> > +
> > +_scratch_mkfs > /dev/null 2>&1
> > +_scratch_mount
> > +
> > +_run_btrfs_util_prog subvolume create $SCRATCH_MNT/vol
> > +
> > +# create a file and 2000 hard links to the same inode
> > +touch $SCRATCH_MNT/vol/foo
> > +for i in {1..2000}; do
> > +       link $SCRATCH_MNT/vol/foo $SCRATCH_MNT/vol/$i
> > +done
> > +
> > +# take a snapshot for a parent snapshot
>
> "take a snapshot for a full send operation"
>
> > +_run_btrfs_util_prog subvolume snapshot -r $SCRATCH_MNT/vol $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
> > +
> > +# take another one for a send snapshot
>
> "take a snapshot for an incremental send operation"
>
> > +_run_btrfs_util_prog subvolume snapshot -r $SCRATCH_MNT/vol $SCRATCH_MNT/snap2
> > +
> > +mkdir $SCRATCH_MNT/receive_dir
> > +_run_btrfs_util_prog send -p $SCRATCH_MNT/snap1 -f $tmp/diff.snap \
> > +       $SCRATCH_MNT/snap2
> > +_run_btrfs_util_prog receive -f $tmp/diff.snap $SCRATCH_MNT/receive_dir
> > +_scratch_unmount
>
> Btw, there's no need to call _scratch_unmount, the fstests framework
> automatically does that when the test finishes.
>
> So, this tests that the send and receive commands do not fail.
>
> However it does not test that they produce the correct results: that
> after the receive we have the file with the same hardlinks, mtime,
> ctime, etc, as in the original subvolume.
> For send/receive tests (well, most tests actually), we always want to
> verify that the operations produce the expected results, not just that
> they don't fail with an error.
>
> The fssum utility can be used here to verify that, and we use it in
> many send/receive tests like btrfs/241 for example.
>
> Thanks for doing this!
>
> > +
> > +echo "Silence is golden"
> > +status=0 ; exit
> > diff --git a/tests/btrfs/272.out b/tests/btrfs/272.out
> > new file mode 100644
> > index 00000000..c18563ad
> > --- /dev/null
> > +++ b/tests/btrfs/272.out
> > @@ -0,0 +1,2 @@
> > +QA output created by 272
> > +Silence is golden
> > --
> > 2.37.1
> >