* [PATCH] fstests: btrfs/248: test if btrfs receive can handle clone command on inodes with different NODATASUM flags @ 2021-09-29 0:44 Qu Wenruo 2021-09-29 6:45 ` Nikolay Borisov 2021-09-29 9:14 ` Filipe Manana 0 siblings, 2 replies; 5+ messages in thread From: Qu Wenruo @ 2021-09-29 0:44 UTC (permalink / raw) To: fstests; +Cc: linux-btrfs 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 +# 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 -- 2.33.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] fstests: btrfs/248: test if btrfs receive can handle clone command on inodes with different NODATASUM flags 2021-09-29 0:44 [PATCH] fstests: btrfs/248: test if btrfs receive can handle clone command on inodes with different NODATASUM flags Qu Wenruo @ 2021-09-29 6:45 ` Nikolay Borisov 2021-09-29 6:59 ` Qu Wenruo 2021-09-29 9:14 ` Filipe Manana 1 sibling, 1 reply; 5+ messages in thread From: Nikolay Borisov @ 2021-09-29 6:45 UTC (permalink / raw) To: Qu Wenruo, fstests; +Cc: linux-btrfs 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? ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] fstests: btrfs/248: test if btrfs receive can handle clone command on inodes with different NODATASUM flags 2021-09-29 6:45 ` Nikolay Borisov @ 2021-09-29 6:59 ` Qu Wenruo 0 siblings, 0 replies; 5+ messages in thread From: Qu Wenruo @ 2021-09-29 6:59 UTC (permalink / raw) To: Nikolay Borisov, Qu Wenruo, fstests; +Cc: linux-btrfs 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] fstests: btrfs/248: test if btrfs receive can handle clone command on inodes with different NODATASUM flags 2021-09-29 0:44 [PATCH] fstests: btrfs/248: test if btrfs receive can handle clone command on inodes with different NODATASUM flags Qu Wenruo 2021-09-29 6:45 ` Nikolay Borisov @ 2021-09-29 9:14 ` Filipe Manana 2021-09-29 9:23 ` Qu Wenruo 1 sibling, 1 reply; 5+ messages in thread From: Filipe Manana @ 2021-09-29 9:14 UTC (permalink / raw) To: Qu Wenruo; +Cc: fstests, linux-btrfs 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 > -- Filipe David Manana, “Whether you think you can, or you think you can't — you're right.” ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] fstests: btrfs/248: test if btrfs receive can handle clone command on inodes with different NODATASUM flags 2021-09-29 9:14 ` Filipe Manana @ 2021-09-29 9:23 ` Qu Wenruo 0 siblings, 0 replies; 5+ messages in thread From: Qu Wenruo @ 2021-09-29 9:23 UTC (permalink / raw) To: fdmanana; +Cc: fstests, linux-btrfs 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 >> > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-09-29 9:23 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-09-29 0:44 [PATCH] fstests: btrfs/248: test if btrfs receive can handle clone command on inodes with different NODATASUM flags Qu Wenruo 2021-09-29 6:45 ` Nikolay Borisov 2021-09-29 6:59 ` Qu Wenruo 2021-09-29 9:14 ` Filipe Manana 2021-09-29 9:23 ` Qu Wenruo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox