[PATCH blktests 0/6] fix module check issues

[PATCH blktests 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.

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    | 10 ++++++----
 7 files changed, 62 insertions(+), 24 deletions(-)

-- 
2.36.1

[PATCH blktests 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>
---
 common/rc | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/common/rc b/common/rc
index 01df6fa..8150fee 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.36.1

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

[Christoph Hellwig]

On Wed, Jul 27, 2022 at 05:52:46PM +0900, 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.
> 
> 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.

Looks good:

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

[PATCH blktests 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.36.1

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

[Christoph Hellwig]

On Wed, Jul 27, 2022 at 05:52:47PM +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().

Did you test this with built-in nbd? 

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

[Shinichiro Kawasaki]

On Jul 28, 2022 / 16:46, Christoph Hellwig wrote:
> On Wed, Jul 27, 2022 at 05:52:47PM +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().
> 
> Did you test this with built-in nbd? 

Yes. I confirmed all test cases in nbd test group pass with built-in nbd.
The modprobe command with -q option does nothing for built-in drivers.

When I confirmed that, I found even nbd/004 passes, which should be skipped.
Next patch addresses this issue.

-- 
Shin'ichiro Kawasaki

[PATCH blktests 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 8150fee..ee2289c 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" -or \
+		     -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.36.1

[PATCH blktests 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().

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 +-
 5 files changed, 28 insertions(+), 20 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 ee2289c..4b01fc3 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() {
-- 
2.36.1

[PATCH blktests 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.36.1

[PATCH blktests 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 | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/tests/srp/rc b/tests/srp/rc
index 94ee97c..46c75c6 100755
--- a/tests/srp/rc
+++ b/tests/srp/rc
@@ -28,13 +28,18 @@ is_lio_configured() {
 }
 
 group_requires() {
-	local m name p required_modules
+	local m name p required_drivers required_modules
 
 	_have_configfs || return
 	if is_lio_configured; then
 		SKIP_REASONS+=("LIO must be unloaded before the SRP tests are run")
 		return
 	fi
+	required_drivers=(
+		sd_mod
+		sg
+	)
+	_have_drivers "${required_drivers[@]}"
 	required_modules=(
 		dm_multipath
 		dm_queue_length
@@ -51,9 +56,6 @@ group_requires() {
 		scsi_dh_alua
 		scsi_dh_emc
 		scsi_dh_rdac
-		sd_mod
-		sd_mod
-		sg
 		target_core_iblock
 		target_core_mod
 	)
-- 
2.36.1