reduce the dependency on modules

reduce the dependency on modules

[Christoph Hellwig]

Hi Shin'ichiro,

this series reduces the dependency of blktests on modular builds
of various block drivers.  There are still plenty of tests that
do require modules, mostly because a lot of scsi_debug and null_blk
can only be set at load time, but I plan to address those in the
kernel soon.

[PATCH blktests 1/9] nvme: use _have_loop instead of _have_modules loop

[Christoph Hellwig]

Also check for the losetup existance.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 tests/nvme/002 | 2 +-
 tests/nvme/003 | 2 +-
 tests/nvme/004 | 2 +-
 tests/nvme/005 | 3 +--
 tests/nvme/006 | 2 +-
 tests/nvme/008 | 2 +-
 tests/nvme/010 | 2 +-
 tests/nvme/012 | 2 +-
 tests/nvme/014 | 2 +-
 tests/nvme/015 | 2 +-
 tests/nvme/018 | 2 +-
 tests/nvme/019 | 2 +-
 tests/nvme/021 | 2 +-
 tests/nvme/022 | 2 +-
 tests/nvme/023 | 2 +-
 tests/nvme/024 | 2 +-
 tests/nvme/025 | 2 +-
 tests/nvme/026 | 2 +-
 tests/nvme/027 | 2 +-
 tests/nvme/028 | 2 +-
 tests/nvme/029 | 2 +-
 tests/nvme/030 | 2 +-
 tests/nvme/031 | 2 +-
 23 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/tests/nvme/002 b/tests/nvme/002
index ca11c11..6c6ae5f 100755
--- a/tests/nvme/002
+++ b/tests/nvme/002
@@ -11,7 +11,7 @@ DESCRIPTION="create many subsystems and test discovery"
 
 requires() {
 	_nvme_requires
-	_have_modules loop
+	_have_loop
 	_require_nvme_trtype_is_loop
 }
 
diff --git a/tests/nvme/003 b/tests/nvme/003
index 101c184..2ba6954 100755
--- a/tests/nvme/003
+++ b/tests/nvme/003
@@ -12,7 +12,7 @@ QUICK=1
 
 requires() {
 	_nvme_requires
-	_have_modules loop
+	_have_loop
 	_require_nvme_trtype_is_fabrics
 }
 
diff --git a/tests/nvme/004 b/tests/nvme/004
index 4b0b7ae..9dda538 100755
--- a/tests/nvme/004
+++ b/tests/nvme/004
@@ -13,7 +13,7 @@ QUICK=1
 
 requires() {
 	_nvme_requires
-	_have_modules loop
+	_have_loop
 	_require_nvme_trtype_is_fabrics
 }
 
diff --git a/tests/nvme/005 b/tests/nvme/005
index 9f3e388..de567a7 100755
--- a/tests/nvme/005
+++ b/tests/nvme/005
@@ -12,8 +12,7 @@ QUICK=1
 
 requires() {
 	_nvme_requires
-	_have_modules loop && \
-		_have_module_param_value nvme_core multipath Y
+	_have_loop && _have_module_param_value nvme_core multipath Y
 	_require_nvme_trtype_is_fabrics
 }
 
diff --git a/tests/nvme/006 b/tests/nvme/006
index 9230dc6..d993861 100755
--- a/tests/nvme/006
+++ b/tests/nvme/006
@@ -11,7 +11,7 @@ QUICK=1
 
 requires() {
 	_nvme_requires
-	_have_modules loop
+	_have_loop
 	_require_nvme_trtype_is_fabrics
 }
 
diff --git a/tests/nvme/008 b/tests/nvme/008
index 219fe9b..5568fe4 100755
--- a/tests/nvme/008
+++ b/tests/nvme/008
@@ -11,7 +11,7 @@ QUICK=1
 
 requires() {
 	_nvme_requires
-	_have_modules loop
+	_have_loop
 	_require_nvme_trtype_is_fabrics
 }
 
diff --git a/tests/nvme/010 b/tests/nvme/010
index 08e39d5..b7b1d51 100755
--- a/tests/nvme/010
+++ b/tests/nvme/010
@@ -11,7 +11,7 @@ TIMED=1
 
 requires() {
 	_nvme_requires
-	_have_fio && _have_modules loop
+	_have_fio && _have_loop
 	_require_nvme_trtype_is_fabrics
 }
 
diff --git a/tests/nvme/012 b/tests/nvme/012
index 6bb4972..c9d2438 100755
--- a/tests/nvme/012
+++ b/tests/nvme/012
@@ -14,7 +14,7 @@ requires() {
 	_nvme_requires
 	_have_xfs
 	_have_fio
-	_have_modules loop
+	_have_loop
 	_require_nvme_trtype_is_fabrics
 }
 
diff --git a/tests/nvme/014 b/tests/nvme/014
index 48f8caa..d13cff7 100755
--- a/tests/nvme/014
+++ b/tests/nvme/014
@@ -11,7 +11,7 @@ QUICK=1
 
 requires() {
 	_nvme_requires
-	_have_modules loop
+	_have_loop
 	_require_nvme_trtype_is_fabrics
 }
 
diff --git a/tests/nvme/015 b/tests/nvme/015
index e33cfde..bb52ba2 100755
--- a/tests/nvme/015
+++ b/tests/nvme/015
@@ -11,7 +11,7 @@ QUICK=1
 
 requires() {
 	_nvme_requires
-	_have_modules loop
+	_have_loop
 	_require_nvme_trtype_is_fabrics
 }
 
diff --git a/tests/nvme/018 b/tests/nvme/018
index 7f407da..315e795 100755
--- a/tests/nvme/018
+++ b/tests/nvme/018
@@ -12,7 +12,7 @@ QUICK=1
 
 requires() {
 	_nvme_requires
-	_have_modules loop
+	_have_loop
 	_require_nvme_trtype_is_fabrics
 }
 
diff --git a/tests/nvme/019 b/tests/nvme/019
index 8259e2e..4cb3509 100755
--- a/tests/nvme/019
+++ b/tests/nvme/019
@@ -11,7 +11,7 @@ QUICK=1
 
 requires() {
 	_nvme_requires
-	_have_modules loop
+	_have_loop
 	_require_nvme_trtype_is_fabrics
 }
 
diff --git a/tests/nvme/021 b/tests/nvme/021
index fb77f9c..6ee0af1 100755
--- a/tests/nvme/021
+++ b/tests/nvme/021
@@ -11,7 +11,7 @@ QUICK=1
 
 requires() {
 	_nvme_requires
-	_have_modules loop
+	_have_loop
 	_require_nvme_trtype_is_fabrics
 }
 
diff --git a/tests/nvme/022 b/tests/nvme/022
index 62c4690..1d76ffa 100755
--- a/tests/nvme/022
+++ b/tests/nvme/022
@@ -11,7 +11,7 @@ QUICK=1
 
 requires() {
 	_nvme_requires
-	_have_modules loop
+	_have_loop
 	_require_nvme_trtype_is_fabrics
 }
 
diff --git a/tests/nvme/023 b/tests/nvme/023
index bce21b5..b65be07 100755
--- a/tests/nvme/023
+++ b/tests/nvme/023
@@ -11,7 +11,7 @@ QUICK=1
 
 requires() {
 	_nvme_requires
-	_have_modules loop
+	_have_loop
 	_require_nvme_trtype_is_fabrics
 }
 
diff --git a/tests/nvme/024 b/tests/nvme/024
index ffec36c..f756797 100755
--- a/tests/nvme/024
+++ b/tests/nvme/024
@@ -11,7 +11,7 @@ QUICK=1
 
 requires() {
 	_nvme_requires
-	_have_modules loop
+	_have_loop
 	_require_nvme_trtype_is_fabrics
 }
 
diff --git a/tests/nvme/025 b/tests/nvme/025
index 3d3f01b..941bf36 100755
--- a/tests/nvme/025
+++ b/tests/nvme/025
@@ -11,7 +11,7 @@ QUICK=1
 
 requires() {
 	_nvme_requires
-	_have_modules loop
+	_have_loop
 	_require_nvme_trtype_is_fabrics
 }
 
diff --git a/tests/nvme/026 b/tests/nvme/026
index 2f56077..c3f06c2 100755
--- a/tests/nvme/026
+++ b/tests/nvme/026
@@ -11,7 +11,7 @@ QUICK=1
 
 requires() {
 	_nvme_requires
-	_have_modules loop
+	_have_loop
 	_require_nvme_trtype_is_fabrics
 }
 
diff --git a/tests/nvme/027 b/tests/nvme/027
index 53f0664..0ad663a 100755
--- a/tests/nvme/027
+++ b/tests/nvme/027
@@ -11,7 +11,7 @@ QUICK=1
 
 requires() {
 	_nvme_requires
-	_have_modules loop
+	_have_loop
 	_require_nvme_trtype_is_fabrics
 }
 
diff --git a/tests/nvme/028 b/tests/nvme/028
index 3d9084f..7de977a 100755
--- a/tests/nvme/028
+++ b/tests/nvme/028
@@ -11,7 +11,7 @@ QUICK=1
 
 requires() {
 	_nvme_requires
-	_have_modules loop
+	_have_loop
 	_require_nvme_trtype_is_fabrics
 }
 
diff --git a/tests/nvme/029 b/tests/nvme/029
index 960e5f5..f8b4cbb 100755
--- a/tests/nvme/029
+++ b/tests/nvme/029
@@ -12,7 +12,7 @@ QUICK=1
 
 requires() {
 	_nvme_requires
-	_have_modules loop
+	_have_loop
 	_require_nvme_trtype_is_fabrics
 }
 
diff --git a/tests/nvme/030 b/tests/nvme/030
index c6d485e..20fef69 100755
--- a/tests/nvme/030
+++ b/tests/nvme/030
@@ -11,7 +11,7 @@ QUICK=1
 
 requires() {
 	_nvme_requires
-	_have_modules loop
+	_have_loop
 	_require_nvme_trtype_is_fabrics
 }
 
diff --git a/tests/nvme/031 b/tests/nvme/031
index 7c18a64..4e17982 100755
--- a/tests/nvme/031
+++ b/tests/nvme/031
@@ -19,7 +19,7 @@ QUICK=1
 
 requires() {
 	_nvme_requires
-	_have_modules loop
+	_have_loop
 	_require_nvme_trtype_is_fabrics
 }
 
-- 
2.30.2

[PATCH blktests 2/9] common: add a helper if a driver is available

[Christoph Hellwig]

Unlike _have_modules this allows allows for a built-in driver.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 common/rc | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/common/rc b/common/rc
index 5e35e21..a93b227 100644
--- a/common/rc
+++ b/common/rc
@@ -28,6 +28,18 @@ _have_root() {
 	return 0
 }
 
+_have_driver()
+{
+	local modname="${1/-/_}"
+
+	if [ ! -d "/sys/module/${modname}" ] && ! modprobe -q ${modname}; then
+		SKIP_REASON="driver ${modname} is not available"
+		return 1
+	fi
+
+	return 0
+}
+
 _have_modules() {
 	local missing=()
 	local module
-- 
2.30.2

[PATCH blktests 3/9] common: fix _have_module_param_value to work with built-in drivers

[Christoph Hellwig]

Don't bother to call modprobe directly and just check the /sys/module/
directory.  Also switch to using descriptive variable names for the
paramters.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 common/rc | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/common/rc b/common/rc
index a93b227..ffd15b6 100644
--- a/common/rc
+++ b/common/rc
@@ -74,17 +74,22 @@ _have_module_param() {
 }
 
 _have_module_param_value() {
+	local modname="${1/-/_}"
+	local param="$2"
+	local expected_value="$3"
 	local value
 
-	modprobe "$1"
+	if ! have_driver $modname; then
+		return 1;
+	fi
 
-	if ! _have_module_param "$1" "$2"; then
+	if ! _have_module_param $modname $param; then
 		return 1
 	fi
 
-	value=$(cat "/sys/module/$1/parameters/$2")
-	if [[ "${value}" != "$3" ]]; then
-		SKIP_REASON="$1 module parameter $2 must be set to $3"
+	value=$(cat "/sys/module/$modname/parameters/$param")
+	if [[ "${value}" != "$expected_value" ]]; then
+		SKIP_REASON="$modname module parameter $param must be set to $expected_value"
 		return 1
 	fi
 
-- 
2.30.2

[PATCH blktests 4/9] common: do not require loop support to be modular

[Christoph Hellwig]

Use _have_driver instead of _have_modules in _have_loop as nothing requires
the loop driver to be modular.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 common/rc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/rc b/common/rc
index ffd15b6..d71a81e 100644
--- a/common/rc
+++ b/common/rc
@@ -128,7 +128,7 @@ _have_src_program() {
 }
 
 _have_loop() {
-	_have_modules loop && _have_program losetup
+	_have_driver loop && _have_program losetup
 }
 
 _have_blktrace() {
-- 
2.30.2

[PATCH blktests 5/9] common: do not require null_blk support to be modular

[Christoph Hellwig]

Use _have_driver instead of _have_modules in _have_null_blk for the basic
null_blk check, and instead only require an actual module in
_init_null_blk when specific module parameters are passed.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 common/null_blk | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/common/null_blk b/common/null_blk
index 6611db0..ccf3750 100644
--- a/common/null_blk
+++ b/common/null_blk
@@ -5,7 +5,7 @@
 # null_blk helper functions.
 
 _have_null_blk() {
-	_have_modules null_blk
+	_have_driver null_blk
 }
 
 _remove_null_blk_devices() {
@@ -16,15 +16,19 @@ _remove_null_blk_devices() {
 }
 
 _init_null_blk() {
-	_remove_null_blk_devices
+	local modparams="$@"
 
-	local zoned=""
-	if (( RUN_FOR_ZONED )); then zoned="zoned=1"; fi
+	if (( RUN_FOR_ZONED )); then
+		modparams="${modparams} zoned=1"
+	fi
 
-	if ! modprobe -r null_blk || ! modprobe null_blk "$@" "${zoned}" ; then
+	if [ -n "${modparams}" ] && ! _have_modules null_blk; then
 		return 1
 	fi
 
+	_remove_null_blk_devices
+	modprobe -qr null_blk && modprobe -q null_blk ${modparams}
+
 	udevadm settle
 	return 0
 }
@@ -46,5 +50,5 @@ _configure_null_blk() {
 _exit_null_blk() {
 	_remove_null_blk_devices
 	udevadm settle
-	modprobe -r null_blk
+	modprobe -qr null_blk
 }
-- 
2.30.2

[PATCH blktests 6/9] nbd: do not require nbd support to be modular

[Christoph Hellwig]

Use _have_driver instead of _have_modules in _have_nbd as nothing requires
the nbd driver to be modular.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 tests/nbd/rc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/nbd/rc b/tests/nbd/rc
index 9d0e3d1..118553c 100644
--- a/tests/nbd/rc
+++ b/tests/nbd/rc
@@ -7,11 +7,11 @@
 . common/rc
 
 group_requires() {
-	_have_root && _have_nbd && modprobe nbd
+	_have_root && _have_nbd
 }
 
 _have_nbd() {
-	if ! _have_modules nbd; then
+	if ! _have_driver nbd; then
 		return 1
 	fi
 	if ! _have_program nbd-server; then
-- 
2.30.2

[PATCH blktests 7/9] scsi: don't require sg to be built in

[Christoph Hellwig]

Use _have_driver instead of _have_modules in _have_scsi_generic as
nothing requires the sg driver to be modular.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 tests/scsi/rc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/scsi/rc b/tests/scsi/rc
index c8d2f42..0751e77 100644
--- a/tests/scsi/rc
+++ b/tests/scsi/rc
@@ -15,7 +15,7 @@ group_device_requires() {
 }
 
 _have_scsi_generic() {
-	_have_modules sg
+	_have_driver sg
 }
 
 _require_test_dev_is_scsi() {
-- 
2.30.2

[PATCH blktests 8/9] nvme: don't require the nvme drivers to be built in

[Christoph Hellwig]

Use _have_driver instead of _have_modules to check for the availability
of the nvme drivers, and don't bother checking at all for drivers that
are pulled in as dependencies.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 tests/nvme/rc | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/tests/nvme/rc b/tests/nvme/rc
index ccdccf9..998b181 100644
--- a/tests/nvme/rc
+++ b/tests/nvme/rc
@@ -16,21 +16,23 @@ _nvme_requires() {
 	_have_program nvme
 	case ${nvme_trtype} in
 	loop)
-		_have_modules nvmet nvme-core nvme-loop
+		_have_driver nvme-loop
 		_have_configfs
 		;;
 	pci)
-		_have_modules nvme nvme-core
+		_have_driver nvme
 		;;
 	tcp)
-		_have_modules nvmet nvme-core nvme-tcp nvmet-tcp
+		_have_driver nvme-tcp
+		_have_driver nvmet-tcp
 		_have_configfs
 		;;
 	rdma)
-		_have_modules nvmet nvme-core nvme-rdma nvmet-rdma
+		_have_driver nvme-rdma
+		_have_driver nvmet-rdma
 		_have_configfs
 		_have_program rdma
-		_have_modules rdma_rxe || _have_modules siw
+		_have_driver rdma_rxe || _have_driver siw
 		;;
 	*)
 		SKIP_REASON="unsupported nvme_trtype=${nvme_trtype}"
@@ -125,11 +127,11 @@ _cleanup_nvmet() {
 	shopt -u nullglob
 	trap SIGINT
 
-	modprobe -r nvme-"${nvme_trtype}" 2>/dev/null
+	modprobe -rq nvme-"${nvme_trtype}" 2>/dev/null
 	if [[ "${nvme_trtype}" != "loop" ]]; then
-		modprobe -r nvmet-"${nvme_trtype}" 2>/dev/null
+		modprobe -rq nvmet-"${nvme_trtype}" 2>/dev/null
 	fi
-	modprobe -r nvmet 2>/dev/null
+	modprobe -rq nvmet 2>/dev/null
 	if [[ "${nvme_trtype}" == "rdma" ]]; then
 		stop_soft_rdma
 	fi
@@ -137,11 +139,11 @@ _cleanup_nvmet() {
 
 _setup_nvmet() {
 	_register_test_cleanup _cleanup_nvmet
-	modprobe nvmet
+	modprobe -q nvmet
 	if [[ "${nvme_trtype}" != "loop" ]]; then
-		modprobe nvmet-"${nvme_trtype}"
+		modprobe -q nvmet-"${nvme_trtype}"
 	fi
-	modprobe nvme-"${nvme_trtype}"
+	modprobe -q nvme-"${nvme_trtype}"
 	if [[ "${nvme_trtype}" == "rdma" ]]; then
 		start_soft_rdma
 		for i in $(rdma_network_interfaces)
-- 
2.30.2

[PATCH blktests 9/9] common: do not require scsi_debug support to be modular

[Christoph Hellwig]

Use _have_driver instead of _have_modules in _have_scsi_debug for the
basic scsi_debug check, and instead only require an actual module in
_init_null_blk when specific module parameters are passed.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 common/scsi_debug | 14 ++++++++++----
 tests/block/001   |  4 +++-
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/common/scsi_debug b/common/scsi_debug
index 95da14e..e9161d3 100644
--- a/common/scsi_debug
+++ b/common/scsi_debug
@@ -5,7 +5,7 @@
 # scsi_debug helper functions.
 
 _have_scsi_debug() {
-	_have_modules scsi_debug
+	_have_driver scsi_debug
 }
 
 _init_scsi_debug() {
@@ -18,8 +18,14 @@ _init_scsi_debug() {
 		args+=(zbc=host-managed zone_nr_conv=0)
 	fi
 
-	if ! modprobe -r scsi_debug || ! modprobe scsi_debug "${args[@]}"; then
-		return 1
+	if ((${#args[@]})); then
+		if ! modprobe -qr scsi_debug; then
+			exit 1
+		fi
+		if ! modprobe scsi_debug "${args[@]}"; then
+			SKIP_REASON="scsi_debug not modular"
+			return 1
+		fi
 	fi
 
 	udevadm settle
@@ -60,5 +66,5 @@ _exit_scsi_debug() {
 	unset SCSI_DEBUG_TARGETS
 	unset SCSI_DEBUG_DEVICES
 	udevadm settle
-	modprobe -r scsi_debug
+	modprobe -rq scsi_debug
 }
diff --git a/tests/block/001 b/tests/block/001
index 5f05fa8..fb93932 100755
--- a/tests/block/001
+++ b/tests/block/001
@@ -13,7 +13,9 @@ DESCRIPTION="stress device hotplugging"
 TIMED=1
 
 requires() {
-	_have_scsi_debug && _have_modules sd_mod sr_mod
+	_have_scsi_debug
+	_have_driver sd_mod
+	_have_driver sr_mod
 }
 
 stress_scsi_debug() {
-- 
2.30.2

Re: reduce the dependency on modules

[Jens Axboe]

On 5/30/22 7:08 AM, Christoph Hellwig wrote:
> Hi Shin'ichiro,
> 
> this series reduces the dependency of blktests on modular builds
> of various block drivers.  There are still plenty of tests that
> do require modules, mostly because a lot of scsi_debug and null_blk
> can only be set at load time, but I plan to address those in the
> kernel soon.

Which null_blk options can only be set at load time? If there are some,
we should get them fixed up / added to the configfs side. Yes, the
configfs side also kind of sucks, but at least it's there.

-- 
Jens Axboe

Re: reduce the dependency on modules

[Christoph Hellwig]

On Mon, May 30, 2022 at 07:12:22AM -0600, Jens Axboe wrote:
> Which null_blk options can only be set at load time?

The example I ran into is the queue mode.

> If there are some,
> we should get them fixed up / added to the configfs side. Yes, the
> configfs side also kind of sucks, but at least it's there.

That is my plan.  I just wanted to get this series out first and then
work on the non-tivial bits involving the kernel.

Re: [PATCH blktests 1/9] nvme: use _have_loop instead of _have_modules loop

[Shinichiro Kawasaki]

On May 30, 2022 / 15:08, Christoph Hellwig wrote:
> Also check for the losetup existance.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good to me.

Reviewed-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>

-- 
Shin'ichiro Kawasaki

Re: [PATCH blktests 2/9] common: add a helper if a driver is available

[Shinichiro Kawasaki]

On May 30, 2022 / 15:08, Christoph Hellwig wrote:
> Unlike _have_modules this allows allows for a built-in driver.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  common/rc | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/common/rc b/common/rc
> index 5e35e21..a93b227 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -28,6 +28,18 @@ _have_root() {
>  	return 0
>  }
>  
> +_have_driver()
> +{
> +	local modname="${1/-/_}"
> +
> +	if [ ! -d "/sys/module/${modname}" ] && ! modprobe -q ${modname}; then

Nit: double quote is required for ${modname} to make shellcheck happy.

Other than that, looks good to me.

Reviewed-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>

-- 
Shin'ichiro Kawasaki

Re: [PATCH blktests 3/9] common: fix _have_module_param_value to work with built-in drivers

[Shinichiro Kawasaki]

On May 30, 2022 / 15:08, Christoph Hellwig wrote:
> Don't bother to call modprobe directly and just check the /sys/module/
> directory.  Also switch to using descriptive variable names for the
> paramters.

s/paramters/parameters/

> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  common/rc | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index a93b227..ffd15b6 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -74,17 +74,22 @@ _have_module_param() {
>  }
>  
>  _have_module_param_value() {
> +	local modname="${1/-/_}"
> +	local param="$2"
> +	local expected_value="$3"
>  	local value
>  
> -	modprobe "$1"
> +	if ! have_driver $modname; then

Underscore is missing for '_have_driver'.
Also, shellcheck requires double quote of $modname.

> +		return 1;
> +	fi
>  
> -	if ! _have_module_param "$1" "$2"; then
> +	if ! _have_module_param $modname $param; then

Same here, double quotes are required for $modname and $param.

>  		return 1
>  	fi
>  
> -	value=$(cat "/sys/module/$1/parameters/$2")
> -	if [[ "${value}" != "$3" ]]; then
> -		SKIP_REASON="$1 module parameter $2 must be set to $3"
> +	value=$(cat "/sys/module/$modname/parameters/$param")
> +	if [[ "${value}" != "$expected_value" ]]; then
> +		SKIP_REASON="$modname module parameter $param must be set to $expected_value"
>  		return 1
>  	fi
>  
> -- 
> 2.30.2
> 

-- 
Shin'ichiro Kawasaki

Re: [PATCH blktests 4/9] common: do not require loop support to be modular

[Shinichiro Kawasaki]

On May 30, 2022 / 15:08, Christoph Hellwig wrote:
> Use _have_driver instead of _have_modules in _have_loop as nothing requires
> the loop driver to be modular.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good to me.

Reviewed-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>

-- 
Shin'ichiro Kawasaki

Re: [PATCH blktests 5/9] common: do not require null_blk support to be modular

[Shinichiro Kawasaki]

On May 30, 2022 / 15:08, Christoph Hellwig wrote:
> Use _have_driver instead of _have_modules in _have_null_blk for the basic
> null_blk check, and instead only require an actual module in
> _init_null_blk when specific module parameters are passed.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  common/null_blk | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/common/null_blk b/common/null_blk
> index 6611db0..ccf3750 100644
> --- a/common/null_blk
> +++ b/common/null_blk
> @@ -5,7 +5,7 @@
>  # null_blk helper functions.
>  
>  _have_null_blk() {
> -	_have_modules null_blk
> +	_have_driver null_blk
>  }
>  
>  _remove_null_blk_devices() {
> @@ -16,15 +16,19 @@ _remove_null_blk_devices() {
>  }
>  
>  _init_null_blk() {
> -	_remove_null_blk_devices
> +	local modparams="$@"

Shellcheck complains on this line:
common/null_blk:19:18: warning: Assigning an array to a string! Assign as array, or use * instead of @ to concatenate. [SC2124]

I think array would be the better.

>  
> -	local zoned=""
> -	if (( RUN_FOR_ZONED )); then zoned="zoned=1"; fi
> +	if (( RUN_FOR_ZONED )); then
> +		modparams="${modparams} zoned=1"
> +	fi
>  
> -	if ! modprobe -r null_blk || ! modprobe null_blk "$@" "${zoned}" ; then
> +	if [ -n "${modparams}" ] && ! _have_modules null_blk; then

I tried this part on my test machine and it looks that '_have_modules null_blk'
returns 0=success even when null_blk module is built-in. This may not be what
you intended here. I think _have_modules needs modification so that it returns
1=failure when one of the modules is built-in. (At least until all null_blk
parameters get supported by configfs.)

>  		return 1
>  	fi
>  
> +	_remove_null_blk_devices
> +	modprobe -qr null_blk && modprobe -q null_blk ${modparams}

Double quotes for ${modparams} is required for shellcheck.

-- 
Shin'ichiro Kawasaki

Re: [PATCH blktests 9/9] common: do not require scsi_debug support to be modular

[Shinichiro Kawasaki]

On May 30, 2022 / 15:08, Christoph Hellwig wrote:
> Use _have_driver instead of _have_modules in _have_scsi_debug for the
> basic scsi_debug check, and instead only require an actual module in
> _init_null_blk when specific module parameters are passed.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  common/scsi_debug | 14 ++++++++++----
>  tests/block/001   |  4 +++-
>  2 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/common/scsi_debug b/common/scsi_debug
> index 95da14e..e9161d3 100644
> --- a/common/scsi_debug
> +++ b/common/scsi_debug
> @@ -5,7 +5,7 @@
>  # scsi_debug helper functions.
>  
>  _have_scsi_debug() {
> -	_have_modules scsi_debug
> +	_have_driver scsi_debug
>  }
>  
>  _init_scsi_debug() {
> @@ -18,8 +18,14 @@ _init_scsi_debug() {
>  		args+=(zbc=host-managed zone_nr_conv=0)
>  	fi
>  
> -	if ! modprobe -r scsi_debug || ! modprobe scsi_debug "${args[@]}"; then
> -		return 1
> +	if ((${#args[@]})); then
> +		if ! modprobe -qr scsi_debug; then
> +			exit 1
> +		fi
> +		if ! modprobe scsi_debug "${args[@]}"; then
> +			SKIP_REASON="scsi_debug not modular"

I tried scsi_debug built-in kernel and observed that 'modprobe -qr scsi_debug'
command fails, and the script exits at the line of "exit 1". The SKIP_REASON
comment above will not be printed. I think we need to check the scsi_debug is
not built-in. As I commented for _init_null_blk, _have_modules needs
modification to check if the module is not built-in. I guess it can be used for
scsi_debug also.

-- 
Shin'ichiro Kawasaki

Re: [PATCH blktests 9/9] common: do not require scsi_debug support to be modular

[Shinichiro Kawasaki]

On May 31, 2022 / 12:21, Shinichiro Kawasaki wrote:
> On May 30, 2022 / 15:08, Christoph Hellwig wrote:
> > Use _have_driver instead of _have_modules in _have_scsi_debug for the
> > basic scsi_debug check, and instead only require an actual module in
> > _init_null_blk when specific module parameters are passed.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> >  common/scsi_debug | 14 ++++++++++----
> >  tests/block/001   |  4 +++-
> >  2 files changed, 13 insertions(+), 5 deletions(-)
> > 
> > diff --git a/common/scsi_debug b/common/scsi_debug
> > index 95da14e..e9161d3 100644
> > --- a/common/scsi_debug
> > +++ b/common/scsi_debug
> > @@ -5,7 +5,7 @@
> >  # scsi_debug helper functions.
> >  
> >  _have_scsi_debug() {
> > -	_have_modules scsi_debug
> > +	_have_driver scsi_debug
> >  }
> >  
> >  _init_scsi_debug() {
> > @@ -18,8 +18,14 @@ _init_scsi_debug() {
> >  		args+=(zbc=host-managed zone_nr_conv=0)
> >  	fi
> >  
> > -	if ! modprobe -r scsi_debug || ! modprobe scsi_debug "${args[@]}"; then
> > -		return 1
> > +	if ((${#args[@]})); then
> > +		if ! modprobe -qr scsi_debug; then
> > +			exit 1
> > +		fi
> > +		if ! modprobe scsi_debug "${args[@]}"; then
> > +			SKIP_REASON="scsi_debug not modular"
> 
> I tried scsi_debug built-in kernel and observed that 'modprobe -qr scsi_debug'
> command fails, and the script exits at the line of "exit 1". The SKIP_REASON
> comment above will not be printed. I think we need to check the scsi_debug is
> not built-in. As I commented for _init_null_blk, _have_modules needs
> modification to check if the module is not built-in. I guess it can be used for
> scsi_debug also.

Another nit: the SKIP_REASON value update will make shellcheck complain:

common/scsi_debug:26:4: warning: SKIP_REASON appears unused. Verify use (or export if used externally). [SC2034]

As some of other common/* does, sourcing common/shellcheck at beginning of
common/scsi_debug will avoid it.

-- 
Shin'ichiro Kawasaki

Re: reduce the dependency on modules

[Shinichiro Kawasaki]

On May 30, 2022 / 15:08, Christoph Hellwig wrote:
> Hi Shin'ichiro,
> 
> this series reduces the dependency of blktests on modular builds
> of various block drivers.  There are still plenty of tests that
> do require modules, mostly because a lot of scsi_debug and null_blk
> can only be set at load time, but I plan to address those in the
> kernel soon.

Hi Christoph, thank you for the pathces. I think this is the nice first-step to
avoid the module load dependency.

I tried to run blktests with the patches. At first, I kept blktests dependent
drivers as modules, and observed no additional failure. Good. As the next step,
I ran blktests making blktests dependent drivers built into kernel, and observed
some unexpected behaviors. For example, block/016, which calls _init_null_blk,
was not skipped and failed. block/025, which calls _init_scsi_debug was
terminated unexpectedly. I made related comments on 5th and 9th patches.

My other comments are nits. Many of them are related to shellcheck warnings. It
will be appreciated to run 'make check' command to see the warnings and clean
them up.

-- 
Shin'ichiro Kawasaki

Re: [PATCH blktests 2/9] common: add a helper if a driver is available

[Christoph Hellwig]

On Tue, May 31, 2022 at 11:12:45AM +0000, Shinichiro Kawasaki wrote:
> > +_have_driver()
> > +{
> > +	local modname="${1/-/_}"
> > +
> > +	if [ ! -d "/sys/module/${modname}" ] && ! modprobe -q ${modname}; then
> 
> Nit: double quote is required for ${modname} to make shellcheck happy.

It is aready quote when assigning to modname, so there is no actual need.
But I'll add the quotes to make the checker happy.