[PATCH blktests 0/5] improve _have_driver() module load issue solution

[PATCH blktests 0/5] improve _have_driver() module load issue solution

[Shin'ichiro Kawasaki]

The commit 06a0ba866d90 ("common/rc: avoid module load in _have_driver()")
removed module load from _have_driver(). However, it turned out that it was not
a good solution. In the recent discussion for module load preparation for nvme
test cases, it was pointed out no module load in _have_driver() is confusing and
adding complexity [1]:

 - Without module load in _have_driver(), explicit module load and unload are
   required in number of test cases. Boiler plates.
 - The module unload is not always safe. Need care if the module unload is
   expected or not.
 - The module load needs error handling.

I can think of a new helper function to address the comments above, but it will
look like _have_driver() with module load. Hence, I suggest to revert back some
part of the the commit 06a0ba866d90 to load modules in _have_driver() (Sorry
Christoph, Bart for doing this on the commit you reviewed). As a better
solution, I propose to record the modules loaded in _have_driver() and unload
them at each test case end, regardless of the test case is skipped or executed.
I confirmed this fix avoids the issue that the commit 06a0ba866d90 tried to fix.

In this series, 4th patch is the core change in _have_driver. 1st, 2nd and 3rd
patches are clean-up preparation patches for the 4th patch. 5th patch reverts
changes in nbd/rc, which is no longer required after the 4th patch.

[1] https://lore.kernel.org/linux-block/89aedf1d-ae08-adef-db29-17e5bf85d054@grimberg.me/

Shin'ichiro Kawasaki (5):
  check: clean up _run_test()
  common,tests: rename unload_module() to _unload_module()
  check,common/rc: move _unload_module() from common/rc to check
  check,common/rc: load module in _have_driver() and unload after test
  Revert "nbd/rc: load nbd module explicitly"

 check                      | 36 +++++++++++++++++++++++++++++++-----
 common/multipath-over-rdma |  4 ++--
 common/rc                  | 26 ++++++++++----------------
 tests/nbd/rc               | 12 ++----------
 tests/nvmeof-mp/rc         | 12 ++++++------
 tests/srp/rc               |  8 ++++----
 6 files changed, 55 insertions(+), 43 deletions(-)

-- 
2.37.1

[PATCH blktests 1/5] check: clean up _run_test()

[Shin'ichiro Kawasaki]

Avoid duplicated declarations and returns of local variable 'ret' in
_run_test(). This is a preparation for a following commit.

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

diff --git a/check b/check
index e6c321c..85e0569 100755
--- a/check
+++ b/check
@@ -447,6 +447,8 @@ _run_test() {
 	RUN_FOR_ZONED=0
 	FALLBACK_DEVICE=0
 
+	local ret=0
+
 	# Ensure job control monitor mode is off in the sub-shell for test case
 	# runs to suppress job status output.
 	set +m
@@ -461,14 +463,13 @@ _run_test() {
 
 		RESULTS_DIR="$OUTPUT/nodev"
 		_call_test test
-		local ret=$?
+		ret=$?
 		if (( RUN_ZONED_TESTS && CAN_BE_ZONED )); then
 			RESULTS_DIR="$OUTPUT/nodev_zoned"
 			RUN_FOR_ZONED=1
 			_call_test test
 			ret=$(( ret || $? ))
 		fi
-		return $ret
 	else
 		if [[ ${#TEST_DEVS[@]} -eq 0 ]] && \
 			declare -fF fallback_device >/dev/null; then
@@ -494,7 +495,6 @@ _run_test() {
 			requires
 		fi
 
-		local ret=0
 		for TEST_DEV in "${TEST_DEVS[@]}"; do
 			TEST_DEV_SYSFS="${TEST_DEV_SYSFS_DIRS["$TEST_DEV"]}"
 			TEST_DEV_PART_SYSFS="${TEST_DEV_PART_SYSFS_DIRS["$TEST_DEV"]}"
@@ -523,9 +523,9 @@ _run_test() {
 			unset "TEST_DEV_PART_SYSFS_DIRS[${TEST_DEVS[0]}]"
 			TEST_DEVS=()
 		fi
-
-		return $ret
 	fi
+
+	return $ret
 }
 
 _run_group() {
-- 
2.37.1

[PATCH blktests 2/5] common,tests: rename unload_module() to _unload_module()

[Shin'ichiro Kawasaki]

All helper functions in common/rc have underscore prefix except
unload_module(). Add the prefix to it.

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 common/multipath-over-rdma |  4 ++--
 common/rc                  |  2 +-
 tests/nvmeof-mp/rc         | 12 ++++++------
 tests/srp/rc               |  8 ++++----
 4 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/common/multipath-over-rdma b/common/multipath-over-rdma
index c1c2e7c..fb820d6 100644
--- a/common/multipath-over-rdma
+++ b/common/multipath-over-rdma
@@ -428,11 +428,11 @@ stop_soft_rdma() {
 		      echo "$i ..."
 		      rdma link del "${i}" || echo "Failed to remove ${i}"
 		done
-	if ! unload_module rdma_rxe 10; then
+	if ! _unload_module rdma_rxe 10; then
 		echo "Unloading rdma_rxe failed"
 		return 1
 	fi
-	if ! unload_module siw 10; then
+	if ! _unload_module siw 10; then
 		echo "Unloading siw failed"
 		return 1
 	fi
diff --git a/common/rc b/common/rc
index be69a4d..738a32f 100644
--- a/common/rc
+++ b/common/rc
@@ -385,7 +385,7 @@ _uptime_s() {
 }
 
 # Arguments: module to unload ($1) and retry count ($2).
-unload_module() {
+_unload_module() {
 	local i m=$1 rc=${2:-1}
 
 	[ ! -e "/sys/module/$m" ] && return 0
diff --git a/tests/nvmeof-mp/rc b/tests/nvmeof-mp/rc
index ed27b5c..4238a4c 100755
--- a/tests/nvmeof-mp/rc
+++ b/tests/nvmeof-mp/rc
@@ -162,12 +162,12 @@ start_nvme_client() {
 }
 
 stop_nvme_client() {
-	unload_module nvme-rdma || return $?
-	unload_module nvme-fabrics || return $?
+	_unload_module nvme-rdma || return $?
+	_unload_module nvme-fabrics || return $?
 	# Ignore nvme and nvme-core unload errors - this test may be run on a
 	# system equipped with one or more NVMe SSDs.
-	unload_module nvme >&/dev/null
-	unload_module nvme-core >&/dev/null
+	_unload_module nvme >&/dev/null
+	_unload_module nvme-core >&/dev/null
 	return 0
 }
 
@@ -267,8 +267,8 @@ stop_nvme_target() {
 				rmdir "$d"
 			done
 	)
-	unload_module nvmet_rdma &&
-		unload_module nvmet &&
+	_unload_module nvmet_rdma &&
+		_unload_module nvmet &&
 		_exit_null_blk
 }
 
diff --git a/tests/srp/rc b/tests/srp/rc
index 23f87e4..55b535a 100755
--- a/tests/srp/rc
+++ b/tests/srp/rc
@@ -325,14 +325,14 @@ stop_srp_ini() {
 	log_out
 	for ((i=40;i>=0;i--)); do
 		remove_mpath_devs || return $?
-		unload_module ib_srp >/dev/null 2>&1 && break
+		_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 $?
+	_unload_module scsi_transport_srp || return $?
 }
 
 # Associate the LIO device with name $1/$2 with file $3 and SCSI serial $4.
@@ -491,7 +491,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
+	_unload_module ib_srpt
 	modprobe ib_srpt "${opts[@]}" || return $?
 	i=0
 	for r in "${vdev_path[@]}"; do
@@ -553,7 +553,7 @@ stop_lio_srpt() {
 			 target_core_file target_core_stgt target_core_user \
 			 target_core_mod
 	do
-		unload_module $m 10 || return $?
+		_unload_module $m 10 || return $?
 	done
 }
 
-- 
2.37.1

[PATCH blktests 3/5] check,common/rc: move _unload_module() from common/rc to check

[Shin'ichiro Kawasaki]

To use in the 'check' script in the following commit, move the helper
function _unload_module() from 'common/rc' to 'check'.

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 check     | 13 +++++++++++++
 common/rc | 13 -------------
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/check b/check
index 85e0569..5f57386 100755
--- a/check
+++ b/check
@@ -439,6 +439,19 @@ _test_dev_is_zoned() {
 	   $(cat "${TEST_DEV_SYSFS}/queue/zoned") != none ]]
 }
 
+# Arguments: module to unload ($1) and retry count ($2).
+_unload_module() {
+	local i m=$1 rc=${2:-1}
+
+	[ ! -e "/sys/module/$m" ] && return 0
+	for ((i=rc;i>0;i--)); do
+		modprobe -r "$m"
+		[ ! -e "/sys/module/$m" ] && return 0
+		sleep .1
+	done
+	return 1
+}
+
 _run_test() {
 	TEST_NAME="$1"
 	CAN_BE_ZONED=0
diff --git a/common/rc b/common/rc
index 738a32f..9bc0dbc 100644
--- a/common/rc
+++ b/common/rc
@@ -384,19 +384,6 @@ _uptime_s() {
 	awk '{ print int($1) }' /proc/uptime
 }
 
-# Arguments: module to unload ($1) and retry count ($2).
-_unload_module() {
-	local i m=$1 rc=${2:-1}
-
-	[ ! -e "/sys/module/$m" ] && return 0
-	for ((i=rc;i>0;i--)); do
-		modprobe -r "$m"
-		[ ! -e "/sys/module/$m" ] && return 0
-		sleep .1
-	done
-	return 1
-}
-
 _have_writeable_kmsg() {
 	if [[ ! -w /dev/kmsg ]]; then
 		SKIP_REASONS+=("cannot write to /dev/kmsg")
-- 
2.37.1

[PATCH blktests 4/5] check,common/rc: load module in _have_driver() and unload after test

[Shin'ichiro Kawasaki]

The commit 06a0ba866d90 ("common/rc: avoid module load in
_have_driver()") removed module load from _have_driver(). However, it
was pointed out no module load in _have_driver() is confusing and adds
complexity [1]. It requires explicit module loads and unloads in number
of test cases. The module unloads must be checked if unload is safe or
not. Also module load error must be handled. To avoid these complexity,
a new helper function would be required, but it will be look like the
_have_driver() with module load.

Then revert back the feature to load module in _have_driver(). To
address the issue that the commit 06a0ba866d90 tried to fix, record the
modules loaded by _have_driver() in MODULES_TO_UNLOAD array. Unload
the recorded modules after each test case processing completed. This
avoids the side-effect by the modules loaded by _have_driver().

[1] https://lore.kernel.org/linux-block/89aedf1d-ae08-adef-db29-17e5bf85d054@grimberg.me/

Fixes: 06a0ba866d90 ("common/rc: avoid module load in _have_driver()")
Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 check     | 13 +++++++++++++
 common/rc | 13 ++++++++++---
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/check b/check
index 5f57386..34e96c4 100755
--- a/check
+++ b/check
@@ -452,6 +452,16 @@ _unload_module() {
 	return 1
 }
 
+_unload_modules() {
+	local i
+
+	for ((i=${#MODULES_TO_UNLOAD[@]}; i > 0; i--)); do
+		_unload_module "${MODULES_TO_UNLOAD[i-1]}" 10
+	done
+
+	unset MODULES_TO_UNLOAD
+}
+
 _run_test() {
 	TEST_NAME="$1"
 	CAN_BE_ZONED=0
@@ -459,6 +469,7 @@ _run_test() {
 	DMESG_FILTER="cat"
 	RUN_FOR_ZONED=0
 	FALLBACK_DEVICE=0
+	MODULES_TO_UNLOAD=()
 
 	local ret=0
 
@@ -538,6 +549,8 @@ _run_test() {
 		fi
 	fi
 
+	_unload_modules
+
 	return $ret
 }
 
diff --git a/common/rc b/common/rc
index 9bc0dbc..a764b57 100644
--- a/common/rc
+++ b/common/rc
@@ -43,17 +43,24 @@ _module_file_exists()
 }
 
 # Check that the specified module or driver is available, regardless of whether
-# it is built-in or built separately as a module.
+# it is built-in or built separately as a module. Load the module if it is
+# loadable and not yet loaded. In this case, the loaded module is unloaded at
+# test case end regardless of whether the test case is skipped or executed.
 _have_driver()
 {
 	local modname="${1//-/_}"
 
-	if [ ! -d "/sys/module/${modname}" ] &&
-		   ! modprobe -qn "${modname}"; then
+	if [ -d "/sys/module/${modname}" ]; then
+		return 0
+	fi
+
+	if ! modprobe -q "${modname}"; then
 		SKIP_REASONS+=("driver ${modname} is not available")
 		return 1
 	fi
 
+	MODULES_TO_UNLOAD+=("${modname}")
+
 	return 0
 }
 
-- 
2.37.1

[PATCH blktests 5/5] Revert "nbd/rc: load nbd module explicitly"

[Shin'ichiro Kawasaki]

This reverts commit 78271b8bb8c939e1d0b9cfa3ea321a4ed06635bd.

Once I thought explicit nbd module load in nbd/rc is required due to the
commit 06a0ba866d90 ("common/rc: avoid module load in _have_driver()").
However, it was not a good solution and _have_driver() was modified
again to load module. Hence, revert explicit nbd module load in nbd/rc.

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 tests/nbd/rc | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/tests/nbd/rc b/tests/nbd/rc
index 32eea45..9c1c15b 100644
--- a/tests/nbd/rc
+++ b/tests/nbd/rc
@@ -28,21 +28,17 @@ _have_nbd() {
 }
 
 _have_nbd_netlink() {
-	local ret=0
-
 	if ! _have_nbd; then
 		return 1
 	fi
 	if ! _have_program genl-ctrl-list; then
 		return 1
 	fi
-	modprobe -q nbd
 	if ! genl-ctrl-list | grep -q nbd; then
 		SKIP_REASONS+=("nbd does not support netlink")
-		ret=1
+		return 1
 	fi
-	modprobe -qr nbd
-	return $ret
+	return 0
 }
 
 _wait_for_nbd_connect() {
@@ -66,7 +62,6 @@ _wait_for_nbd_disconnect() {
 }
 
 _start_nbd_server() {
-	modprobe -q nbd
 	truncate -s 10G "${TMPDIR}/export"
 	cat > "${TMPDIR}/nbd.conf" << EOF
 [generic]
@@ -78,20 +73,17 @@ EOF
 
 _stop_nbd_server() {
 	kill -SIGTERM "$(cat "${TMPDIR}/nbd.pid")"
-	modprobe -qr nbd
 	rm -f "${TMPDIR}/nbd.pid"
 	rm -f "${TMPDIR}/export"
 }
 
 _start_nbd_server_netlink() {
-	modprobe -q nbd
 	truncate -s 10G "${TMPDIR}/export"
 	nbd-server 8000 "${TMPDIR}/export" >/dev/null 2>&1
 }
 
 _stop_nbd_server_netlink() {
 	killall -SIGTERM nbd-server
-	modprobe -qr nbd
 	rm -f "${TMPDIR}/export"
 }
 
-- 
2.37.1