2021-08-11 15:47:50

by Luis Chamberlain

[permalink] [raw]
Subject: [PATCH v2 0/3] fstests: add patient module remover

This is v2 of my series of enhancements to fstests to deal with
false positives with meta block drivers we use for tests such as
scsi_debug which are caused by races by not being able to remove
a driver.

Changes in v2:

- Now that I have confirmed the issue with the refcnt being bumped
after it becomes 0 is also present on linux-next, and *is* a generic
"this is life with modules" matter, I went ahead and implemented
a patient module remover support into kmod and posted patches.
What this means for this series of patches is that we get a real
patient module remover support in modprobe, and so modprobe -p
will soon be an option, if merged. This series then now checks for
that and if its present uses it, otherwise it open codes a similar
solution.

- The patient module remover now also re-tries to remove the module,
as *any* race can easily bump a module refcnt up. We just then need
an upper limit threshold on timeout or to decide if we run forever.

- adds udevadm settle after pvremove

- I confirm now I don't get any stupid module false positives on older
or newer kernels, and life can move on.

Luis Chamberlain (3):
fstests: use udevadm settle after pvremove
common/module: add patient module rmmod support
common/scsi_debug: use the patient module remover

common/config | 31 ++++++++++++++
common/module | 107 ++++++++++++++++++++++++++++++++++++++++++++++
common/scsi_debug | 6 ++-
tests/generic/081 | 5 ++-
tests/generic/108 | 1 +
tests/generic/459 | 1 +
6 files changed, 148 insertions(+), 3 deletions(-)

--
2.30.2


2021-08-11 15:48:15

by Luis Chamberlain

[permalink] [raw]
Subject: [PATCH v2 1/3] fstests: use udevadm settle after pvremove

As with creation, we also need to use udevadm settle
when removing a pv, otherwise we can trip on races with
module removals for the block devices in use.

This reduces the amount of time in which a block device
module refcnt for test modules such as scsi_debug spends
outside of 0.

Races with the refcnt being greater than 0 means module
removal can fail causing false positives. This helps
ensure that the pv is really long gone. These issues
are tracked for scsi_debug [0] and later found to be a
generic issue regardless of filesystem with pvremove [1].

Using udevadm settle *helps*, it does not address all
possible races with the refcnt as noted in the generic
bug entry [1].

[0] https://bugzilla.kernel.org/show_bug.cgi?id=212337
[1] https://bugzilla.kernel.org/show_bug.cgi?id=214015
Signed-off-by: Luis Chamberlain <[email protected]>
---
tests/generic/081 | 5 ++++-
tests/generic/108 | 1 +
tests/generic/459 | 1 +
3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/tests/generic/081 b/tests/generic/081
index f795b2c1..8e552074 100755
--- a/tests/generic/081
+++ b/tests/generic/081
@@ -12,6 +12,7 @@ _begin_fstest auto quick
# Override the default cleanup function.
_cleanup()
{
+ local pv_ret
cd /
rm -f $tmp.*

@@ -34,7 +35,9 @@ _cleanup()
$UMOUNT_PROG $mnt >> $seqres.full 2>&1
$LVM_PROG vgremove -f $vgname >>$seqres.full 2>&1
$LVM_PROG pvremove -f $SCRATCH_DEV >>$seqres.full 2>&1
- test $? -eq 0 && break
+ pv_ret=$?
+ $UDEV_SETTLE_PROG
+ test $pv_ret -eq 0 && break
sleep 2
done
}
diff --git a/tests/generic/108 b/tests/generic/108
index 7dd426c1..b7797e8f 100755
--- a/tests/generic/108
+++ b/tests/generic/108
@@ -21,6 +21,7 @@ _cleanup()
$UMOUNT_PROG $SCRATCH_MNT >>$seqres.full 2>&1
$LVM_PROG vgremove -f $vgname >>$seqres.full 2>&1
$LVM_PROG pvremove -f $SCRATCH_DEV $SCSI_DEBUG_DEV >>$seqres.full 2>&1
+ $UDEV_SETTLE_PROG
_put_scsi_debug_dev
rm -f $tmp.*
}
diff --git a/tests/generic/459 b/tests/generic/459
index e5e5e9ab..5b44e245 100755
--- a/tests/generic/459
+++ b/tests/generic/459
@@ -29,6 +29,7 @@ _cleanup()
$UMOUNT_PROG $SCRATCH_MNT >>$seqres.full 2>&1
$LVM_PROG vgremove -ff $vgname >>$seqres.full 2>&1
$LVM_PROG pvremove -ff $SCRATCH_DEV >>$seqres.full 2>&1
+ $UDEV_SETTLE_PROG
}

# Import common functions.
--
2.30.2

2021-08-15 12:37:42

by Eryu Guan

[permalink] [raw]
Subject: Re: [PATCH v2 1/3] fstests: use udevadm settle after pvremove

On Wed, Aug 11, 2021 at 08:45:10AM -0700, Luis Chamberlain wrote:
> As with creation, we also need to use udevadm settle
> when removing a pv, otherwise we can trip on races with
> module removals for the block devices in use.
>
> This reduces the amount of time in which a block device
> module refcnt for test modules such as scsi_debug spends
> outside of 0.
>
> Races with the refcnt being greater than 0 means module
> removal can fail causing false positives. This helps
> ensure that the pv is really long gone. These issues
> are tracked for scsi_debug [0] and later found to be a
> generic issue regardless of filesystem with pvremove [1].
>
> Using udevadm settle *helps*, it does not address all
> possible races with the refcnt as noted in the generic
> bug entry [1].
>
> [0] https://bugzilla.kernel.org/show_bug.cgi?id=212337
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=214015
> Signed-off-by: Luis Chamberlain <[email protected]>

I applied this patch for this week's update.

Thanks,
Eryu

> ---
> tests/generic/081 | 5 ++++-
> tests/generic/108 | 1 +
> tests/generic/459 | 1 +
> 3 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/tests/generic/081 b/tests/generic/081
> index f795b2c1..8e552074 100755
> --- a/tests/generic/081
> +++ b/tests/generic/081
> @@ -12,6 +12,7 @@ _begin_fstest auto quick
> # Override the default cleanup function.
> _cleanup()
> {
> + local pv_ret
> cd /
> rm -f $tmp.*
>
> @@ -34,7 +35,9 @@ _cleanup()
> $UMOUNT_PROG $mnt >> $seqres.full 2>&1
> $LVM_PROG vgremove -f $vgname >>$seqres.full 2>&1
> $LVM_PROG pvremove -f $SCRATCH_DEV >>$seqres.full 2>&1
> - test $? -eq 0 && break
> + pv_ret=$?
> + $UDEV_SETTLE_PROG
> + test $pv_ret -eq 0 && break
> sleep 2
> done
> }
> diff --git a/tests/generic/108 b/tests/generic/108
> index 7dd426c1..b7797e8f 100755
> --- a/tests/generic/108
> +++ b/tests/generic/108
> @@ -21,6 +21,7 @@ _cleanup()
> $UMOUNT_PROG $SCRATCH_MNT >>$seqres.full 2>&1
> $LVM_PROG vgremove -f $vgname >>$seqres.full 2>&1
> $LVM_PROG pvremove -f $SCRATCH_DEV $SCSI_DEBUG_DEV >>$seqres.full 2>&1
> + $UDEV_SETTLE_PROG
> _put_scsi_debug_dev
> rm -f $tmp.*
> }
> diff --git a/tests/generic/459 b/tests/generic/459
> index e5e5e9ab..5b44e245 100755
> --- a/tests/generic/459
> +++ b/tests/generic/459
> @@ -29,6 +29,7 @@ _cleanup()
> $UMOUNT_PROG $SCRATCH_MNT >>$seqres.full 2>&1
> $LVM_PROG vgremove -ff $vgname >>$seqres.full 2>&1
> $LVM_PROG pvremove -ff $SCRATCH_DEV >>$seqres.full 2>&1
> + $UDEV_SETTLE_PROG
> }
>
> # Import common functions.
> --
> 2.30.2