[PATCH blktests v3 0/6] fix module check issues

[PATCH blktests v3 0/6] fix module check issues

[Shin'ichiro Kawasaki]

Current blktests have unexpected test case failures caused by module
availability checks. This series addresses two issues related to module check
and fix the failures.

The first issue is caused by module load by _have_driver(). When this helper
function checks the specified module (or driver) is available, it loads the
module using modprobe command. It leaves the module loaded and affects following
test cases. The first patch addresses this issue. The second patch avoids side
affects of the first patch on nbd test cases.

The second issue is in _have_modules(). Recently, _have_driver() helper function
was introduced to provide similar but different feature from _have_modules().
However, it turned out that _have_modules() is not working as expected and does
exactly same check as _have_driver(). The third patch fixes _have_module() to
work as expected. This change makes block/001 and srp test group skipped.
Following three patches adjust skip conditions not to skip the test cases.

Changes from v2:
* 1st patch: kept single bracket style
* 3rd patch: replaced find -or option with -o
* 4th patch: adjusted to the 1st patch change
* 6th patch: simplified _have_drivers() arguments

Changes from v1:
* 1st patch: added Reviewed-by tag
* 4th patch: added changes in new test cases

Shin'ichiro Kawasaki (6):
  common/rc: avoid module load in _have_driver()
  nbd/rc: load nbd module explicitly
  common/rc: ensure modules are loadable in _have_modules()
  common,tests: replace _have_driver() with _have_drivers()
  block/001: use _have_drivers() in place of _have_modules()
  srp/rc: allow test with built-in sd_mod and sg drivers

 common/null_blk |  2 +-
 common/rc       | 43 ++++++++++++++++++++++++++++++++++++-------
 tests/block/001 |  3 ++-
 tests/nbd/rc    | 14 +++++++++++---
 tests/nvme/rc   | 12 +++++-------
 tests/scsi/rc   |  2 +-
 tests/srp/rc    |  4 +---
 tests/zbd/009   |  2 +-
 tests/zbd/010   |  2 +-
 9 files changed, 59 insertions(+), 25 deletions(-)

-- 
2.37.1

[PATCH blktests v3 1/6] common/rc: avoid module load in _have_driver()

[Shin'ichiro Kawasaki]

The helper function _have_driver() checks availability of the specified
driver, or module, regardless whether it is loadable or not. When the
driver is loadable, it loads the module for checking, but does not
unload it. This makes following test cases fail.

Such failure happens when nvmeof-mp test group is executed after nvme
test group with tcp transport. _have_driver() for tcp transport loads
nvmet and nvmet-tcp modules. nvmeof-mp test group tries to unload the
nvmet module but it fails because of dependency to the nvmet-tcp module.

To avoid the failure, do not load module in _have_driver() using -n
dry run option of the modprobe command. While at it, fix a minor problem
of modname '-' replacement. Currently, only the first '-' in modname is
replaced with '_'. Replace all '-'s.

Fixes: e9645877fbf0 ("common: add a helper if a driver is available")
Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 common/rc | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/common/rc b/common/rc
index 01df6fa..5b34c60 100644
--- a/common/rc
+++ b/common/rc
@@ -30,9 +30,10 @@ _have_root() {
 
 _have_driver()
 {
-	local modname="${1/-/_}"
+	local modname="${1//-/_}"
 
-	if [ ! -d "/sys/module/${modname}" ] && ! modprobe -q "${modname}"; then
+	if [ ! -d "/sys/module/${modname}" ] &&
+		   ! modprobe -qn "${modname}"; then
 		SKIP_REASONS+=("driver ${modname} is not available")
 		return 1
 	fi
-- 
2.37.1

[PATCH blktests v3 2/6] nbd/rc: load nbd module explicitly

[Shin'ichiro Kawasaki]

After the commit "common/rc: avoid module load in _have_driver()",
_have_driver() no longer loads specified module. However, nbd test cases
and _have_nbd_netlink() function assume that the module is loaded by
calling _have_driver(). This causes test case failures and unexpected
skips. To fix them, load and unload modules explicitly in functions
_start_nbd_server*(), _stop_nbd_server*() and _have_nbd_netlink().

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

diff --git a/tests/nbd/rc b/tests/nbd/rc
index 9c1c15b..32eea45 100644
--- a/tests/nbd/rc
+++ b/tests/nbd/rc
@@ -28,17 +28,21 @@ _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")
-		return 1
+		ret=1
 	fi
-	return 0
+	modprobe -qr nbd
+	return $ret
 }
 
 _wait_for_nbd_connect() {
@@ -62,6 +66,7 @@ _wait_for_nbd_disconnect() {
 }
 
 _start_nbd_server() {
+	modprobe -q nbd
 	truncate -s 10G "${TMPDIR}/export"
 	cat > "${TMPDIR}/nbd.conf" << EOF
 [generic]
@@ -73,17 +78,20 @@ 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

[PATCH blktests v3 3/6] common/rc: ensure modules are loadable in _have_modules()

[Shin'ichiro Kawasaki]

The commit e9645877fbf0 ("common: add a helper if a driver is
available") introduced the helper function _have_driver() to check the
driver or module is available no matter whether it is a loadable module
or built-in module. It was assumed that _have_modules() whould check
that specified modules are loadable and not built-in.

However, the function _have_modules() returns true even if the specified
modules are built-in and not loadable. This causes failures of some test
cases on test system with built-in modules such as nbd/004. It also
means that _have_modules() and _have_driver() have same functionality.

To avoid the unexpected failures, fix _have_modules() to return false
when the specified modules are built-in. Check if loadable module file
exists by searching the module file path. If the module file does not
exist, return false. Also add comments to describe the difference
between _have_driver() and _have_modules().

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 common/rc | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/common/rc b/common/rc
index 5b34c60..8681a46 100644
--- a/common/rc
+++ b/common/rc
@@ -28,6 +28,22 @@ _have_root() {
 	return 0
 }
 
+_module_file_exists()
+{
+	local ko_underscore=${1//-/_}.ko
+	local ko_hyphen=${1//_/-}.ko
+	local libpath
+	local -i count
+
+	libpath="/lib/modules/$(uname -r)/kernel"
+	count=$(find "$libpath" -name "$ko_underscore" -o \
+		     -name "$ko_hyphen" | wc -l)
+	((count)) && return 0
+	return 1
+}
+
+# Check that the specified module or driver is available, regardless of whether
+# it is built-in or built separately as a module.
 _have_driver()
 {
 	local modname="${1//-/_}"
@@ -41,12 +57,14 @@ _have_driver()
 	return 0
 }
 
+# Check that the specified modules are available as loadable modules and not
+# built-in the kernel.
 _have_modules() {
 	local missing=()
 	local module
 
 	for module in "$@"; do
-		if ! modprobe -n -q "$module"; then
+		if ! _module_file_exists "${module}"; then
 			missing+=("$module")
 		fi
 	done
-- 
2.37.1

[PATCH blktests v3 4/6] common,tests: replace _have_driver() with _have_drivers()

[Shin'ichiro Kawasaki]

The helper function _have_driver() checks single driver but it is often
required to check multiple drivers. Rename _have_driver() to
_have_drivers() and improve it to check multiple drivers. This makes its
usage consistent with _have_modules().

While at it, replace single brackets in _have_drivers() with double
brackets to have consistent bracket style as _have_modules().

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 common/null_blk |  2 +-
 common/rc       | 30 ++++++++++++++++++++----------
 tests/nbd/rc    |  2 +-
 tests/nvme/rc   | 12 +++++-------
 tests/scsi/rc   |  2 +-
 tests/zbd/009   |  2 +-
 tests/zbd/010   |  2 +-
 7 files changed, 30 insertions(+), 22 deletions(-)

diff --git a/common/null_blk b/common/null_blk
index 52eb486..15bb35e 100644
--- a/common/null_blk
+++ b/common/null_blk
@@ -7,7 +7,7 @@
 . common/shellcheck
 
 _have_null_blk() {
-	_have_driver null_blk
+	_have_drivers null_blk
 }
 
 _remove_null_blk_devices() {
diff --git a/common/rc b/common/rc
index 8681a46..fdeda24 100644
--- a/common/rc
+++ b/common/rc
@@ -42,15 +42,25 @@ _module_file_exists()
 	return 1
 }
 
-# Check that the specified module or driver is available, regardless of whether
-# it is built-in or built separately as a module.
-_have_driver()
-{
-	local modname="${1//-/_}"
+# Check that the specified modules or drivers are available, regardless of
+# whether they are built-in or built separately as module files.
+_have_drivers() {
+	local missing=()
+	local driver modname
+
+	for driver in "$@"; do
+		modname=${driver//-/_}
+		if [[ ! -d "/sys/module/${modname}" ]] &&
+			   ! modprobe -qn "${modname}"; then
+			missing+=("$driver")
+		fi
+	done
 
-	if [ ! -d "/sys/module/${modname}" ] &&
-		   ! modprobe -qn "${modname}"; then
-		SKIP_REASONS+=("driver ${modname} is not available")
+	if [[ ${#missing} -gt 1 ]]; then
+		SKIP_REASONS+=("the following drivers are not available: ${missing[*]}")
+		return 1
+	elif [[ ${#missing} -eq 1 ]]; then
+		SKIP_REASONS+=("${missing[0]} driver is not available")
 		return 1
 	fi
 
@@ -98,7 +108,7 @@ _have_module_param_value() {
 	local expected_value="$3"
 	local value
 
-	if ! _have_driver "$modname"; then
+	if ! _have_drivers "$modname"; then
 		return 1;
 	fi
 
@@ -147,7 +157,7 @@ _have_src_program() {
 }
 
 _have_loop() {
-	_have_driver loop && _have_program losetup
+	_have_drivers loop && _have_program losetup
 }
 
 _have_blktrace() {
diff --git a/tests/nbd/rc b/tests/nbd/rc
index 32eea45..d3ec084 100644
--- a/tests/nbd/rc
+++ b/tests/nbd/rc
@@ -11,7 +11,7 @@ group_requires() {
 }
 
 _have_nbd() {
-	if ! _have_driver nbd; then
+	if ! _have_drivers nbd; then
 		return 1
 	fi
 	if ! _have_program nbd-server; then
diff --git a/tests/nvme/rc b/tests/nvme/rc
index ff13ea2..df13548 100644
--- a/tests/nvme/rc
+++ b/tests/nvme/rc
@@ -18,23 +18,21 @@ _nvme_requires() {
 	_have_program nvme
 	case ${nvme_trtype} in
 	loop)
-		_have_driver nvme-loop
+		_have_drivers nvme-loop
 		_have_configfs
 		;;
 	pci)
-		_have_driver nvme
+		_have_drivers nvme
 		;;
 	tcp)
-		_have_driver nvme-tcp
-		_have_driver nvmet-tcp
+		_have_drivers nvme-tcp nvmet-tcp
 		_have_configfs
 		;;
 	rdma)
-		_have_driver nvme-rdma
-		_have_driver nvmet-rdma
+		_have_drivers nvme-rdma nvmet-rdma
 		_have_configfs
 		_have_program rdma
-		_have_driver rdma_rxe || _have_driver siw
+		_have_drivers rdma_rxe || _have_drivers siw
 		;;
 	*)
 		SKIP_REASONS+=("unsupported nvme_trtype=${nvme_trtype}")
diff --git a/tests/scsi/rc b/tests/scsi/rc
index 3b4a33c..d9750c0 100644
--- a/tests/scsi/rc
+++ b/tests/scsi/rc
@@ -15,7 +15,7 @@ group_device_requires() {
 }
 
 _have_scsi_generic() {
-	_have_driver sg
+	_have_drivers sg
 }
 
 _require_test_dev_is_scsi() {
diff --git a/tests/zbd/009 b/tests/zbd/009
index 483cbf6..4d6a637 100755
--- a/tests/zbd/009
+++ b/tests/zbd/009
@@ -33,7 +33,7 @@ have_good_mkfs_btrfs() {
 
 requires() {
 	_have_fio
-	_have_driver btrfs
+	_have_drivers btrfs
 	_have_module_param scsi_debug zone_cap_mb
 	_have_program mkfs.btrfs
 	_have_scsi_debug
diff --git a/tests/zbd/010 b/tests/zbd/010
index 6d634b0..ef5e0fc 100644
--- a/tests/zbd/010
+++ b/tests/zbd/010
@@ -11,7 +11,7 @@ QUICK=1
 
 requires() {
 	_have_fio
-	_have_driver f2fs
+	_have_drivers f2fs
 	_have_modules null_blk
 	_have_module_param scsi_debug zone_cap_mb
 	_have_program mkfs.f2fs
-- 
2.37.1

[PATCH blktests v3 5/6] block/001: use _have_drivers() in place of _have_modules()

[Shin'ichiro Kawasaki]

The drivers sd_mod and sr_mod do not need to be loadable. Replace the
check with _have_drivers() and allow test with built-in modules.

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 tests/block/001 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/block/001 b/tests/block/001
index 5f05fa8..a84d0a1 100755
--- a/tests/block/001
+++ b/tests/block/001
@@ -13,7 +13,8 @@ DESCRIPTION="stress device hotplugging"
 TIMED=1
 
 requires() {
-	_have_scsi_debug && _have_modules sd_mod sr_mod
+	_have_scsi_debug
+	_have_drivers sd_mod sr_mod
 }
 
 stress_scsi_debug() {
-- 
2.37.1

[PATCH blktests v3 6/6] srp/rc: allow test with built-in sd_mod and sg drivers

[Shin'ichiro Kawasaki]

The srp test group can be executed with built-in sd_mod and sg drivers.
Check the drivers with _have_drivers() in place of _have_modules.

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 tests/srp/rc | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/tests/srp/rc b/tests/srp/rc
index 94ee97c..a3a96a7 100755
--- a/tests/srp/rc
+++ b/tests/srp/rc
@@ -35,6 +35,7 @@ group_requires() {
 		SKIP_REASONS+=("LIO must be unloaded before the SRP tests are run")
 		return
 	fi
+	_have_drivers sd_mod sg
 	required_modules=(
 		dm_multipath
 		dm_queue_length
@@ -51,9 +52,6 @@ group_requires() {
 		scsi_dh_alua
 		scsi_dh_emc
 		scsi_dh_rdac
-		sd_mod
-		sd_mod
-		sg
 		target_core_iblock
 		target_core_mod
 	)
-- 
2.37.1

Re: [PATCH blktests v3 6/6] srp/rc: allow test with built-in sd_mod and sg drivers

[Bart Van Assche]

On 8/19/22 02:39, Shin'ichiro Kawasaki wrote:
> The srp test group can be executed with built-in sd_mod and sg drivers.
> Check the drivers with _have_drivers() in place of _have_modules.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH blktests v3 1/6] common/rc: avoid module load in _have_driver()

[Bart Van Assche]

On 8/19/22 02:39, Shin'ichiro Kawasaki wrote:
> The helper function _have_driver() checks availability of the specified
> driver, or module, regardless whether it is loadable or not. When the
> driver is loadable, it loads the module for checking, but does not
> unload it. This makes following test cases fail.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH blktests v3 2/6] nbd/rc: load nbd module explicitly

[Christoph Hellwig]

On Fri, Aug 19, 2022 at 06:39:16PM +0900, Shin'ichiro Kawasaki wrote:
> After the commit "common/rc: avoid module load in _have_driver()",
> _have_driver() no longer loads specified module. However, nbd test cases
> and _have_nbd_netlink() function assume that the module is loaded by
> calling _have_driver(). This causes test case failures and unexpected
> skips. To fix them, load and unload modules explicitly in functions
> _start_nbd_server*(), _stop_nbd_server*() and _have_nbd_netlink().

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH blktests v3 3/6] common/rc: ensure modules are loadable in _have_modules()

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH blktests v3 4/6] common,tests: replace _have_driver() with _have_drivers()

[Christoph Hellwig]

I always found _have_modules rather strange as multiple lines to check
for one driver at a time seems more readable. 

Re: [PATCH blktests v3 4/6] common,tests: replace _have_driver() with _have_drivers()

[Shinichiro Kawasaki]

On Aug 21, 2022 / 07:49, Christoph Hellwig wrote:
> I always found _have_modules rather strange as multiple lines to check
> for one driver at a time seems more readable. 

Okay, that will be a bit simpler. I'll drop this patch from the series. Instead,
will add a patch which will replace _have_modules with _have_module.

-- 
Shin'ichiro Kawasaki