| Message ID | 20210819131456.304721-1-nborisov@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: Add test for rename exchange behavior between subvolumes | expand |
On Thu, Aug 19, 2021 at 2:17 PM Nikolay Borisov <nborisov@suse.com> wrote: > > Signed-off-by: Nikolay Borisov <nborisov@suse.com> At the very least the change log could mention which patch / commit motivated this test. > --- > tests/btrfs/246 | 46 +++++++++++++++++++++++++++++++++++++++++++++ > tests/btrfs/246.out | 27 ++++++++++++++++++++++++++ > 2 files changed, 73 insertions(+) > create mode 100755 tests/btrfs/246 > create mode 100644 tests/btrfs/246.out > > diff --git a/tests/btrfs/246 b/tests/btrfs/246 > new file mode 100755 > index 000000000000..0934932d1f22 > --- /dev/null > +++ b/tests/btrfs/246 > @@ -0,0 +1,46 @@ > +#! /bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (c) 2021 SUSE Linux Products GmbH. All Rights Reserved. > +# > +# FS QA Test 246 > +# > +# Tests rename exchange behavior across subvolumes > +# > +. ./common/preamble > +_begin_fstest auto quick rename > + > +# Import common functions. > + . ./common/renameat2 > + > +# real QA test starts here > + > +# Modify as appropriate. > +_supported_fs btrfs > +_require_renameat2 exchange > +_require_scratch > + > +_scratch_mkfs >> $seqres.full 2>&1 > +_scratch_mount > + > +# Create 2 subvols to use as parents for the rename ops > +$BTRFS_UTIL_PROG subvolume create $SCRATCH_MNT/subvol1 1>/dev/null > +$BTRFS_UTIL_PROG subvolume create $SCRATCH_MNT/subvol2 1>/dev/null > + > +# _rename_tests_source_dest internally expects the flags variable to contain > +# specific options to rename syscall. Ensure cross subvol ops are forbidden > +flags="-x" > +_rename_tests_source_dest $SCRATCH_MNT/subvol1/src $SCRATCH_MNT/subvol2/dst "cross-subvol" > + > +# Prepare a subvolume and a directory whose parents are different subvolumes > +$BTRFS_UTIL_PROG subvolume create $SCRATCH_MNT/subvol1/sub-subvol 1>/dev/null > +mkdir $SCRATCH_MNT/subvol2/dir > + > +# Ensure exchanging a subvol with a dir when both parents are different fails > +$here/src/renameat2 -x $SCRATCH_MNT/subvol1/sub-subvol $SCRATCH_MNT/subvol2/dir > + > +# force transaction commit which runs the tree checker > +sync There's no need to invoke sync, when the test unmounts the device, the tree checker runs and fstests notices and complains about the error message from the tree checker. Plus the mismatch between the produced output and the golden output, is enough to make the test fail on an unpatched kernel. Finally, this would also be a good opportunity to test regular renames with subvolumes too, as we had bugs and regressions in the past like in commit 4871c1588f92c6c13f4713a7009f25f217055807 ("Btrfs: use right root when checking for hash collision "), and never got any test cases for them. Thanks. > + > +# success, all done > +status=0 > +exit > diff --git a/tests/btrfs/246.out b/tests/btrfs/246.out > new file mode 100644 > index 000000000000..d50dc28b1b40 > --- /dev/null > +++ b/tests/btrfs/246.out > @@ -0,0 +1,27 @@ > +QA output created by 246 > +cross-subvol none/none -> No such file or directory > +cross-subvol none/regu -> No such file or directory > +cross-subvol none/symb -> No such file or directory > +cross-subvol none/dire -> No such file or directory > +cross-subvol none/tree -> No such file or directory > +cross-subvol regu/none -> No such file or directory > +cross-subvol regu/regu -> Invalid cross-device link > +cross-subvol regu/symb -> Invalid cross-device link > +cross-subvol regu/dire -> Invalid cross-device link > +cross-subvol regu/tree -> Invalid cross-device link > +cross-subvol symb/none -> No such file or directory > +cross-subvol symb/regu -> Invalid cross-device link > +cross-subvol symb/symb -> Invalid cross-device link > +cross-subvol symb/dire -> Invalid cross-device link > +cross-subvol symb/tree -> Invalid cross-device link > +cross-subvol dire/none -> No such file or directory > +cross-subvol dire/regu -> Invalid cross-device link > +cross-subvol dire/symb -> Invalid cross-device link > +cross-subvol dire/dire -> Invalid cross-device link > +cross-subvol dire/tree -> Invalid cross-device link > +cross-subvol tree/none -> No such file or directory > +cross-subvol tree/regu -> Invalid cross-device link > +cross-subvol tree/symb -> Invalid cross-device link > +cross-subvol tree/dire -> Invalid cross-device link > +cross-subvol tree/tree -> Invalid cross-device link > +Invalid cross-device link > -- > 2.17.1 >
On Thu, Aug 19, 2021 at 04:14:56PM +0300, Nikolay Borisov wrote: > Signed-off-by: Nikolay Borisov <nborisov@suse.com> I noticed that test currently fails with v5.15-rc6 kernel as cross-subvol tree/symb -> Invalid cross-device link cross-subvol tree/dire -> Invalid cross-device link cross-subvol tree/tree -> Invalid cross-device link -Invalid cross-device link--- So is there any background info about this test? Is it motivated by a known bug? If so is there a proposed fix available? Some descriptions would be good in commit log. > tests/btrfs/246 | 46 +++++++++++++++++++++++++++++++++++++++++++++ > tests/btrfs/246.out | 27 ++++++++++++++++++++++++++ > 2 files changed, 73 insertions(+) > create mode 100755 tests/btrfs/246 > create mode 100644 tests/btrfs/246.out > > diff --git a/tests/btrfs/246 b/tests/btrfs/246 > new file mode 100755 > index 000000000000..0934932d1f22 > --- /dev/null > +++ b/tests/btrfs/246 > @@ -0,0 +1,46 @@ > +#! /bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (c) 2021 SUSE Linux Products GmbH. All Rights Reserved. > +# > +# FS QA Test 246 > +# > +# Tests rename exchange behavior across subvolumes Trailing white space in above line > +# > +. ./common/preamble > +_begin_fstest auto quick rename Should be in 'subvol' group as well. > + > +# Import common functions. > + . ./common/renameat2 > + > +# real QA test starts here > + > +# Modify as appropriate. > +_supported_fs btrfs > +_require_renameat2 exchange > +_require_scratch > + > +_scratch_mkfs >> $seqres.full 2>&1 > +_scratch_mount > + > +# Create 2 subvols to use as parents for the rename ops > +$BTRFS_UTIL_PROG subvolume create $SCRATCH_MNT/subvol1 1>/dev/null > +$BTRFS_UTIL_PROG subvolume create $SCRATCH_MNT/subvol2 1>/dev/null The "1" in "1>/dev/null" could be dropped. > + > +# _rename_tests_source_dest internally expects the flags variable to contain > +# specific options to rename syscall. Ensure cross subvol ops are forbidden > +flags="-x" > +_rename_tests_source_dest $SCRATCH_MNT/subvol1/src $SCRATCH_MNT/subvol2/dst "cross-subvol" I think _rename_tests_source_dest should be updated to take flags as arguments instead of inheriting $flags variable from caller. That could be done in a separate patch as preparation. > + > +# Prepare a subvolume and a directory whose parents are different subvolumes > +$BTRFS_UTIL_PROG subvolume create $SCRATCH_MNT/subvol1/sub-subvol 1>/dev/null > +mkdir $SCRATCH_MNT/subvol2/dir > + > +# Ensure exchanging a subvol with a dir when both parents are different fails > +$here/src/renameat2 -x $SCRATCH_MNT/subvol1/sub-subvol $SCRATCH_MNT/subvol2/dir > + > +# force transaction commit which runs the tree checker > +sync A global sync seems a bit heavy, does syncfs on scratch fs work? Or does umounting scratch dev work? If so we could depend on the test harness to umount scratch dev after each test. Thanks, Eryu > + > +# success, all done > +status=0 > +exit > diff --git a/tests/btrfs/246.out b/tests/btrfs/246.out > new file mode 100644 > index 000000000000..d50dc28b1b40 > --- /dev/null > +++ b/tests/btrfs/246.out > @@ -0,0 +1,27 @@ > +QA output created by 246 > +cross-subvol none/none -> No such file or directory > +cross-subvol none/regu -> No such file or directory > +cross-subvol none/symb -> No such file or directory > +cross-subvol none/dire -> No such file or directory > +cross-subvol none/tree -> No such file or directory > +cross-subvol regu/none -> No such file or directory > +cross-subvol regu/regu -> Invalid cross-device link > +cross-subvol regu/symb -> Invalid cross-device link > +cross-subvol regu/dire -> Invalid cross-device link > +cross-subvol regu/tree -> Invalid cross-device link > +cross-subvol symb/none -> No such file or directory > +cross-subvol symb/regu -> Invalid cross-device link > +cross-subvol symb/symb -> Invalid cross-device link > +cross-subvol symb/dire -> Invalid cross-device link > +cross-subvol symb/tree -> Invalid cross-device link > +cross-subvol dire/none -> No such file or directory > +cross-subvol dire/regu -> Invalid cross-device link > +cross-subvol dire/symb -> Invalid cross-device link > +cross-subvol dire/dire -> Invalid cross-device link > +cross-subvol dire/tree -> Invalid cross-device link > +cross-subvol tree/none -> No such file or directory > +cross-subvol tree/regu -> Invalid cross-device link > +cross-subvol tree/symb -> Invalid cross-device link > +cross-subvol tree/dire -> Invalid cross-device link > +cross-subvol tree/tree -> Invalid cross-device link > +Invalid cross-device link > -- > 2.17.1
On 29.08.21 г. 17:04, Eryu Guan wrote: > On Thu, Aug 19, 2021 at 04:14:56PM +0300, Nikolay Borisov wrote: >> Signed-off-by: Nikolay Borisov <nborisov@suse.com> > > I noticed that test currently fails with v5.15-rc6 kernel as > > cross-subvol tree/symb -> Invalid cross-device link > cross-subvol tree/dire -> Invalid cross-device link > cross-subvol tree/tree -> Invalid cross-device link > -Invalid cross-device link--- > > So is there any background info about this test? Is it motivated by a > known bug? If so is there a proposed fix available? > > Some descriptions would be good in commit log. > >> tests/btrfs/246 | 46 +++++++++++++++++++++++++++++++++++++++++++++ >> tests/btrfs/246.out | 27 ++++++++++++++++++++++++++ >> 2 files changed, 73 insertions(+) >> create mode 100755 tests/btrfs/246 >> create mode 100644 tests/btrfs/246.out >> >> diff --git a/tests/btrfs/246 b/tests/btrfs/246 >> new file mode 100755 >> index 000000000000..0934932d1f22 >> --- /dev/null >> +++ b/tests/btrfs/246 >> @@ -0,0 +1,46 @@ >> +#! /bin/bash >> +# SPDX-License-Identifier: GPL-2.0 >> +# Copyright (c) 2021 SUSE Linux Products GmbH. All Rights Reserved. >> +# >> +# FS QA Test 246 >> +# >> +# Tests rename exchange behavior across subvolumes > > Trailing white space in above line > >> +# >> +. ./common/preamble >> +_begin_fstest auto quick rename > > Should be in 'subvol' group as well. > >> + >> +# Import common functions. >> + . ./common/renameat2 >> + >> +# real QA test starts here >> + >> +# Modify as appropriate. >> +_supported_fs btrfs >> +_require_renameat2 exchange >> +_require_scratch >> + >> +_scratch_mkfs >> $seqres.full 2>&1 >> +_scratch_mount >> + >> +# Create 2 subvols to use as parents for the rename ops >> +$BTRFS_UTIL_PROG subvolume create $SCRATCH_MNT/subvol1 1>/dev/null >> +$BTRFS_UTIL_PROG subvolume create $SCRATCH_MNT/subvol2 1>/dev/null > > The "1" in "1>/dev/null" could be dropped. Nope, that's intentional because I want to disregard the usual output of: Create subvolume '/media/scratch/subvol1' Yet in case an error occurs I want it to fail the test. > >> + >> +# _rename_tests_source_dest internally expects the flags variable to contain >> +# specific options to rename syscall. Ensure cross subvol ops are forbidden >> +flags="-x" >> +_rename_tests_source_dest $SCRATCH_MNT/subvol1/src $SCRATCH_MNT/subvol2/dst "cross-subvol" > > I think _rename_tests_source_dest should be updated to take flags as > arguments instead of inheriting $flags variable from caller. That could > be done in a separate patch as preparation. I agree, will implement this. <snip> >> +sync > > A global sync seems a bit heavy, does syncfs on scratch fs work? Or does > umounting scratch dev work? If so we could depend on the test harness to > umount scratch dev after each test. Yeah, the unmount at the end of the test is sufficient to trigger the failure. <snip>
<snip> > Finally, this would also be a good opportunity to test regular renames > with subvolumes too, as we had bugs and regressions in the past like > in commit 4871c1588f92c6c13f4713a7009f25f217055807 ("Btrfs: use right > root when checking for hash collision > "), and never got any test cases for them. What specific tests do you have in mind? Ordinary renames of files within a subvolume are already tested by merit of generic geneirc/02[345]. The test in the patch you cited is basically renaming a subvolume within the same subvolume, that's easy enough. <snip>
On Mon, Aug 30, 2021 at 8:18 AM Nikolay Borisov <nborisov@suse.com> wrote: > > > > <snip> > > Finally, this would also be a good opportunity to test regular renames > > with subvolumes too, as we had bugs and regressions in the past like > > in commit 4871c1588f92c6c13f4713a7009f25f217055807 ("Btrfs: use right > > root when checking for hash collision > > "), and never got any test cases for them. > > What specific tests do you have in mind? Ordinary renames of files > within a subvolume are already tested by merit of generic geneirc/02[345]. So besides the case mentioned in that patch's changelog (renaming a subvolume), checking that we can't rename an inode across subvolumes. Something like checking that: rename /mnt/subvol1/file /mnt/subvol2/file fails with -EXDEV. Thanks. > > The test in the patch you cited is basically renaming a subvolume within > the same subvolume, that's easy enough. > > <snip>
On 30.08.21 г. 13:56, Filipe Manana wrote: > On Mon, Aug 30, 2021 at 8:18 AM Nikolay Borisov <nborisov@suse.com> wrote: >> >> >> >> <snip> >>> Finally, this would also be a good opportunity to test regular renames >>> with subvolumes too, as we had bugs and regressions in the past like >>> in commit 4871c1588f92c6c13f4713a7009f25f217055807 ("Btrfs: use right >>> root when checking for hash collision >>> "), and never got any test cases for them. >> >> What specific tests do you have in mind? Ordinary renames of files >> within a subvolume are already tested by merit of generic geneirc/02[345]. > > So besides the case mentioned in that patch's changelog (renaming a > subvolume), checking that we can't rename an inode across subvolumes. > Something like checking that: > > rename /mnt/subvol1/file /mnt/subvol2/file > > fails with -EXDEV. But this is already checked by merit of using this line: _rename_tests_source_dest $SCRATCH_MNT/subvol1/src $SCRATCH_MNT/subvol2/dst "cross-subvol" "-x" it tests multiple combinations of regular/symbolic/directory/dev/null pairs. I.e : cross-subvol regu/regu -> Invalid cross-device link So this is already covered I'd say. Or you mean to test those combinations even without rename exchange, which would boil down to calling _rename_tests_source_dest without the -x flag. > > Thanks. > > >> >> The test in the patch you cited is basically renaming a subvolume within >> the same subvolume, that's easy enough. >> >> <snip> > > >
On Mon, Aug 30, 2021 at 12:08 PM Nikolay Borisov <nborisov@suse.com> wrote: > > > > On 30.08.21 г. 13:56, Filipe Manana wrote: > > On Mon, Aug 30, 2021 at 8:18 AM Nikolay Borisov <nborisov@suse.com> wrote: > >> > >> > >> > >> <snip> > >>> Finally, this would also be a good opportunity to test regular renames > >>> with subvolumes too, as we had bugs and regressions in the past like > >>> in commit 4871c1588f92c6c13f4713a7009f25f217055807 ("Btrfs: use right > >>> root when checking for hash collision > >>> "), and never got any test cases for them. > >> > >> What specific tests do you have in mind? Ordinary renames of files > >> within a subvolume are already tested by merit of generic geneirc/02[345]. > > > > So besides the case mentioned in that patch's changelog (renaming a > > subvolume), checking that we can't rename an inode across subvolumes. > > Something like checking that: > > > > rename /mnt/subvol1/file /mnt/subvol2/file > > > > fails with -EXDEV. > > But this is already checked by merit of using this line: > > _rename_tests_source_dest $SCRATCH_MNT/subvol1/src > $SCRATCH_MNT/subvol2/dst "cross-subvol" "-x" > > > it tests multiple combinations of regular/symbolic/directory/dev/null > pairs. I.e : > > cross-subvol regu/regu -> Invalid cross-device link > > > > So this is already covered I'd say. Or you mean to test those > combinations even without rename exchange, which would boil down to > calling _rename_tests_source_dest without the -x flag. Yes, without "-x" (that's what I meant by "regular renames"). > > > > > Thanks. > > > > > >> > >> The test in the patch you cited is basically renaming a subvolume within > >> the same subvolume, that's easy enough. > >> > >> <snip> > > > > > >
diff --git a/tests/btrfs/246 b/tests/btrfs/246 new file mode 100755 index 000000000000..0934932d1f22 --- /dev/null +++ b/tests/btrfs/246 @@ -0,0 +1,46 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2021 SUSE Linux Products GmbH. All Rights Reserved. +# +# FS QA Test 246 +# +# Tests rename exchange behavior across subvolumes +# +. ./common/preamble +_begin_fstest auto quick rename + +# Import common functions. + . ./common/renameat2 + +# real QA test starts here + +# Modify as appropriate. +_supported_fs btrfs +_require_renameat2 exchange +_require_scratch + +_scratch_mkfs >> $seqres.full 2>&1 +_scratch_mount + +# Create 2 subvols to use as parents for the rename ops +$BTRFS_UTIL_PROG subvolume create $SCRATCH_MNT/subvol1 1>/dev/null +$BTRFS_UTIL_PROG subvolume create $SCRATCH_MNT/subvol2 1>/dev/null + +# _rename_tests_source_dest internally expects the flags variable to contain +# specific options to rename syscall. Ensure cross subvol ops are forbidden +flags="-x" +_rename_tests_source_dest $SCRATCH_MNT/subvol1/src $SCRATCH_MNT/subvol2/dst "cross-subvol" + +# Prepare a subvolume and a directory whose parents are different subvolumes +$BTRFS_UTIL_PROG subvolume create $SCRATCH_MNT/subvol1/sub-subvol 1>/dev/null +mkdir $SCRATCH_MNT/subvol2/dir + +# Ensure exchanging a subvol with a dir when both parents are different fails +$here/src/renameat2 -x $SCRATCH_MNT/subvol1/sub-subvol $SCRATCH_MNT/subvol2/dir + +# force transaction commit which runs the tree checker +sync + +# success, all done +status=0 +exit diff --git a/tests/btrfs/246.out b/tests/btrfs/246.out new file mode 100644 index 000000000000..d50dc28b1b40 --- /dev/null +++ b/tests/btrfs/246.out @@ -0,0 +1,27 @@ +QA output created by 246 +cross-subvol none/none -> No such file or directory +cross-subvol none/regu -> No such file or directory +cross-subvol none/symb -> No such file or directory +cross-subvol none/dire -> No such file or directory +cross-subvol none/tree -> No such file or directory +cross-subvol regu/none -> No such file or directory +cross-subvol regu/regu -> Invalid cross-device link +cross-subvol regu/symb -> Invalid cross-device link +cross-subvol regu/dire -> Invalid cross-device link +cross-subvol regu/tree -> Invalid cross-device link +cross-subvol symb/none -> No such file or directory +cross-subvol symb/regu -> Invalid cross-device link +cross-subvol symb/symb -> Invalid cross-device link +cross-subvol symb/dire -> Invalid cross-device link +cross-subvol symb/tree -> Invalid cross-device link +cross-subvol dire/none -> No such file or directory +cross-subvol dire/regu -> Invalid cross-device link +cross-subvol dire/symb -> Invalid cross-device link +cross-subvol dire/dire -> Invalid cross-device link +cross-subvol dire/tree -> Invalid cross-device link +cross-subvol tree/none -> No such file or directory +cross-subvol tree/regu -> Invalid cross-device link +cross-subvol tree/symb -> Invalid cross-device link +cross-subvol tree/dire -> Invalid cross-device link +cross-subvol tree/tree -> Invalid cross-device link +Invalid cross-device link
Signed-off-by: Nikolay Borisov <nborisov@suse.com> --- tests/btrfs/246 | 46 +++++++++++++++++++++++++++++++++++++++++++++ tests/btrfs/246.out | 27 ++++++++++++++++++++++++++ 2 files changed, 73 insertions(+) create mode 100755 tests/btrfs/246 create mode 100644 tests/btrfs/246.out