linux-btrfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] btrfs/254: test cleaning up of the stale device
@ 2021-12-08 14:07 Anand Jain
  2021-12-08 14:50 ` Josef Bacik
  0 siblings, 1 reply; 6+ messages in thread
From: Anand Jain @ 2021-12-08 14:07 UTC (permalink / raw)
  To: fstests; +Cc: linux-btrfs, josef

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.
+# 
+. ./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
+
+	_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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] btrfs/254: test cleaning up of the stale device
  2021-12-08 14:07 [PATCH] btrfs/254: test cleaning up of the stale device Anand Jain
@ 2021-12-08 14:50 ` Josef Bacik
  2021-12-09  6:41   ` Anand Jain
  0 siblings, 1 reply; 6+ messages in thread
From: Josef Bacik @ 2021-12-08 14:50 UTC (permalink / raw)
  To: Anand Jain; +Cc: fstests, linux-btrfs

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.
 
> +. ./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.

> +	_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
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?  I was expecting to not see the "*** Some devices
missing" part as well, but I guess that's the racier part?

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,

Josef

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] btrfs/254: test cleaning up of the stale device
  2021-12-08 14:50 ` Josef Bacik
@ 2021-12-09  6:41   ` Anand Jain
  2021-12-09 14:28     ` Josef Bacik
  0 siblings, 1 reply; 6+ messages in thread
From: Anand Jain @ 2021-12-09  6:41 UTC (permalink / raw)
  To: Josef Bacik; +Cc: fstests, linux-btrfs

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] btrfs/254: test cleaning up of the stale device
  2021-12-09  6:41   ` Anand Jain
@ 2021-12-09 14:28     ` Josef Bacik
  2021-12-10 18:07       ` Anand Jain
  2021-12-14 13:12       ` David Sterba
  0 siblings, 2 replies; 6+ messages in thread
From: Josef Bacik @ 2021-12-09 14:28 UTC (permalink / raw)
  To: Anand Jain; +Cc: fstests, linux-btrfs

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,

Josef

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] btrfs/254: test cleaning up of the stale device
  2021-12-09 14:28     ` Josef Bacik
@ 2021-12-10 18:07       ` Anand Jain
  2021-12-14 13:12       ` David Sterba
  1 sibling, 0 replies; 6+ messages in thread
From: Anand Jain @ 2021-12-10 18:07 UTC (permalink / raw)
  To: Josef Bacik; +Cc: fstests, linux-btrfs

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



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] btrfs/254: test cleaning up of the stale device
  2021-12-09 14:28     ` Josef Bacik
  2021-12-10 18:07       ` Anand Jain
@ 2021-12-14 13:12       ` David Sterba
  1 sibling, 0 replies; 6+ messages in thread
From: David Sterba @ 2021-12-14 13:12 UTC (permalink / raw)
  To: Josef Bacik; +Cc: Anand Jain, fstests, linux-btrfs

On Thu, Dec 09, 2021 at 09:28:11AM -0500, 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:
> 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"

Unrelated to the patch, but why do you disable no-holes and
free-space-tree? Is this a special testing setup?

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-12-14 13:13 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-08 14:07 [PATCH] btrfs/254: test cleaning up of the stale device Anand Jain
2021-12-08 14:50 ` Josef Bacik
2021-12-09  6:41   ` Anand Jain
2021-12-09 14:28     ` Josef Bacik
2021-12-10 18:07       ` Anand Jain
2021-12-14 13:12       ` David Sterba

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).