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

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

On 09/12/2021 22:28, Josef Bacik wrote:
> On Thu, Dec 09, 2021 at 02:41:22PM +0800, Anand Jain wrote:
>> 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?
>>
> 
> Yeah that's what I'm assuming, since the original problem I had was transient, I
> just want to make sure that's what I'm seeing and not that the test isn't quite
> right.  As long as it fails we're good, but it makes me nervous when the
> expectected output matches the failure case so I wanted to double check.
> 
>>> 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?
>>
> 
> Yup, its the following, I was using -s btrfs_normal
> 
> [btrfs_normal]
> TEST_DIR=/mnt/test
> TEST_DEV=/dev/mapper/vg0-lv0
> SCRATCH_DEV_POOL="/dev/mapper/vg0-lv9 /dev/mapper/vg0-lv8 /dev/mapper/vg0-lv7 /dev/mapper/vg0-lv6 /dev/mapper/vg0-lv5 /dev/mapper/vg0-lv4 /dev/mapper/vg0-lv3 /dev/mapper/vg0-lv2 /dev/mapper/vg0-lv1 "
> SCRATCH_MNT=/mnt/scratch
> LOGWRITES_DEV=/dev/mapper/vg0-lv10
> PERF_CONFIGNAME=jbacik
> MKFS_OPTIONS="-K -O ^no-holes -R ^free-space-tree"
> MOUNT_OPTIONS="-o discard=async"
> 
> [btrfs_compression]
> MOUNT_OPTIONS="-o compress=zstd,discard=async"
> MKFS_OPTIONS="-K -O ^no-holes -R ^free-space-tree"
> 
> Weirdly compression would pass sometimes without the fix applied, so using -s
> btrfs_compression.  IDK why, clearly this is a weird problem and depends on udev
> timing, but maybe take another look?  Thanks,


Sure.

I guess the udev rule in this file is playing the trick.
--------
$ cat /usr/lib/udev/rules.d/64-btrfs-dm.rules

SUBSYSTEM!="block", GOTO="btrfs_end"
KERNEL!="dm-[0-9]*", GOTO="btrfs_end"
ACTION!="add|change", GOTO="btrfs_end"
ENV{ID_FS_TYPE}!="btrfs", GOTO="btrfs_end"

# Once the device mapper symlink is created, tell btrfs about it
# so we get the friendly name in /proc/mounts (and tools that read it)
ENV{DM_NAME}=="?*", RUN{builtin}+="btrfs ready /dev/mapper/$env{DM_NAME}"

LABEL="btrfs_end"
--------

I tried to find if I could disable the udev scan, can't find a way yet.

However, if we flip the lv path and dm path, then the udev scan will be 
ineffective because we already have that path in the kernel.

I could reproduce the bug without the fix in the kernel patch.

-------------
$ cat <...>254.out.bad

QA output created by 254
ERROR: cannot unregister device '/dev/mapper/254-test': No such file or 
directory
Label: none  uuid: <UUID>
         Total devices <NUM> FS bytes used <SIZE>
         devid <DEVID> size <SIZE> used <SIZE> path /dev/mapper/254-test
         devid <DEVID> size <SIZE> used <SIZE> path SCRATCH_DEV
-------------


I have made this change in v2. Could you please give it a try.

Also, I have accepted the above two review comments.

Thanks, Anand