diff mbox series

fstests: btrfs/248: test if btrfs receive can handle clone command on inodes with different NODATASUM flags

Message ID 20210929004446.12654-1-wqu@suse.com (mailing list archive)
State New, archived
Headers show
Series fstests: btrfs/248: test if btrfs receive can handle clone command on inodes with different NODATASUM flags | expand

Commit Message

Qu Wenruo Sept. 29, 2021, 12:44 a.m. UTC 
The planned fix is titled "btrfs-progs: receive: fallback to buffered
copy if clone failed".

The test case itself will create two send streams, and the 2nd stream is
an incremental stream with a clone command in it.

Using different mount options we are able to create a situation where
clone source and destination have different NODATASUM flags, which is
prohibited inside btrfs.

The planned fix will make btrfs receive to fall back to buffered write
to copy the data from the source file.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 tests/btrfs/248     | 74 +++++++++++++++++++++++++++++++++++++++++++++
 tests/btrfs/248.out |  2 ++
 2 files changed, 76 insertions(+)
 create mode 100755 tests/btrfs/248
 create mode 100644 tests/btrfs/248.out

Comments

Nikolay Borisov Sept. 29, 2021, 6:45 a.m. UTC | #1 
On 29.09.21 г. 3:44, Qu Wenruo wrote:
> The planned fix is titled "btrfs-progs: receive: fallback to buffered
> copy if clone failed".
> 
> The test case itself will create two send streams, and the 2nd stream is
> an incremental stream with a clone command in it.
> 
> Using different mount options we are able to create a situation where
> clone source and destination have different NODATASUM flags, which is
> prohibited inside btrfs.
> 
> The planned fix will make btrfs receive to fall back to buffered write
> to copy the data from the source file.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

If this is fixed by btrfs-progs fix shouldn't the test be part of
btrfs-progs and not the kernel test suite?
Qu Wenruo Sept. 29, 2021, 6:59 a.m. UTC | #2 
On 2021/9/29 14:45, Nikolay Borisov wrote:
>
>
> On 29.09.21 г. 3:44, Qu Wenruo wrote:
>> The planned fix is titled "btrfs-progs: receive: fallback to buffered
>> copy if clone failed".
>>
>> The test case itself will create two send streams, and the 2nd stream is
>> an incremental stream with a clone command in it.
>>
>> Using different mount options we are able to create a situation where
>> clone source and destination have different NODATASUM flags, which is
>> prohibited inside btrfs.
>>
>> The planned fix will make btrfs receive to fall back to buffered write
>> to copy the data from the source file.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>
> If this is fixed by btrfs-progs fix shouldn't the test be part of
> btrfs-progs and not the kernel test suite?
>

Makes sense.

Would move it to btrfs-progs misc-tests, which can be more flex there.

THanks,
Qu
Filipe Manana Sept. 29, 2021, 9:14 a.m. UTC | #3 
On Wed, Sep 29, 2021 at 1:45 AM Qu Wenruo <wqu@suse.com> wrote:
>
> The planned fix is titled "btrfs-progs: receive: fallback to buffered
> copy if clone failed".
>
> The test case itself will create two send streams, and the 2nd stream is
> an incremental stream with a clone command in it.
>
> Using different mount options we are able to create a situation where
> clone source and destination have different NODATASUM flags, which is
> prohibited inside btrfs.
>
> The planned fix will make btrfs receive to fall back to buffered write
> to copy the data from the source file.
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  tests/btrfs/248     | 74 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/btrfs/248.out |  2 ++
>  2 files changed, 76 insertions(+)
>  create mode 100755 tests/btrfs/248
>  create mode 100644 tests/btrfs/248.out
>
> diff --git a/tests/btrfs/248 b/tests/btrfs/248
> new file mode 100755
> index 00000000..964d3e85
> --- /dev/null
> +++ b/tests/btrfs/248
> @@ -0,0 +1,74 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2021 SUSE Linux Products GmbH.  All Rights Reserved.
> +#
> +# FS QA Test 248
> +#
> +# Make sure btrfs receive can still handle clone stream even if the source

s/clone stream/clone operations/

> +# and destination has different NODATASUM flags

s/has/have/

> +#
> +. ./common/preamble
> +_begin_fstest quick send
> +
> +# Override the default cleanup function.
> +_cleanup()
> +{
> +       cd /
> +       rm -r -f $tmp.*
> +}
> +
> +# Import common functions.
> +# . ./common/filter
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs btrfs
> +_require_scratch
> +
> +_scratch_mkfs >> $seqres.full 2>&1
> +_scratch_mount -o datasum
> +
> +# Create the initial subvolume with a file
> +$BTRFS_UTIL_PROG subvolume create $SCRATCH_MNT/parent >> $seqres.full
> +$XFS_IO_PROG -f -c "pwrite 0 1m" $SCRATCH_MNT/parent/source \
> +       > /dev/null
> +sync

There's no need to call sync.

> +$BTRFS_UTIL_PROG prop set $SCRATCH_MNT/parent ro true
> +$BTRFS_UTIL_PROG send -q $SCRATCH_MNT/parent -f $tmp.parent_stream
> +_scratch_unmount
> +_scratch_mkfs >> $seqres.full 2>&1
> +_scratch_mount -o datasum
> +
> +# Then create a new subvolume with cloned file from above send stream
> +$BTRFS_UTIL_PROG receive -q -f $tmp.parent_stream $SCRATCH_MNT
> +$BTRFS_UTIL_PROG subvolume snapshot $SCRATCH_MNT/parent $SCRATCH_MNT/dest \
> +       >> $seqres.full
> +$XFS_IO_PROG -f -c "reflink $SCRATCH_MNT/parent/source 4k 0 128K" \

This will fail on a 64K sector size, so always use offsets and lengths
that are multiples of 64K, so that the test can run on all possible
sector sizes.

> +       $SCRATCH_MNT/dest/new > /dev/null
> +$BTRFS_UTIL_PROG prop set $SCRATCH_MNT/dest ro true
> +$BTRFS_UTIL_PROG send -q $SCRATCH_MNT/dest -p $SCRATCH_MNT/parent \
> +       -f $tmp.clone_stream

Man, this is so much more complicated than necessary.
Switching from RW to RO, having to create and mount another filesystem
to create the incremental stream, etc.

Why didn't you follow the simple steps that most of the other tests follow?

Example:

1) mkfs
2) mount with -o datacow or -o datasum
3) create file $SCRATCH_MNT/foo with some data
4) create a RO snapshot of the default subvolume as  $SCRATCH_MNT/snap1
5) clone  $SCRATCH_MNT/foo  into  $SCRATCH_MNT/bar for example
6) create another RO snapshot of the default subvolume as  $SCRATCH_MNT/snap2
7) do a full send of $SCRATCH_MNT/snap1 and save the stream into a file
8) do an incremental send of $SCRATCH_MNT/snap2 using
$SCRATCH_MNT/snap1 as the parent and save the stream into another file

See, no need to mkfs and mount between creating the streams, and
neither the need to switch subvolumes or snapshots from RW to RO.


> +
> +_scratch_unmount
> +_scratch_mkfs >> $seqres.full 2>&1
> +_scratch_mount -o datasum
> +
> +# Now try to receive both streams
> +$BTRFS_UTIL_PROG receive -q -f $tmp.parent_stream $SCRATCH_MNT/
> +
> +# Remount to NODATASUM, so that the 2nd stream will get all its inodes to have
> +# NODATASUM flags due to mount option
> +_scratch_remount nodatasum
> +
> +# Patched receive may warn about the clone failure, so here we redirect all
> +# output
> +$BTRFS_UTIL_PROG receive -q -f $tmp.clone_stream $SCRATCH_MNT/ \
> +       >> $seqres.full 2>&1
> +
> +# We check the destination file's csum to verify if the clone is done properly
> +_md5_checksum $SCRATCH_MNT/dest/new
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/btrfs/248.out b/tests/btrfs/248.out
> new file mode 100644
> index 00000000..b49cfad7
> --- /dev/null
> +++ b/tests/btrfs/248.out
> @@ -0,0 +1,2 @@
> +QA output created by 248
> +d48858312a922db7eb86377f638dbc9f

This is neither very friendly to debug nor easy to validate.

I suggest either:

1) Print the checksum on the original filesystem too, so that we can compare:

echo "checksum in the source fs: $(_md5_checksum $SCRATCH_MNT/...)"
(...)
# Should match the checksum in the source fs.
echo "checksum in the destination fs: $(_md5_checksum $SCRATCH_MNT/...)"

2) Or just dump the file with 'od -A d -t x1' or hexdump.

As for having the test in fstests or btrfs-progs, I won't mind either option.

As Dave Chinner once mentioned in some threads in a distant past,
fstests is not exclusive for testing kernel only changes - it's a
testing framework for filesystems, which includes kernel and tools.
I would prefer if we could have all tests inside the same test suite,
but we already have things spread between fstests and btrfs-progs, and
just noticed that btrfs-progs has now at least 1 test case to cover a
kernel-only fix (misc-tests/041-subvolume-delete-during-send/test.sh).

Thanks.

> --
> 2.33.0
>
Qu Wenruo Sept. 29, 2021, 9:23 a.m. UTC | #4 
On 2021/9/29 17:14, Filipe Manana wrote:
> On Wed, Sep 29, 2021 at 1:45 AM Qu Wenruo <wqu@suse.com> wrote:
>>
>> The planned fix is titled "btrfs-progs: receive: fallback to buffered
>> copy if clone failed".
>>
>> The test case itself will create two send streams, and the 2nd stream is
>> an incremental stream with a clone command in it.
>>
>> Using different mount options we are able to create a situation where
>> clone source and destination have different NODATASUM flags, which is
>> prohibited inside btrfs.
>>
>> The planned fix will make btrfs receive to fall back to buffered write
>> to copy the data from the source file.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   tests/btrfs/248     | 74 +++++++++++++++++++++++++++++++++++++++++++++
>>   tests/btrfs/248.out |  2 ++
>>   2 files changed, 76 insertions(+)
>>   create mode 100755 tests/btrfs/248
>>   create mode 100644 tests/btrfs/248.out
>>
>> diff --git a/tests/btrfs/248 b/tests/btrfs/248
>> new file mode 100755
>> index 00000000..964d3e85
>> --- /dev/null
>> +++ b/tests/btrfs/248
>> @@ -0,0 +1,74 @@
>> +#! /bin/bash
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (c) 2021 SUSE Linux Products GmbH.  All Rights Reserved.
>> +#
>> +# FS QA Test 248
>> +#
>> +# Make sure btrfs receive can still handle clone stream even if the source
> 
> s/clone stream/clone operations/
> 
>> +# and destination has different NODATASUM flags
> 
> s/has/have/
> 
>> +#
>> +. ./common/preamble
>> +_begin_fstest quick send
>> +
>> +# Override the default cleanup function.
>> +_cleanup()
>> +{
>> +       cd /
>> +       rm -r -f $tmp.*
>> +}
>> +
>> +# Import common functions.
>> +# . ./common/filter
>> +
>> +# real QA test starts here
>> +
>> +# Modify as appropriate.
>> +_supported_fs btrfs
>> +_require_scratch
>> +
>> +_scratch_mkfs >> $seqres.full 2>&1
>> +_scratch_mount -o datasum
>> +
>> +# Create the initial subvolume with a file
>> +$BTRFS_UTIL_PROG subvolume create $SCRATCH_MNT/parent >> $seqres.full
>> +$XFS_IO_PROG -f -c "pwrite 0 1m" $SCRATCH_MNT/parent/source \
>> +       > /dev/null
>> +sync
> 
> There's no need to call sync.
> 
>> +$BTRFS_UTIL_PROG prop set $SCRATCH_MNT/parent ro true
>> +$BTRFS_UTIL_PROG send -q $SCRATCH_MNT/parent -f $tmp.parent_stream
>> +_scratch_unmount
>> +_scratch_mkfs >> $seqres.full 2>&1
>> +_scratch_mount -o datasum
>> +
>> +# Then create a new subvolume with cloned file from above send stream
>> +$BTRFS_UTIL_PROG receive -q -f $tmp.parent_stream $SCRATCH_MNT
>> +$BTRFS_UTIL_PROG subvolume snapshot $SCRATCH_MNT/parent $SCRATCH_MNT/dest \
>> +       >> $seqres.full
>> +$XFS_IO_PROG -f -c "reflink $SCRATCH_MNT/parent/source 4k 0 128K" \
> 
> This will fail on a 64K sector size, so always use offsets and lengths
> that are multiples of 64K, so that the test can run on all possible
> sector sizes.
> 
>> +       $SCRATCH_MNT/dest/new > /dev/null
>> +$BTRFS_UTIL_PROG prop set $SCRATCH_MNT/dest ro true
>> +$BTRFS_UTIL_PROG send -q $SCRATCH_MNT/dest -p $SCRATCH_MNT/parent \
>> +       -f $tmp.clone_stream
> 
> Man, this is so much more complicated than necessary.
> Switching from RW to RO, having to create and mount another filesystem
> to create the incremental stream, etc.
> 
> Why didn't you follow the simple steps that most of the other tests follow?
> 
> Example:
> 
> 1) mkfs
> 2) mount with -o datacow or -o datasum
> 3) create file $SCRATCH_MNT/foo with some data
> 4) create a RO snapshot of the default subvolume as  $SCRATCH_MNT/snap1
> 5) clone  $SCRATCH_MNT/foo  into  $SCRATCH_MNT/bar for example
> 6) create another RO snapshot of the default subvolume as  $SCRATCH_MNT/snap2
> 7) do a full send of $SCRATCH_MNT/snap1 and save the stream into a file
> 8) do an incremental send of $SCRATCH_MNT/snap2 using
> $SCRATCH_MNT/snap1 as the parent and save the stream into another file

Thanks a lot, this is exactly what I want.

> 
> See, no need to mkfs and mount between creating the streams, and
> neither the need to switch subvolumes or snapshots from RW to RO.
Indeed way much better.

> 
> 
>> +
>> +_scratch_unmount
>> +_scratch_mkfs >> $seqres.full 2>&1
>> +_scratch_mount -o datasum
>> +
>> +# Now try to receive both streams
>> +$BTRFS_UTIL_PROG receive -q -f $tmp.parent_stream $SCRATCH_MNT/
>> +
>> +# Remount to NODATASUM, so that the 2nd stream will get all its inodes to have
>> +# NODATASUM flags due to mount option
>> +_scratch_remount nodatasum
>> +
>> +# Patched receive may warn about the clone failure, so here we redirect all
>> +# output
>> +$BTRFS_UTIL_PROG receive -q -f $tmp.clone_stream $SCRATCH_MNT/ \
>> +       >> $seqres.full 2>&1
>> +
>> +# We check the destination file's csum to verify if the clone is done properly
>> +_md5_checksum $SCRATCH_MNT/dest/new
>> +
>> +# success, all done
>> +status=0
>> +exit
>> diff --git a/tests/btrfs/248.out b/tests/btrfs/248.out
>> new file mode 100644
>> index 00000000..b49cfad7
>> --- /dev/null
>> +++ b/tests/btrfs/248.out
>> @@ -0,0 +1,2 @@
>> +QA output created by 248
>> +d48858312a922db7eb86377f638dbc9f
> 
> This is neither very friendly to debug nor easy to validate.
> 
> I suggest either:
> 
> 1) Print the checksum on the original filesystem too, so that we can compare:
> 
> echo "checksum in the source fs: $(_md5_checksum $SCRATCH_MNT/...)"
> (...)
> # Should match the checksum in the source fs.
> echo "checksum in the destination fs: $(_md5_checksum $SCRATCH_MNT/...)"
> 
> 2) Or just dump the file with 'od -A d -t x1' or hexdump.
> 
> As for having the test in fstests or btrfs-progs, I won't mind either option.

For btrfs-progs testing part, the benefit is we don't need to bother 
generating the stream, as btrfs-progs is more tolerant for binary dumps.

And as you have already seen, I really struggle at creating the send stream.

(And no need to wrap every external program makes me feel more at east).

I'm totally fine to have both or either.

Thanks,
Qu

> 
> As Dave Chinner once mentioned in some threads in a distant past,
> fstests is not exclusive for testing kernel only changes - it's a
> testing framework for filesystems, which includes kernel and tools.
> I would prefer if we could have all tests inside the same test suite,
> but we already have things spread between fstests and btrfs-progs, and
> just noticed that btrfs-progs has now at least 1 test case to cover a
> kernel-only fix (misc-tests/041-subvolume-delete-during-send/test.sh).
> 
> Thanks.
> 
>> --
>> 2.33.0
>>
> 
>
diff mbox series

Patch

diff --git a/tests/btrfs/248 b/tests/btrfs/248
new file mode 100755
index 00000000..964d3e85
--- /dev/null
+++ b/tests/btrfs/248
@@ -0,0 +1,74 @@ 
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2021 SUSE Linux Products GmbH.  All Rights Reserved.
+#
+# FS QA Test 248
+#
+# Make sure btrfs receive can still handle clone stream even if the source
+# and destination has different NODATASUM flags
+#
+. ./common/preamble
+_begin_fstest quick send
+
+# Override the default cleanup function.
+_cleanup()
+{
+	cd /
+	rm -r -f $tmp.*
+}
+
+# Import common functions.
+# . ./common/filter
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs btrfs
+_require_scratch
+
+_scratch_mkfs >> $seqres.full 2>&1
+_scratch_mount -o datasum
+
+# Create the initial subvolume with a file
+$BTRFS_UTIL_PROG subvolume create $SCRATCH_MNT/parent >> $seqres.full
+$XFS_IO_PROG -f -c "pwrite 0 1m" $SCRATCH_MNT/parent/source \
+	> /dev/null
+sync
+$BTRFS_UTIL_PROG prop set $SCRATCH_MNT/parent ro true
+$BTRFS_UTIL_PROG send -q $SCRATCH_MNT/parent -f $tmp.parent_stream
+_scratch_unmount
+_scratch_mkfs >> $seqres.full 2>&1
+_scratch_mount -o datasum
+
+# Then create a new subvolume with cloned file from above send stream
+$BTRFS_UTIL_PROG receive -q -f $tmp.parent_stream $SCRATCH_MNT
+$BTRFS_UTIL_PROG subvolume snapshot $SCRATCH_MNT/parent $SCRATCH_MNT/dest \
+	>> $seqres.full
+$XFS_IO_PROG -f -c "reflink $SCRATCH_MNT/parent/source 4k 0 128K" \
+	$SCRATCH_MNT/dest/new > /dev/null
+$BTRFS_UTIL_PROG prop set $SCRATCH_MNT/dest ro true
+$BTRFS_UTIL_PROG send -q $SCRATCH_MNT/dest -p $SCRATCH_MNT/parent \
+	-f $tmp.clone_stream
+
+_scratch_unmount
+_scratch_mkfs >> $seqres.full 2>&1
+_scratch_mount -o datasum
+
+# Now try to receive both streams
+$BTRFS_UTIL_PROG receive -q -f $tmp.parent_stream $SCRATCH_MNT/
+
+# Remount to NODATASUM, so that the 2nd stream will get all its inodes to have
+# NODATASUM flags due to mount option
+_scratch_remount nodatasum
+
+# Patched receive may warn about the clone failure, so here we redirect all
+# output
+$BTRFS_UTIL_PROG receive -q -f $tmp.clone_stream $SCRATCH_MNT/ \
+	>> $seqres.full 2>&1
+
+# We check the destination file's csum to verify if the clone is done properly
+_md5_checksum $SCRATCH_MNT/dest/new
+
+# success, all done
+status=0
+exit
diff --git a/tests/btrfs/248.out b/tests/btrfs/248.out
new file mode 100644
index 00000000..b49cfad7
--- /dev/null
+++ b/tests/btrfs/248.out
@@ -0,0 +1,2 @@ 
+QA output created by 248
+d48858312a922db7eb86377f638dbc9f