diff mbox series

fstests: btrfs: test incremental send for changed reference paths

Message ID 20220806143436.3501-1-bingjingc@synology.com (mailing list archive)
State New, archived
Headers show
Series fstests: btrfs: test incremental send for changed reference paths | expand

Commit Message

bingjingc Aug. 6, 2022, 2:34 p.m. UTC 
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:

  "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

Comments

Filipe Manana Aug. 8, 2022, 9:30 a.m. UTC | #1 
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
>
bingjing chang Aug. 22, 2022, 12:50 a.m. UTC | #2 
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
> >
diff mbox series

Patch

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.
+#
+# This issue is fixed by the following linux kernel btrfs patch:
+#
+#   btrfs: send: fix sending link commands for existing file paths
+#
+. ./common/preamble
+_begin_fstest auto quick send
+
+tmp=`mktemp -d`
+
+# Override the default cleanup function.
+_cleanup()
+{
+	rm -rf $tmp
+}
+
+# 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
+_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
+_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
+
+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