[PATCH] btrfs: Add test for rename exchange behavior between subvolumes

[PATCH] btrfs: Add test for rename exchange behavior between subvolumes

[Nikolay Borisov]

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

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
-- 
2.17.1

Re: [PATCH] btrfs: Add test for rename exchange behavior between subvolumes

[Filipe Manana]

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
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH] btrfs: Add test for rename exchange behavior between subvolumes

[Nikolay Borisov]

<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>

Re: [PATCH] btrfs: Add test for rename exchange behavior between subvolumes

[Filipe Manana]

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>



-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH] btrfs: Add test for rename exchange behavior between subvolumes

[Nikolay Borisov]

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>
> 
> 
> 

Re: [PATCH] btrfs: Add test for rename exchange behavior between subvolumes

[Filipe Manana]

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>
> >
> >
> >



-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH] btrfs: Add test for rename exchange behavior between subvolumes

[Eryu Guan]

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

Re: [PATCH] btrfs: Add test for rename exchange behavior between subvolumes

[Nikolay Borisov]

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>