Re: [PATCH v2 10/10] btrfs: test tempfsid with device add, seed, and balance

[Anand Jain <anand.jain@oracle.com>]

On 2/20/24 22:35, Filipe Manana wrote:
> On Mon, Feb 19, 2024 at 7:50 PM Anand Jain <anand.jain@oracle.com> wrote:
>>
>> Make sure that basic functions such as seeding and device add fail,
>> while balance runs successfully with tempfsid.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> v2:
>>   Remove unnecessary function.
>>   Add clone group
>>   use $UMOUNT_PROG
>>   Let _cp_reflink fail on the stdout.
>>
>>   tests/btrfs/315     | 79 +++++++++++++++++++++++++++++++++++++++++++++
>>   tests/btrfs/315.out |  9 ++++++
>>   2 files changed, 88 insertions(+)
>>   create mode 100755 tests/btrfs/315
>>   create mode 100644 tests/btrfs/315.out
>>
>> diff --git a/tests/btrfs/315 b/tests/btrfs/315
>> new file mode 100755
>> index 000000000000..4376c7f1849c
>> --- /dev/null
>> +++ b/tests/btrfs/315
>> @@ -0,0 +1,79 @@
>> +#! /bin/bash
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (c) 2024 YOUR NAME HERE.  All Rights Reserved.
> 
> Your name here...
> 
>> +#
>> +# FS QA Test 315
>> +#
>> +# Verify if the seed and device add to a tempfsid filesystem fails.
> 
> And balance too...
> 
>> +#
>> +. ./common/preamble
>> +_begin_fstest auto quick volume seed tempfsid
> 
> Missing 'balance' group.
> 

   Now added.

>> +
>> +_cleanup()
>> +{
>> +       cd /
>> +       $UMOUNT_PROG $tempfsid_mnt 2>/dev/null
>> +       rm -r -f $tmp.*
>> +       rm -r -f $tempfsid_mnt
>> +}
>> +
>> +. ./common/filter.btrfs
>> +
>> +_supported_fs btrfs
>> +_require_btrfs_sysfs_fsid
>> +_require_scratch_dev_pool 3
>> +_require_btrfs_fs_feature temp_fsid

>> +_require_btrfs_command inspect-internal dump-super
>> +_require_btrfs_mkfs_uuid_option
> 
> So same as before, these last 2 _require_* are because of the
> mkfs_clone() function,
> defined at common/btrfs, so they should be in the function and not
> spread over every test case that calls it.

The last two _require have now been removed because mkfs_clone()
includes those checks.

>> +
>> +_scratch_dev_pool_get 3
>> +
>> +# mount point for the tempfsid device
>> +tempfsid_mnt=$TEST_DIR/$seq/tempfsid_mnt
>> +
>> +seed_device_must_fail()
>> +{
>> +       echo ---- $FUNCNAME ----
>> +
>> +       mkfs_clone ${SCRATCH_DEV} ${SCRATCH_DEV_NAME[1]}
>> +
>> +       $BTRFS_TUNE_PROG -S 1 ${SCRATCH_DEV}
>> +       $BTRFS_TUNE_PROG -S 1 ${SCRATCH_DEV_NAME[1]}
>> +
>> +       _scratch_mount 2>&1 | _filter_scratch
>> +       _mount ${SCRATCH_DEV_NAME[1]} ${tempfsid_mnt} 2>&1 | _filter_test_dir
>> +}
>> +
>> +device_add_must_fail()
>> +{
>> +       echo ---- $FUNCNAME ----
>> +
>> +       mkfs_clone ${SCRATCH_DEV} ${SCRATCH_DEV_NAME[1]}
>> +       _scratch_mount
>> +       _mount ${SCRATCH_DEV_NAME[1]} ${tempfsid_mnt}
>> +
>> +       $XFS_IO_PROG -fc 'pwrite -S 0x61 0 9000' $SCRATCH_MNT/foo | \
>> +                                                       _filter_xfs_io
>> +


>> +$BTRFS_UTIL_PROG device add -f ${SCRATCH_DEV_NAME[2]} ${tempfsid_mnt} >> \
>> +                       $seqres.full 2>&1 && _fail "Failed to file device add"
> 
> Can't we do without the && _fail?
> Just call device add and put the expected error message in the golden output.
> It's the preferred pattern in fstests in general, and makes everything
> shorter and easier to read.


Upon rethinking, it can be done as outlined below, also updated
the golden output accordingly.

$BTRFS_UTIL_PROG device add -f ${SCRATCH_DEV_NAME[2]} ${tempfsid_mnt} 
2>&1 | \
                 grep -v "Performing full device TRIM" | 
_filter_scratch_pool


Thanks.