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

[Nikolay Borisov <nborisov@suse.com>]

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>