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

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

[Filipe Manana]

On Mon, Aug 22, 2022 at 08:49:32AM +0800, bingjingc wrote:
> From: BingJing Chang <bingjingc@synology.com>
> 
> 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@synology.com>
> ---
>  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@suse.com>

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
> 

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

[bingjing chang]

Hi Zorro,

Thank you for your review comments.

Zorro Lang <zlang@redhat.com> 於 2022年8月23日 週二 晚上8:52寫道：
>
> On Mon, Aug 22, 2022 at 08:49:32AM +0800, bingjingc wrote:
> > From: BingJing Chang <bingjingc@synology.com>
> >
> > 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@synology.com>
> > ---
> >  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")
>
> As Filipe said, welcome to use _fixed_by_kernel_commit and others related
> functions for a known regression test.

Thank you both for telling me this.

>
> > +#
> > +. ./common/preamble
> > +_begin_fstest auto quick send
> > +
> > +# Override the default cleanup function.
> > +_cleanup()
> > +{
> > +     cd /
> > +     rm -fr $send_files_dir
>
> Will things in $send_files_dir take much disk space? If not, we can keep it
> in $TEST_DIR (then remove this specific _cleanup), generally we don't need
> to keep $TEST_DIR clean (except it will affect later testing).
>

No, in this test, it only generates many hard links to an empty file.
It takes only a little space to store the btrfs send stream.
I'm not familiar with the testing framework, so I added the _cleanup()
as other tests. It makes sense to me, so I will remove it in patch v3.

> > +     rm -f $tmp.*
> > +}
> > +
> > +# Import common functions.
> > +. ./common/filter
> > +
> > +# real QA test starts here
> > +_supported_fs btrfs
> > +_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
>
> What kind of stdout/stderr you need to fill out? Generally we can output it
> into .full file for later debug. Anyway, I can't say this's wrong.
>
No. I should have outputted it to $seqres.ful.
Thanks for reminding me.

> > +_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
> > +_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
> > +
> > +$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
> >
>

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

[bingjing chang]

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@kernel.org> 於 2022年8月22日 週一 晚上7:24寫道：
>
> On Mon, Aug 22, 2022 at 08:49:32AM +0800, bingjingc wrote:
> > From: BingJing Chang <bingjingc@synology.com>
> >
> > 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@synology.com>
> > ---
> >  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@suse.com>
>
> 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
> >