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

[Eryu Guan <guan@eryu.me>]

On Tue, Jan 04, 2022 at 07:25:20PM +0800, Anand Jain wrote:
> 
> Gentle ping? More below.

It's already been merged in last update.

> 
> On 20/12/2021 19:06, Anand Jain wrote:
> > 
> > 
> > On 19/12/2021 22:02, Eryu Guan wrote:
> > > On Sat, Dec 11, 2021 at 02:14:41AM +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>
> > > 
> > > I was testing with v5.16-rc2 kernel, which should not contain the kernel
> > > fix, but test still passed for me, I was testing with three loop devices
> > > as SCRATCH_DEV_POOL, and all default mkfs & mount options
> > > 
> > > SECTION       -- btrfs
> > > RECREATING    -- btrfs on /dev/mapper/testvg-lv1
> > > FSTYP         -- btrfs
> > > PLATFORM      -- Linux/x86_64 fedoravm 5.16.0-rc2 #22 SMP PREEMPT
> > > Mon Nov 29 00:54:26 CST 2021
> > > MKFS_OPTIONS  -- /dev/loop0
> > > MOUNT_OPTIONS -- /dev/loop0 /mnt/scratch
> > > btrfs/254 5s ...  5s
> > > Ran: btrfs/254
> > > Passed all 1 tests
> > > 
> > > Anything wrong with my setup?
> > > 
> > > And if tested with lv devices as SCRATCH_DEV_POOL
> > > 
> > > SCRATCH_DEV_POOL="/dev/mapper/testvg-lv2 /dev/mapper/testvg-lv3
> > > /dev/mapper/testvg-lv4 /dev/mapper/testvg-lv5
> > > /dev/mapper/testvg-lv6"
> > > 
> > > I got the following test failure
> > > 
> > >   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 SCRATCH_DEV
> > > 
> > > Maybe we should use _require_scratch_nolvm as well?
> > > 
> > 
> 
> > The test case is inconsistent because the systemd-udev block device scan
> > interferes with the test script. There is no way to disable the
> > systemd-udev scan (that I could find).
> > 
> > The same inconsistency (due to race with systemd-udev scan) would
> > persist with/without lvm.
> > 
> > So when the race fails, the test case is successful to reproduce the
> > issue. As you saw in the 2nd iteration.
> 
> 
> There isn't any approach to stop the test case from racing with the device
> scan, so reproducing the issue is inconsistent. As we have had some success
> so IMO this test case can still integrate, it will help to verify the kernel

Agreed.

> fix on some systems.
> 
> More below.
> 
> > 
> > Any suggestions?
> > 
> > Thanks, Anand
> > 
> > 
> > > > ---
> > > > v2: Add kernel patch title in the test case
> > > >      Redirect device add output to /dev/null (avoids tirm message)
> > > >      Use the lv path for mkfs and the dm path for the device add
> > > >       so that now path used in udev scan should match with what
> > > >       we already have in the kernel memory.
> > > > 
> > > > -       _mkfs_dev $uuid -draid1 -mraid1 $dmdev $scratch_dev2
> > > > +       _mkfs_dev $uuid -draid1 -mraid1 $lvdev $scratch_dev2
> > > >          # Add device should free the device under $uuid in the kernel.
> > > > -       $BTRFS_UTIL_PROG device add -f $lvdev $seq_mnt > /dev/null 2>&1
> > > > +       $BTRFS_UTIL_PROG device add -f $dmdev $seq_mnt > /dev/null 2>&1
> > > > 
> > > > 
> > > >   tests/btrfs/254     | 113 ++++++++++++++++++++++++++++++++++++++++++++
> > > >   tests/btrfs/254.out |   6 +++
> > > >   2 files changed, 119 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..b70b9d165897
> > > > --- /dev/null
> > > > +++ b/tests/btrfs/254
> > > > @@ -0,0 +1,113 @@
> > > > +#! /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.
> > > > +#
> > > > +# Tests bug fixed by the kernel patch:
> > > > +#    btrfs: harden identification of the stale device
> > > > +#
> > > > +. ./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
> > > 
> > > Should wipefs in cleanup as well, otherwise test fails with non-unique
> > > UUID
> > > 
> > > -Label: none  uuid: <UUID>
> > > -       Total devices <NUM> FS bytes used <SIZE>
> > > -       devid <DEVID> size <SIZE> used <SIZE> path SCRATCH_DEV
> > > -       *** Some devices missing
> 
> > > +ERROR: non-unique UUID: 12345678-1234-1234-1234-123456789abc
> 
> 
>  We don't need non-unique UUID. I have fixed this in v3.

Then would you please help patch the existing test? And we should add
'volume' group as well?

Thanks,
Eryu

> 
> Thanks,
> Anand
> 
> 
> > > +btrfs-progs v5.4
> > > +See http://btrfs.wiki.kernel.org for more information.
> > > 
> > > Thanks,
> > > Eryu
> > > 
> > > > +}
> > > > +
> > > > +# 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 $lvdev $scratch_dev2
> > > > +
> > > > +    # Add device should free the device under $uuid in the kernel.
> > > > +    $BTRFS_UTIL_PROG device add -f $dmdev $seq_mnt > /dev/null 2>&1
> > > > +
> > > > +    _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
> > > > +
> > > > -- 
> > > > 2.27.0