Re: [PATCH] btrfs/254: test cleaning up of the stale device

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

On 08/12/2021 22:50, Josef Bacik wrote:
> On Wed, Dec 08, 2021 at 10:07:46PM +0800, Anand Jain wrote:
>> Recreating a new filesystem or adding a device to a mounted the filesystem
>> should remove the device entries under its previous fsid even when
>> confused with different device paths to the same device.
>>
>> Fixed by the kernel patch (in the ml):
>>    btrfs: harden identification of the stale device
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   tests/btrfs/254     | 110 ++++++++++++++++++++++++++++++++++++++++++++
>>   tests/btrfs/254.out |   6 +++
>>   2 files changed, 116 insertions(+)
>>   create mode 100755 tests/btrfs/254
>>   create mode 100644 tests/btrfs/254.out
>>
>> diff --git a/tests/btrfs/254 b/tests/btrfs/254
>> new file mode 100755
>> index 000000000000..6c3414f73d15
>> --- /dev/null
>> +++ b/tests/btrfs/254
>> @@ -0,0 +1,110 @@
>> +#! /bin/bash
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (c) 2021 Anand Jain. All Rights Reserved.
>> +# Copyright (c) 2021 Oracle. All Rights Reserved.
>> +#
>> +# FS QA Test No. 254
>> +#
>> +# Test if the kernel can free the stale device entries.
>> +#
> 
> Can you include the patch name here as well, it makes it easier when I'm
> rebasing our staging branch to figure out if I need to disable a new test for
> our overnight runs.
>   

  Ok. I will include.

>> +. ./common/preamble
>> +_begin_fstest auto quick
>> +
>> +# Override the default cleanup function.
>> +node=$seq-test
>> +cleanup_dmdev()
>> +{
>> +	_dmsetup_remove $node
>> +}
>> +
>> +_cleanup()
>> +{
>> +	cd /
>> +	rm -f $tmp.*
>> +	rm -rf $seq_mnt > /dev/null 2>&1
>> +	cleanup_dmdev
>> +}
>> +
>> +# Import common functions.
>> +. ./common/filter
>> +. ./common/filter.btrfs
>> +
>> +# real QA test starts here
>> +_supported_fs btrfs
>> +_require_scratch_dev_pool 3
>> +_require_block_device $SCRATCH_DEV
>> +_require_dm_target linear
>> +_require_btrfs_forget_or_module_loadable
>> +_require_scratch_nocheck
>> +_require_command "$WIPEFS_PROG" wipefs
>> +
>> +_scratch_dev_pool_get 3
>> +
>> +setup_dmdev()
>> +{
>> +	# Some small size.
>> +	size=$((1024 * 1024 * 1024))
>> +	size_in_sector=$((size / 512))
>> +
>> +	table="0 $size_in_sector linear $SCRATCH_DEV 0"
>> +	_dmsetup_create $node --table "$table" || \
>> +		_fail "setup dm device failed"
>> +}
>> +
>> +# Use a known it is much easier to debug.
>> +uuid="--uuid 12345678-1234-1234-1234-123456789abc"
>> +lvdev=/dev/mapper/$node
>> +
>> +seq_mnt=$TEST_DIR/$seq.mnt
>> +mkdir -p $seq_mnt
>> +
>> +test_forget()
>> +{
>> +	setup_dmdev
>> +	dmdev=$(realpath $lvdev)
>> +
>> +	_mkfs_dev $uuid $dmdev
>> +
>> +	# Check if we can un-scan using the mapper device path.
>> +	$BTRFS_UTIL_PROG device scan --forget $lvdev
>> +
>> +	# Cleanup
>> +	$WIPEFS_PROG -a $lvdev > /dev/null 2>&1
>> +	$BTRFS_UTIL_PROG device scan --forget
>> +
>> +	cleanup_dmdev
>> +}
>> +
>> +test_add_device()
>> +{
>> +	setup_dmdev
>> +	dmdev=$(realpath $lvdev)
>> +	scratch_dev2=$(echo $SCRATCH_DEV_POOL | awk '{print $2}')
>> +	scratch_dev3=$(echo $SCRATCH_DEV_POOL | awk '{print $3}')
>> +
>> +	_mkfs_dev $scratch_dev3
>> +	_mount $scratch_dev3 $seq_mnt
>> +
>> +	_mkfs_dev $uuid -draid1 -mraid1 $dmdev $scratch_dev2
>> +
>> +	# Add device should free the device under $uuid in the kernel.
>> +	$BTRFS_UTIL_PROG device add -f $lvdev $seq_mnt
>> +
> 
> You need to redirect this to /dev/null, otherwise we get the TRIM message with
> newer btrfs-progs.
> 

   Ok.


>> +	_mount -o degraded $scratch_dev2 $SCRATCH_MNT
>> +
>> +	# Check if the missing device is shown.
>> +	$BTRFS_UTIL_PROG filesystem show -m $SCRATCH_MNT | \
>> +					_filter_btrfs_filesystem_show
>> +
>> +	$UMOUNT_PROG $seq_mnt
>> +	_scratch_unmount
>> +	cleanup_dmdev
>> +}
>> +
>> +test_forget
>> +test_add_device
>> +
>> +_scratch_dev_pool_put
>> +
>> +status=0
>> +exit
>> diff --git a/tests/btrfs/254.out b/tests/btrfs/254.out
>> new file mode 100644
>> index 000000000000..20819cf5140c
>> --- /dev/null
>> +++ b/tests/btrfs/254.out
>> @@ -0,0 +1,6 @@
>> +QA output created by 254
>> +Label: none  uuid: <UUID>
>> +	Total devices <NUM> FS bytes used <SIZE>
>> +	devid <DEVID> size <SIZE> used <SIZE> path SCRATCH_DEV
>> +	*** Some devices missing
> 
> I ran this on a box without your fix and I got this failure
> 
> [root@xfstests2 xfstests-dev]# cat /xfstests-dev/results//kdave/btrfs/254.out.bad
> QA output created by 254

> ERROR: cannot unregister device '/dev/mapper/254-test': No such file or directory

  Without the fix the error is expected.

> Label: none  uuid: <UUID>
>          Total devices <NUM> FS bytes used <SIZE>
>          devid <DEVID> size <SIZE> used <SIZE> path SCRATCH_DEV
>          *** Some devices missing
> 
> Is this what you're expecting? 

  Hmm, no. Without the fix, we shouldn't see the missing here.

> I was expecting to not see the "*** Some devices
> missing" part as well, but I guess that's the racier part?

  Right. I am guessing race with udev auto scan?

> It does fail properly without the patch and pass with your patch, so as long as
> this is what you expect to see then I'm good with this part.  Thanks,

  Yeah, we shouldn't see missing device _without_ the fix.
  Could you please share your xfstests config?

Thanks, Anand

> 
> Josef