[PATCH blktests v5 0/4] replace module removal with patient module removal

[PATCH blktests v5 0/4] replace module removal with patient module removal

[Shin'ichiro Kawasaki]

This patch series was originally authored by Luis Chamberlain [0][1]. I
reworked and post it as this v5 series.

[0] https://lore.kernel.org/all/20221220235324.1445248-2-mcgrof@kernel.org/T/#u
[1] https://lore.kernel.org/linux-block/20251126171102.3663957-1-mcgrof@kernel.org/

Original cover letter:

We now have the modprobe --wait upstream so use that if available.

The patient module remover addresses race conditions where module removal
can fail due to userspace temporarily bumping the refcount (e.g., via
blkdev_open() calls). If your version of kmod supports modprobe --wait,
we use that. Otherwise we implement our own patient module remover.

* Changes from v4
- 1st patch: moved the new functions from "common/rc" to "check"
- 1st patch: reflected comments by Bart
- 2nd patch: moved the srp/rc hunk from the 1st patch
- Added the 3rd and the 4th patches

Luis Chamberlain (2):
  check,common/*: replace module removal with patient module removal
  srp/rc: replace module removal with patient module removal

Shin'ichiro Kawasaki (2):
  check: reimplement _unload_modules() with _patient_rmmod()
  check: check reference count for modprobe --remove --wait success case

 check                      | 134 +++++++++++++++++++++++++++++++++----
 common/multipath-over-rdma |  10 +--
 common/null_blk            |   5 +-
 common/nvme                |   8 +--
 common/scsi_debug          |  12 +---
 tests/srp/rc               |  19 ++----
 6 files changed, 139 insertions(+), 49 deletions(-)

-- 
2.52.0

[PATCH blktests v5 1/4] check,common/*: replace module removal with patient module removal

[Shin'ichiro Kawasaki]

From: Luis Chamberlain <mcgrof@kernel.org>

A long time ago, in a galaxy far, far away...

I ran into some odd scsi_debug false positives with fstests. This
prompted me to look into them given these false positives prevents
me from moving forward with establishing a test baseline with high
number of cycles. That is, this stupid issue was prevening creating
high confidence in testing.

I reported it in 2021 [0] and exchanged some ideas with Doug. However,
in the end, despite efforts to help things with scsi_debug there were
still issues lingering which seemed to defy our expectations upstream.
One of the last hanging fruit issues is and always has been that
userspace expectations for proper module removal has been broken,
so in the end I have demonstrated this is a generic issue [1]. The same
problem was reported by Swarna in 2025 [2].

Long ago a WAIT option for module removal was added... that was then
removed as it was deemed not needed as folks couldn't figure out when
these races happened. The races are actually pretty easy to trigger, it
was just never properly documented. A simpe blkdev_open() will easily
bump a module refcnt, and these days many thing scan do that sort of
thing.

The proper solution is to implement then a patient module removal
on kmod and that has been merged now as modprobe --wait=MSEC option.
We need a work around to open code a similar solution for users of
old versions of kmod. An open coded solution for fstests exists
there for over a year now. This now provides the respective blktests
implementation.

[0] https://bugzilla.kernel.org/show_bug.cgi?id=212337
[1] https://bugzilla.kernel.org/show_bug.cgi?id=214015
[2] https://lore.kernel.org/linux-block/aUmJtfPM7A26swxN@deb-101020-bm01.eng.stellus.in/

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
[Shin'ichiro: moved new function to 'check' to not source common/rc]
[Shin'ichiro: moved srp/rc hunk to the next patch]
[Shin'ichiro: reflected comments by Bart for v4 series]
[Shin'ichiro: replaced modprobe -r option with --remove option]
Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 check                      | 118 +++++++++++++++++++++++++++++++++++++
 common/multipath-over-rdma |  10 +---
 common/null_blk            |   5 +-
 common/nvme                |   8 +--
 common/scsi_debug          |  12 +---
 5 files changed, 130 insertions(+), 23 deletions(-)

diff --git a/check b/check
index 6d77d8e..6a156b3 100755
--- a/check
+++ b/check
@@ -463,6 +463,124 @@ _test_dev_is_zoned() {
 	   $(cat "${TEST_DEV_SYSFS}/queue/zoned") != none ]]
 }
 
+_has_modprobe_wait()
+{
+	modprobe --help |& grep --quiet -- --wait
+}
+
+# Check whether modprobe --wait is supported and set up the patient module
+# removal command. This is evaluated at source time, so we need to handle
+# the timeout dynamically in _patient_rmmod() for cases where tests want
+# to override it.
+MODPROBE_HAS_WAIT=""
+if _has_modprobe_wait; then
+	MODPROBE_HAS_WAIT="yes"
+fi
+
+if [[ -z "$MODPROBE_PATIENT_RM_TIMEOUT_SECONDS" ]]; then
+	export MODPROBE_PATIENT_RM_TIMEOUT_SECONDS="50"
+fi
+
+# checks the refcount and returns 0 if we can safely remove the module. rmmod
+# does this check for us, but we can use this to also iterate checking for this
+# refcount before we even try to remove the module. This is useful when using
+# debug test modules which take a while to quiesce.
+_patient_rmmod_check_refcnt()
+{
+	local module=$1
+	local refcnt
+
+	refcnt=$(cat "/sys/module/$module/refcnt" 2>/dev/null)
+	[[ $? -ne 0 || $refcnt -eq 0 ]]
+}
+
+# Tries to wait patiently to remove a module by ensuring first
+# the refcnt is 0 and then trying to remove the module over and over
+# again within the time allowed. The timeout is configurable per test, just set
+# MODPROBE_PATIENT_RM_TIMEOUT_SECONDS prior to calling this function.
+# This applies to both cases where kmod supports the patient module remover
+# (modprobe --wait) and where it does not.
+#
+# If your version of kmod supports modprobe --wait, we use that instead.
+# Otherwise we have to implement a patient module remover ourselves.
+_patient_rmmod()
+{
+	local module=$1
+	local max_tries_max=$MODPROBE_PATIENT_RM_TIMEOUT_SECONDS
+	local max_tries=0
+	local mod_ret=0
+	local refcnt_is_zero=0
+	# Since we are looking for a directory we must adopt the
+	# specific directory used by scripts/Makefile.lib for
+	# KBUILD_MODNAME
+	local module_sys=${module//-/_}
+
+	# Check if module is built-in or not loaded
+	if [[ ! -d "/sys/module/$module_sys" ]]; then
+		return 0
+	fi
+
+	# Check if this is a built-in module (no refcnt file means built-in)
+	if [[ ! -f "/sys/module/$module_sys/refcnt" ]]; then
+		return 0
+	fi
+
+	if [[ -n $MODPROBE_HAS_WAIT ]]; then
+		local timeout_ms=$((MODPROBE_PATIENT_RM_TIMEOUT_SECONDS * 1000))
+		modprobe --remove --wait="${timeout_ms}" "$module"
+		mod_ret=$?
+		if [[ $mod_ret -ne 0 ]]; then
+			echo "kmod patient module removal for $module timed out waiting for refcnt to become 0 using timeout of $max_tries_max returned $mod_ret"
+		fi
+		return $mod_ret
+	fi
+
+	for ((max_tries=max_tries_max; max_tries != 0; max_tries--)); do
+		if _patient_rmmod_check_refcnt "$module_sys"; then
+			refcnt_is_zero=1
+			break
+		fi
+		sleep 1
+	done
+
+	if [[ $refcnt_is_zero -ne 1 ]]; then
+		echo "custom patient module removal for $module timed out waiting for refcnt to become 0 using timeout of $max_tries_max"
+		return 1
+	fi
+
+	# If we ran out of time but our refcnt check confirms we had
+	# a refcnt of 0, just try to remove the module once.
+	if [[ "$max_tries" == "0" ]]; then
+		modprobe --remove "$module"
+		return $?
+	fi
+
+	# If we have extra time left. Use the time left to now try to
+	# persistently remove the module. We do this because although through
+	# the above we found refcnt to be 0, removal can still fail since
+	# userspace can always race to bump the refcnt. An example is any
+	# blkdev_open() calls against a block device. These issues have been
+	# tracked and documented in the following bug reports, which justifies
+	# our need to do this in userspace:
+	# https://bugzilla.kernel.org/show_bug.cgi?id=212337
+	# https://bugzilla.kernel.org/show_bug.cgi?id=214015
+	while [[ $max_tries != 0 ]] && [[ -d /sys/module/$module_sys ]]; do
+		modprobe --remove "$module" 2> /dev/null
+		mod_ret=$?
+		if [[ $mod_ret == 0 ]]; then
+			break;
+		fi
+		sleep 1
+		((max_tries--))
+	done
+
+	if [[ $mod_ret -ne 0 ]]; then
+		echo "custom patient module removal for $module timed out trying to remove using timeout of $max_tries_max last try returned $mod_ret"
+	fi
+
+	return $mod_ret
+}
+
 # Arguments: module to unload ($1) and retry count ($2).
 _unload_module() {
 	local i m=$1 rc=${2:-1} reason
diff --git a/common/multipath-over-rdma b/common/multipath-over-rdma
index 1084f80..9b72d26 100644
--- a/common/multipath-over-rdma
+++ b/common/multipath-over-rdma
@@ -427,14 +427,8 @@ stop_soft_rdma() {
 		      echo "$i ..."
 		      rdma link del "${i}" || echo "Failed to remove ${i}"
 		done
-	if ! _unload_module rdma_rxe 10; then
-		echo "Unloading rdma_rxe failed"
-		return 1
-	fi
-	if ! _unload_module siw 10; then
-		echo "Unloading siw failed"
-		return 1
-	fi
+	_patient_rmmod rdma_rxe || return 1
+	_patient_rmmod siw  || return 1
 	} >>"$FULL"
 }
 
diff --git a/common/null_blk b/common/null_blk
index bbb6f78..c7d6a56 100644
--- a/common/null_blk
+++ b/common/null_blk
@@ -38,7 +38,8 @@ _init_null_blk() {
 	local args=("$@")
 	if (( RUN_FOR_ZONED )); then args+=("zoned=1"); fi
 
-	if ! modprobe -r null_blk || ! modprobe null_blk "${args[@]}" ; then
+	_patient_rmmod null_blk || return 1
+	if ! modprobe null_blk "${args[@]}"; then
 		SKIP_REASONS+=("requires modular null_blk")
 		return 1
 	fi
@@ -79,5 +80,5 @@ _configure_null_blk() {
 _exit_null_blk() {
 	_remove_null_blk_devices
 	udevadm settle
-	modprobe -r -q null_blk
+	_patient_rmmod null_blk
 }
diff --git a/common/nvme b/common/nvme
index 3d43790..601c748 100644
--- a/common/nvme
+++ b/common/nvme
@@ -210,13 +210,13 @@ _cleanup_nvmet() {
 
 	if [[ "${nvme_trtype}" == "fc" ]]; then
 		_nvme_fcloop_del_lport "${def_local_wwnn}" "${def_local_wwpn}"
-		modprobe -rq nvme-fcloop 2>/dev/null
+		_patient_rmmod nvme-fcloop
 	fi
-	modprobe -rq nvme-"${nvme_trtype}" 2>/dev/null
+	_patient_rmmod nvme-"${nvme_trtype}"
 	if [[ "${nvme_trtype}" != "loop" ]]; then
-		modprobe -rq nvmet-"${nvme_trtype}" 2>/dev/null
+		_patient_rmmod nvmet-"${nvme_trtype}"
 	fi
-	modprobe -rq nvmet 2>/dev/null
+	_patient_rmmod nvmet
 	if [[ "${nvme_trtype}" == "rdma" ]]; then
 		stop_soft_rdma
 	fi
diff --git a/common/scsi_debug b/common/scsi_debug
index 89c4801..8964558 100644
--- a/common/scsi_debug
+++ b/common/scsi_debug
@@ -98,14 +98,8 @@ _init_scsi_debug() {
 		args+=(zbc=host-managed zone_nr_conv=0)
 	fi
 
-	if ! _unload_module scsi_debug 10; then
-		echo "Unloading scsi_debug failed" >&2
-		return 1
-	fi
-	if ! modprobe scsi_debug "${args[@]}"; then
-		echo "Loading scsi_debug ${args[*]} failed" >&2
-		return 1
-	fi
+	_patient_rmmod scsi_debug || return 1
+	modprobe scsi_debug "${args[@]}" || return 1
 
 	udevadm settle
 
@@ -180,7 +174,7 @@ _exit_scsi_debug() {
 	udevadm settle
 
 	if _module_file_exists scsi_debug; then
-		_unload_module scsi_debug 10
+		_patient_rmmod scsi_debug
 		return
 	fi
 
-- 
2.52.0

[PATCH blktests v5 2/4] srp/rc: replace module removal with patient module removal

[Shin'ichiro Kawasaki]

From: Luis Chamberlain <mcgrof@kernel.org>

Bart had put the remove_mpath_devs() call inside a loop because multipathd
keeps running while the loop is ongoing and hence can modify paths
while the loop is running. The races that multipathd can trigger with the
module refcnt is precisely the sort of races which patient module
removal is supposed to address.

Suggested-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
[Shin'ichiro: moved a hunk of srp/rc change from the previous patch]
Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 tests/srp/rc | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/tests/srp/rc b/tests/srp/rc
index 47b9546..2d3d615 100755
--- a/tests/srp/rc
+++ b/tests/srp/rc
@@ -331,19 +331,10 @@ start_srp_ini() {
 
 # Unload the SRP initiator driver.
 stop_srp_ini() {
-	local i
-
 	log_out
-	for ((i=40;i>=0;i--)); do
-		remove_mpath_devs || return $?
-		_unload_module ib_srp >/dev/null 2>&1 && break
-		sleep 1
-	done
-	if [ -e /sys/module/ib_srp ]; then
-		echo "Error: unloading kernel module ib_srp failed"
-		return 1
-	fi
-	_unload_module scsi_transport_srp || return $?
+	remove_mpath_devs || return $?
+	_patient_rmmod ib_srp || return 1
+	_patient_rmmod scsi_transport_srp || return $?
 }
 
 # Associate the LIO device with name $1/$2 with file $3 and SCSI serial $4.
@@ -502,7 +493,7 @@ start_lio_srpt() {
 	if modinfo ib_srpt | grep -q '^parm:[[:blank:]]*rdma_cm_port:'; then
 		opts+=("rdma_cm_port=${srp_rdma_cm_port}")
 	fi
-	_unload_module ib_srpt
+	_patient_rmmod ib_srpt
 	modprobe ib_srpt "${opts[@]}" || return $?
 	i=0
 	for r in "${vdev_path[@]}"; do
@@ -564,7 +555,7 @@ stop_lio_srpt() {
 			 target_core_file target_core_stgt target_core_user \
 			 target_core_mod
 	do
-		_unload_module $m 10 || return $?
+		_patient_rmmod $m || return $?
 	done
 }
 
-- 
2.52.0

[PATCH blktests v5 3/4] check: reimplement _unload_modules() with _patient_rmmod()

[Shin'ichiro Kawasaki]

The previous patches dropped most of the calls to the helper function
_unload_module(). The last left caller is _unload_modules(). Reimplement
it to call _patient_rmmod() instead. Then remove _unload_module().

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 check | 21 +++++----------------
 1 file changed, 5 insertions(+), 16 deletions(-)

diff --git a/check b/check
index 6a156b3..cd247a0 100755
--- a/check
+++ b/check
@@ -581,25 +581,14 @@ _patient_rmmod()
 	return $mod_ret
 }
 
-# Arguments: module to unload ($1) and retry count ($2).
-_unload_module() {
-	local i m=$1 rc=${2:-1} reason
-
-	[ ! -e "/sys/module/$m" ] && return 0
-	for ((i=rc;i>0;i--)); do
-		reason=$(modprobe -r "$m" 2>&1)
-		[ ! -e "/sys/module/$m" ] && return 0
-		sleep .1
-	done
-	echo "${reason}" >&2
-	return 1
-}
-
 _unload_modules() {
-	local i
+	local i reason
 
 	for ((i=${#MODULES_TO_UNLOAD[@]}; i > 0; i--)); do
-		_unload_module "${MODULES_TO_UNLOAD[i-1]}" 10
+		if ! reason=$(_patient_rmmod "${MODULES_TO_UNLOAD[i-1]}" \
+					     2>&1); then
+			echo "${reason}" >&2
+		fi
 	done
 
 	unset MODULES_TO_UNLOAD
-- 
2.52.0

[PATCH blktests v5 4/4] check: check reference count for modprobe --remove --wait success case

[Shin'ichiro Kawasaki]

The commit "check,common/*: replace module removal with patient module
removal" introduced the new helper function _patient_rmmod() which calls
modprobe command with --wait option to do patient module removal.
However, the modprobe command can return a zero exit status even when
the module removal fails. In such cases, the failure remains unreported
and hidden. This behavior was observed during the execution of blktests
srp test group using rdma_rxe driver on a kernel affected by the
rdma_rxe module unload failure bug, which was addressed by the recent
patch [1].

To address this problem, check the reference count of the target module
after calling the modprobe command in _patient_rmmod(). If the module's
reference count indicates a removal failure, print an error message to
stderr. While at it, change the print target stream from stdout to
stderr for other error messages in _patient_rmmod() to ensure the
messages are printed on failure.

[1] https://lore.kernel.org/linux-rdma/20251219140408.2300163-1-metze@samba.org/

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 check | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/check b/check
index cd247a0..b554cd0 100755
--- a/check
+++ b/check
@@ -530,7 +530,10 @@ _patient_rmmod()
 		modprobe --remove --wait="${timeout_ms}" "$module"
 		mod_ret=$?
 		if [[ $mod_ret -ne 0 ]]; then
-			echo "kmod patient module removal for $module timed out waiting for refcnt to become 0 using timeout of $max_tries_max returned $mod_ret"
+			echo "kmod patient module removal for $module timed out waiting for refcnt to become 0 using timeout of $max_tries_max returned $mod_ret" >&2
+		elif ! _patient_rmmod_check_refcnt "$module_sys"; then
+			echo "modprobe with --wait option succeeded but still $module has references" >&2
+			mod_ret=1
 		fi
 		return $mod_ret
 	fi
@@ -544,7 +547,7 @@ _patient_rmmod()
 	done
 
 	if [[ $refcnt_is_zero -ne 1 ]]; then
-		echo "custom patient module removal for $module timed out waiting for refcnt to become 0 using timeout of $max_tries_max"
+		echo "custom patient module removal for $module timed out waiting for refcnt to become 0 using timeout of $max_tries_max" 2>&1
 		return 1
 	fi
 
@@ -575,7 +578,7 @@ _patient_rmmod()
 	done
 
 	if [[ $mod_ret -ne 0 ]]; then
-		echo "custom patient module removal for $module timed out trying to remove using timeout of $max_tries_max last try returned $mod_ret"
+		echo "custom patient module removal for $module timed out trying to remove using timeout of $max_tries_max last try returned $mod_ret" 2>&1
 	fi
 
 	return $mod_ret
-- 
2.52.0

Re: [PATCH blktests v5 2/4] srp/rc: replace module removal with patient module removal

[Bart Van Assche]

On 12/25/25 1:09 PM, Shin'ichiro Kawasaki wrote:
> Suggested-by: Bart Van Assche <bvanassche@acm.org>

I can't remember that I suggested to use _patient_rmmod. Luis, can you
provide a link that shows that I suggested to use _patient_rmmod for the
SRP tests? If not, please leave out the Suggested-by tag.

> diff --git a/tests/srp/rc b/tests/srp/rc
> index 47b9546..2d3d615 100755
> --- a/tests/srp/rc
> +++ b/tests/srp/rc
> @@ -331,19 +331,10 @@ start_srp_ini() {
>   
>   # Unload the SRP initiator driver.
>   stop_srp_ini() {
> -	local i
> -
>   	log_out
> -	for ((i=40;i>=0;i--)); do
> -		remove_mpath_devs || return $?
> -		_unload_module ib_srp >/dev/null 2>&1 && break
> -		sleep 1
> -	done
> -	if [ -e /sys/module/ib_srp ]; then
> -		echo "Error: unloading kernel module ib_srp failed"
> -		return 1
> -	fi
> -	_unload_module scsi_transport_srp || return $?
> +	remove_mpath_devs || return $?
> +	_patient_rmmod ib_srp || return 1
> +	_patient_rmmod scsi_transport_srp || return $?
>   }

The loop should be around both remove_mpath_devs and _unload_module
because multipathd may add new paths concurrently with the for-loop.
Please back out the above changes from this patch.

Thanks,

Bart.

Re: [PATCH blktests v5 2/4] srp/rc: replace module removal with patient module removal

[Shinichiro Kawasaki]

On Dec 26, 2025 / 10:08, Bart Van Assche wrote:
> On 12/25/25 1:09 PM, Shin'ichiro Kawasaki wrote:
> > Suggested-by: Bart Van Assche <bvanassche@acm.org>
> 
> I can't remember that I suggested to use _patient_rmmod. Luis, can you
> provide a link that shows that I suggested to use _patient_rmmod for the
> SRP tests? If not, please leave out the Suggested-by tag.

Based on your another comment below, I think this patch can be dropped. If this
is the case, no need to worry about the Suggested-by tag.

> 
> > diff --git a/tests/srp/rc b/tests/srp/rc
> > index 47b9546..2d3d615 100755
> > --- a/tests/srp/rc
> > +++ b/tests/srp/rc
> > @@ -331,19 +331,10 @@ start_srp_ini() {
> >   # Unload the SRP initiator driver.
> >   stop_srp_ini() {
> > -	local i
> > -
> >   	log_out
> > -	for ((i=40;i>=0;i--)); do
> > -		remove_mpath_devs || return $?
> > -		_unload_module ib_srp >/dev/null 2>&1 && break
> > -		sleep 1
> > -	done
> > -	if [ -e /sys/module/ib_srp ]; then
> > -		echo "Error: unloading kernel module ib_srp failed"
> > -		return 1
> > -	fi
> > -	_unload_module scsi_transport_srp || return $?
> > +	remove_mpath_devs || return $?
> > +	_patient_rmmod ib_srp || return 1
> > +	_patient_rmmod scsi_transport_srp || return $?
> >   }
> 
> The loop should be around both remove_mpath_devs and _unload_module
> because multipathd may add new paths concurrently with the for-loop.
> Please back out the above changes from this patch.

I see. If we back out the changes above, this patch does not have much
meaning. Just replacing the _unload_module calls with _patient_rmmod calls. I
will drop this patch in the next spin. In stead, I will modify the first patch
to replace all of _unload_module calls in srp/rc with _patient_rmmod calls.