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

Anand Jain
2021-12-10T18:08:14Z

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