[PATCH blktests v3 0/3] Add support to run against real target

[PATCH blktests v3 0/3] Add support to run against real target

[Daniel Wagner]

I've added a new hook so that the default variables can be configured via the
script. This simple overwrite of the defaults allows to use external configured
setups (there is some trickery involved as it's not possible to do it only once
due to include orders). The upside of this approach is that we don't have to add
more environment variables.

I've run blktests against a PowerStore. That worked fairly okay but there were
some fallouts which is kind of expected at this stage:

# cat ~/.config/blktests/nvme_target_control.toml
[main]
skip_setup_cleanup=true
nvmetcli='/home/wagi/work/nvmetcli/nvmetcli'
remote='http://nvmet:5000'

[host]
blkdev_type='device'
trtype='tcp'
hostnqn='nqn.2014-08.org.nvmexpress:uuid:1a9e23dd-466e-45ca-9f43-a29aaf47cb21'
hostid='1a9e23dd-466e-45ca-9f43-a29aaf47cb21'
host_traddr='10.161.16.48'

[subsys_0]
traddr='10.162.198.45'
trsvid='4420'
subsysnqn='nqn.1988-11.com.dell:powerstore:00:f03028e73ef7D032D81E'
subsys_uuid='3a5c104c-ee41-38a1-8ccf-0968003d54e7'


# NVME_TARGET_CONTROL=/root/blktests/contrib/nvme_target_control.py ./check nvme

nvme/002 (tr=tcp) (create many subsystems and test discovery) [not run]
    nvme_trtype=tcp is not supported in this test
nvme/003 (tr=tcp) (test if we're sending keep-alives to a discovery controller)
nvme/003 (tr=tcp) (test if we're sending keep-alives to a discovery controller) [passed]
    runtime    ...  15.397s
nvme/004 (tr=tcp) (test nvme and nvmet UUID NS descriptors)  [failed]
    runtime    ...  42.584s
    --- tests/nvme/004.out      2024-06-27 09:45:35.496518067 +0200
    +++ /root/blktests/results/nodev_tr_tcp/nvme/004.out.bad    2024-06-27 10:38:59.424409636 +0200
    @@ -1,3 +1,4 @@
     Running nvme/004
    -disconnected 1 controller(s)
    +No namespaces found
    +disconnected 13 controller(s)
     Test complete
nvme/005 (tr=tcp) (reset local loopback target)              [passed]
    runtime    ...  11.160s
nvme/006 (tr=tcp bd=device) (create an NVMeOF target)        [passed]
    runtime    ...  1.350s
nvme/008 (tr=tcp bd=device) (create an NVMeOF host)          [failed]
    runtime    ...  8.748s
    --- tests/nvme/008.out      2024-06-27 09:45:35.496518067 +0200
    +++ /root/blktests/results/nodev_tr_tcp_bd_device/nvme/008.out.bad  2024-06-27 10:39:23.624408817 +0200
    @@ -1,3 +1,4 @@
     Running nvme/008
    +UUID 3a5c104c-ee41-38a1-8ccf-0968003d54e7 mismatch (wwid eui.3a5c104cee4138a18ccf0968003d54e7)
     disconnected 1 controller(s)
     Test complete
nvme/010 (tr=tcp bd=device) (run data verification fio job)  [passed]
    runtime    ...  29.798s
nnvme/012 (tr=tcp bd=device) (run mkfs and data verification fio) [failed]
    runtime    ...  162.299s
    --- tests/nvme/012.out      2024-06-27 09:45:35.500518066 +0200
    +++ /root/blktests/results/nodev_tr_tcp_bd_device/nvme/012.out.bad  2024-06-27 10:42:38.008402238 +0200
    @@ -1,3 +1,6 @@
     Running nvme/012
    +fio: io_u error on file /mnt/blktests//verify.0.0: No space left on device: write offset=44917682176, buflen=4096
    +fio exited with status 1
    +4;fio-3.23;verify;0;28;0;0;0;0;0;0;0.000000;0.000000;0;0;0.000000;0.000000;1.000000%=0;5.000000%=0;10.000000%=0;20.000000%=0;30.000000%=0;40.000000%=0;50.000000%=0;60.000000%=0;70.000000%=0;80.000000%=0;90.000000%=0;95.000000%=0;99.000000%=0;99.500000%=0;99.900000%=0;99.950000%=0;99.990000%=0;0%=0;0%=0;0%=0;0;0;0.000000;0.000000;0;0;0.000000%;0.000000;0.000000;3508332;24662;6165;142251;5;19501;35.466984;172.426891;127;60177;2556.665370;1420.047447;1.000000%=1056;5.000000%=1548;10.000000%=1646;20.000000%=1777;30.000000%=1908;40.000000%=2072;50.000000%=2277;60.000000%=2441;70.000000%=2637;80.000000%=2932;90.000000%=3653;95.000000%=4554;99.000000%=8224;99.500000%=10027;99.900000%=14745;99.950000%=17956;99.990000%=39583;0%=0;0%=0;0%=0;418;60193;2592.427453;1433.177059;18632;44736;100.000000%;24704.690141;4469.952445;0;0;0;0;0;0;0.000000;0.000000;0;0;0.000000;0.000000;1.000000%=0;5.000000%=0;10.000000%=0;20.000000%=0;30.000000%=0;40.000000%=0;50.000000%=0;60.000000%=0;70.000000%=0;80.000000%=0;90.000000%=0;95.000000%=0;99.000000%=0;99.500000%=0;99.900000%=0;99.950000%=0;99.990000%=0;0%=0;0%=0;0%=0;0;0;0.000000;0.000000;0;0;0.000000%;0.000000;0.000000;5.627417%;9.681547%;711749;0;22200;0.1%;0.1%;0.1%;0.1%;100.0%;0.0%;0.0%;0.00%;0.00%;0.00%;0.00%;0.00%;0.00%;0.01%;0.12%;0.29%;0.43%;35.65%;56.04%;6.95%;0.47%;0.03%;0.01%;0.00%;0.00%;0.00%;0.00%;0.00%;0.00%;nvme0n1;0;1739486;0;0;0;4596624;4596624;100.00%
     disconnected 1 controller(s)
     Test complete
nvme/014 (tr=tcp bd=device) (flush a command from host)
^C^C^C

The flush test hanged forever but this could just be an outdated host kernel.

changes:
v3:
  - added support for previous configured target
  - renamed nvme_nvme_target to	_require_kernel_nvme_target
  - use shorter redirect operator
  - https://lore.kernel.org/all/20240612110444.4507-1-dwagner@suse.de/
v2:
  - many of the preperation patches have been merged, drop them
  - added a python script which implements the blktests API
  - add some documentation how to use it
  - changed the casing of the environment variables to upper case

v1:
  - initial version
  - https://lore.kernel.org/linux-nvme/20240318093856.22307-1-dwagner@suse.de/

Daniel Wagner (3):
  nvme/rc: introduce remote target support
  nvme/030: only run against kernel soft target
  contrib: add remote target setup/cleanup script

 Documentation/running-tests.md |  33 ++++++
 check                          |   4 +
 contrib/nvme_target_control.py | 181 +++++++++++++++++++++++++++++++++
 contrib/nvmet-subsys.jinja2    |  71 +++++++++++++
 tests/nvme/030                 |   1 +
 tests/nvme/rc                  |  65 +++++++++++-
 6 files changed, 353 insertions(+), 2 deletions(-)
 create mode 100755 contrib/nvme_target_control.py
 create mode 100644 contrib/nvmet-subsys.jinja2

-- 
2.45.2

[PATCH blktests v3 1/3] nvme/rc: introduce remote target support

[Daniel Wagner]

Most of the NVMEeoF tests are exercising the host code of the nvme
subsystem. There is no real reason not to run these against a real
target. We just have to skip the soft target setup and make it possible
to setup a remote target.

Because all tests use now the common setup/cleanup helpers we just need
to intercept this call and forward it to an external component.

As we already have various nvme variables to setup the target which we
should allow to overwrite. Also introduce a NVME_TARGET_CONTROL variable
which points to a script which gets executed whenever a targets needs to
be created/destroyed.

Signed-off-by: Daniel Wagner <dwagner@suse.de>
---
 Documentation/running-tests.md | 33 ++++++++++++++++++++
 check                          |  4 +++
 tests/nvme/rc                  | 57 ++++++++++++++++++++++++++++++++--
 3 files changed, 92 insertions(+), 2 deletions(-)

diff --git a/Documentation/running-tests.md b/Documentation/running-tests.md
index 968702e76bb5..fe4f729bd037 100644
--- a/Documentation/running-tests.md
+++ b/Documentation/running-tests.md
@@ -120,6 +120,9 @@ The NVMe tests can be additionally parameterized via environment variables.
 - NVME_NUM_ITER: 1000 (default)
   The number of iterations a test should do. This parameter had an old name
   'nvme_num_iter'. The old name is still usable, but not recommended.
+- NVME_TARGET_CONTROL: When defined, the generic target setup/cleanup code will
+  be skipped and this script gets called. This makes it possible to run
+  the fabric nvme tests against a real target.
 
 ### Running nvme-rdma and SRP tests
 
@@ -167,3 +170,33 @@ if ! findmnt -t configfs /sys/kernel/config > /dev/null; then
 	mount -t configfs configfs /sys/kernel/config
 fi
 ```
+### NVME_TARGET_CONTROL
+
+When NVME_TARGET_CONTROL is set, blktests will call the script which the
+environment variable points to, to fetch the configuration values to be used for
+the runs, e.g subsysnqn or hostnqn. This allows the blktest to be run against
+external configured/setup targets.
+
+The blktests expects that the script interface implements following
+commands:
+
+config:
+  --show-blkdev-type
+  --show-trtype
+  --show-hostnqn
+  --show-hostid
+  --show-host-traddr
+  --show-traddr
+  --show-trsvid
+  --show-subsys-uuid
+  --show-subsysnqn
+
+setup:
+  --subsysnqn SUBSYSNQN
+  --subsys-uuid SUBSYS_UUID
+  --hostnqn HOSTNQN
+  --ctrlkey CTRLKEY
+  --hostkey HOSTKEY
+
+cleanup:
+  --subsysnqn SUBSYSNQN
diff --git a/check b/check
index 3ed4510f3f40..d0475629773d 100755
--- a/check
+++ b/check
@@ -603,6 +603,10 @@ _run_group() {
 	# shellcheck disable=SC1090
 	. "tests/${group}/rc"
 
+	if declare -fF group_setup >/dev/null; then
+		group_setup
+	fi
+
 	if declare -fF group_requires >/dev/null; then
 		group_requires
 		if [[ -v SKIP_REASONS ]]; then
diff --git a/tests/nvme/rc b/tests/nvme/rc
index c1ddf412033b..4465dea0370b 100644
--- a/tests/nvme/rc
+++ b/tests/nvme/rc
@@ -23,6 +23,7 @@ _check_conflict_and_set_default NVME_IMG_SIZE nvme_img_size 1G
 _check_conflict_and_set_default NVME_NUM_ITER nvme_num_iter 1000
 nvmet_blkdev_type=${nvmet_blkdev_type:-"device"}
 NVMET_BLKDEV_TYPES=${NVMET_BLKDEV_TYPES:-"device file"}
+nvme_target_control="${NVME_TARGET_CONTROL:-}"
 
 _NVMET_TRTYPES_is_valid() {
 	local type
@@ -135,6 +136,13 @@ _nvme_requires() {
 	return 0
 }
 
+group_setup() {
+	if [[ -n "${nvme_target_control}" ]]; then
+		NVMET_TRTYPES="$(${nvme_target_control} config --show-trtype)"
+		NVMET_BLKDEV_TYPES="$(${nvme_target_control} config --show-blkdev-type)"
+	fi
+}
+
 group_requires() {
 	_have_root
 	_NVMET_TRTYPES_is_valid
@@ -359,6 +367,10 @@ _cleanup_nvmet() {
 		fi
 	done
 
+	if [[ -n "${nvme_target_control}" ]]; then
+		return
+	fi
+
 	for port in "${NVMET_CFS}"/ports/*; do
 		name=$(basename "${port}")
 		echo "WARNING: Test did not clean up port: ${name}"
@@ -403,11 +415,26 @@ _cleanup_nvmet() {
 
 _setup_nvmet() {
 	_register_test_cleanup _cleanup_nvmet
+
+	if [[ -n "${nvme_target_control}" ]]; then
+		def_hostnqn="$(${nvme_target_control} config --show-hostnqn)"
+		def_hostid="$(${nvme_target_control} config --show-hostid)"
+		def_host_traddr="$(${nvme_target_control} config --show-host-traddr)"
+		def_traddr="$(${nvme_target_control} config --show-traddr)"
+		def_trsvcid="$(${nvme_target_control} config --show-trsvid)"
+		def_subsys_uuid="$(${nvme_target_control} config --show-subsys-uuid)"
+		def_subsysnqn="$(${nvme_target_control} config --show-subsysnqn)"
+		return
+	fi
+
 	modprobe -q nvmet
+
 	if [[ "${nvme_trtype}" != "loop" ]]; then
 		modprobe -q nvmet-"${nvme_trtype}"
 	fi
+
 	modprobe -q nvme-"${nvme_trtype}"
+
 	if [[ "${nvme_trtype}" == "rdma" ]]; then
 		start_soft_rdma
 		for i in $(rdma_network_interfaces)
@@ -425,6 +452,7 @@ _setup_nvmet() {
 			fi
 		done
 	fi
+
 	if [[ "${nvme_trtype}" = "fc" ]]; then
 		modprobe -q nvme-fcloop
 		_setup_fcloop "${def_local_wwnn}" "${def_local_wwpn}" \
@@ -873,11 +901,13 @@ _find_nvme_passthru_loop_dev() {
 
 _nvmet_target_setup() {
 	local blkdev_type="${nvmet_blkdev_type}"
+	local subsys_uuid="${def_subsys_uuid}"
+	local subsysnqn="${def_subsysnqn}"
 	local blkdev
+	local ARGS=()
 	local ctrlkey=""
 	local hostkey=""
-	local subsysnqn="${def_subsysnqn}"
-	local subsys_uuid="${def_subsys_uuid}"
+	local blkdev
 	local port
 
 	while [[ $# -gt 0 ]]; do
@@ -909,6 +939,22 @@ _nvmet_target_setup() {
 		esac
 	done
 
+	if [[ -n "${hostkey}" ]]; then
+		ARGS+=(--hostkey "${hostkey}")
+	fi
+	if [[ -n "${ctrlkey}" ]]; then
+		ARGS+=(--ctrkey "${ctrlkey}")
+	fi
+
+	if [[ -n "${nvme_target_control}" ]]; then
+		eval "${nvme_target_control}" setup \
+			--subsysnqn "${subsysnqn}" \
+			--subsys-uuid "${subsys_uuid}" \
+			--hostnqn "${def_hostnqn}" \
+			"${ARGS[@]}" &> /dev/null
+		return
+	fi
+
 	truncate -s "${NVME_IMG_SIZE}" "$(_nvme_def_file_path)"
 	if [[ "${blkdev_type}" == "device" ]]; then
 		blkdev="$(losetup -f --show "$(_nvme_def_file_path)")"
@@ -948,6 +994,13 @@ _nvmet_target_cleanup() {
 		esac
 	done
 
+	if [[ -n "${nvme_target_control}" ]]; then
+		eval "${nvme_target_control}" cleanup \
+			--subsysnqn "${subsysnqn}" \
+			> /dev/null
+		return
+	fi
+
 	_get_nvmet_ports "${subsysnqn}" ports
 
 	for port in "${ports[@]}"; do
-- 
2.45.2

[PATCH blktests v3 2/3] nvme/030: only run against kernel soft target

[Daniel Wagner]

This tests is exercising the target code and not so much the host side.
The problem with nvme/030 is that it depends on interface to interact
with the target which is not covered by the standard. Thus we can't
run it against a real target. Just skip it when we run against a
real target.

Signed-off-by: Daniel Wagner <dwagner@suse.de>
---
 tests/nvme/030 | 1 +
 tests/nvme/rc  | 8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/tests/nvme/030 b/tests/nvme/030
index b1ed8bc20908..ae7081d55354 100755
--- a/tests/nvme/030
+++ b/tests/nvme/030
@@ -13,6 +13,7 @@ requires() {
 	_nvme_requires
 	_have_loop
 	_require_nvme_trtype_is_fabrics
+	_require_kernel_nvme_target
 }
 
 set_conditions() {
diff --git a/tests/nvme/rc b/tests/nvme/rc
index 4465dea0370b..5e2e440ccd47 100644
--- a/tests/nvme/rc
+++ b/tests/nvme/rc
@@ -225,6 +225,14 @@ _require_kernel_nvme_fabrics_feature() {
 	return 0
 }
 
+_require_kernel_nvme_target() {
+	if [[ -n "${nvme_target_control}" ]]; then
+		SKIP_REASONS+=("Linux kernel soft target not available")
+		return 1;
+	fi
+	return 0
+}
+
 _test_dev_nvme_ctrl() {
 	echo "/dev/char/$(cat "${TEST_DEV_SYSFS}/device/dev")"
 }
-- 
2.45.2

[PATCH blktests v3 3/3] contrib: add remote target setup/cleanup script

[Daniel Wagner]

Use nvmetcli to setup/cleanup a remote soft target.

Signed-off-by: Daniel Wagner <dwagner@suse.de>
---
 contrib/nvme_target_control.py | 181 +++++++++++++++++++++++++++++++++
 contrib/nvmet-subsys.jinja2    |  71 +++++++++++++
 2 files changed, 252 insertions(+)
 create mode 100755 contrib/nvme_target_control.py
 create mode 100644 contrib/nvmet-subsys.jinja2

diff --git a/contrib/nvme_target_control.py b/contrib/nvme_target_control.py
new file mode 100755
index 000000000000..0a34a5a85a66
--- /dev/null
+++ b/contrib/nvme_target_control.py
@@ -0,0 +1,181 @@
+#!/usr/bin/env python3
+# SPDX-License-Identifier: GPL-3.0+
+
+# blktests calls this script to setup/teardown remote targets. blktests passes
+# all relevant information via the command line, e.g. --hostnqn.
+#
+# This script uses nvmetcli to setup the remote target (it depends on the REST
+# API feature [1]). There is not technical need for nvmetcli to use but it makes
+# it simple to setup a remote Linux box. If you want to setup someting else
+# you should to replace this part.
+#
+# There are couple of global configuration options which need to be set.
+# Add ~/.config/blktests/nvme_target_control.toml file with something like:
+#
+# [main]
+# skip_setup_cleanup=false
+# nvmetcli='/usr/bin/nvmetcli'
+# remote='http://nvmet.local:5000'
+#
+# [host]
+# blkdev_type='device'
+# trtype='tcp'
+# hostnqn='nqn.2014-08.org.nvmexpress:uuid:0f01fb42-9f7f-4856-b0b3-51e60b8de349'
+# hostid='0f01fb42-9f7f-4856-b0b3-51e60b8de349'
+# host_traddr='192.168.154.187'
+#
+# [subsys_0]
+# traddr='192.168.19.189'
+# trsvid='4420'
+# subsysnqn='blktests-subsystem-1'
+# subsys_uuid='91fdba0d-f87b-4c25-b80f-db7be1418b9e'
+#
+# This expects nvmetcli with the restapi service running on target.
+#
+# Alternatively, you can skip the the target setup/cleanup completely
+# (skip_setup_cleanup) and run against a previously configured target.
+#
+# [main]
+# skip_setup_cleanup=true
+# nvmetcli='/usr/bin/nvmetcli'
+# remote='http://nvmet.local:5000'
+#
+# [host]
+# blkdev_type='device'
+# trtype='tcp'
+# hostnqn='nqn.2014-08.org.nvmexpress:uuid:1a9e23dd-466e-45ca-9f43-a29aaf47cb21'
+# hostid='1a9e23dd-466e-45ca-9f43-a29aaf47cb21'
+# host_traddr='10.161.16.48'
+#
+# [subsys_0]
+# traddr='10.162.198.45'
+# trsvid='4420'
+# subsysnqn='nqn.1988-11.com.dell:powerstore:00:f03028e73ef7D032D81E'
+# subsys_uuid='3a5c104c-ee41-38a1-8ccf-0968003d54e7'
+#
+# nvmetcli uses JSON configuration, thus this script creates a JSON configuration
+# using a jinja2 template. After this step we simple have to set the blktests
+# variable correctly and start blktests.
+#
+#   NVME_TARGET_CONTROL=~/blktests/contrib/nvme_target_control.py ./check nvme
+#
+# [1] https://github.com/hreinecke/nvmetcli/tree/restapi
+
+import os
+import tomllib
+import argparse
+import subprocess
+from jinja2 import Environment, FileSystemLoader
+
+
+XDG_CONFIG_HOME = os.environ.get("XDG_CONFIG_HOME")
+if not XDG_CONFIG_HOME:
+    XDG_CONFIG_HOME = os.environ.get('HOME') + '/.config'
+
+
+with open(f'{XDG_CONFIG_HOME}/blktests/nvme_target_control.toml', 'rb') as f:
+    config = tomllib.load(f)
+    nvmetcli = config['main']['nvmetcli']
+    remote = config['main']['remote']
+
+
+def gen_conf(conf):
+    basepath = os.path.dirname(__file__)
+    environment = Environment(loader=FileSystemLoader(basepath))
+    template = environment.get_template('nvmet-subsys.jinja2')
+    filename = f'{conf["subsysnqn"]}.json'
+    content = template.render(conf)
+    with open(filename, mode='w', encoding='utf-8') as outfile:
+        outfile.write(content)
+
+
+def target_setup(args):
+    if config['main']['skip_setup_cleanup']:
+        return
+
+    conf = {
+        'subsysnqn': args.subsysnqn,
+        'subsys_uuid': args.subsys_uuid,
+        'hostnqn': args.hostnqn,
+        'allowed_hosts': args.hostnqn,
+        'ctrlkey': args.ctrlkey,
+        'hostkey': args.hostkey,
+        'blkdev': '/dev/vdc'
+    }
+
+    gen_conf(conf)
+
+    subprocess.call(['python3', nvmetcli, '--remote=' + remote,
+                     'restore', args.subsysnqn + '.json'])
+
+
+def target_cleanup(args):
+    if config['main']['skip_setup_cleanup']:
+        return
+
+    subprocess.call(['python3', nvmetcli, '--remote=' + remote,
+                     'clear', args.subsysnqn + '.json'])
+
+
+def target_config(args):
+	if args.show_blkdev_type:
+		print(config['host']['blkdev_type'])
+	elif args.show_trtype:
+		print(config['host']['trtype'])
+	elif args.show_hostnqn:
+		print(config['host']['hostnqn'])
+	elif args.show_hostid:
+		print(config['host']['hostid'])
+	elif args.show_host_traddr:
+		print(config['host']['host_traddr'])
+	elif args.show_traddr:
+		print(config['subsys_0']['traddr'])
+	elif args.show_trsvid:
+		print(config['subsys_0']['trsvid'])
+	elif args.show_subsysnqn:
+		print(config['subsys_0']['subsysnqn'])
+	elif args.show_subsys_uuid:
+		print(config['subsys_0']['subsys_uuid'])
+
+
+def build_parser():
+    parser = argparse.ArgumentParser()
+    sub = parser.add_subparsers(required=True)
+
+    setup = sub.add_parser('setup')
+    setup.add_argument('--subsysnqn', required=True)
+    setup.add_argument('--subsys-uuid', required=True)
+    setup.add_argument('--hostnqn', required=True)
+    setup.add_argument('--ctrlkey', default='')
+    setup.add_argument('--hostkey', default='')
+    setup.set_defaults(func=target_setup)
+
+    cleanup = sub.add_parser('cleanup')
+    cleanup.add_argument('--subsysnqn', required=True)
+    cleanup.set_defaults(func=target_cleanup)
+
+    config = sub.add_parser('config')
+    config.add_argument('--show-blkdev-type', action='store_true')
+    config.add_argument('--show-trtype', action='store_true')
+    config.add_argument('--show-hostnqn', action='store_true')
+    config.add_argument('--show-hostid', action='store_true')
+    config.add_argument('--show-host-traddr', action='store_true')
+    config.add_argument('--show-traddr', action='store_true')
+    config.add_argument('--show-trsvid', action='store_true')
+    config.add_argument('--show-subsys-uuid', action='store_true')
+    config.add_argument('--show-subsysnqn', action='store_true')
+    config.set_defaults(func=target_config)
+
+    return parser
+
+
+def main():
+    import sys
+
+    parser = build_parser()
+    args = parser.parse_args()
+    args.func(args)
+
+
+if __name__ == '__main__':
+    main()
diff --git a/contrib/nvmet-subsys.jinja2 b/contrib/nvmet-subsys.jinja2
new file mode 100644
index 000000000000..a446fbd9b784
--- /dev/null
+++ b/contrib/nvmet-subsys.jinja2
@@ -0,0 +1,71 @@
+{
+  "hosts": [
+    {
+      "nqn": "{{ hostnqn }}"
+    }
+  ],
+  "ports": [
+    {
+      "addr": {
+        "adrfam": "ipv4",
+        "traddr": "0.0.0.0",
+        "treq": "not specified",
+        "trsvcid": "4420",
+        "trtype": "tcp",
+        "tsas": "none"
+      },
+      "ana_groups": [
+        {
+          "ana": {
+            "state": "optimized"
+          },
+          "grpid": 1
+        }
+      ],
+      "param": {
+        "inline_data_size": "16384",
+        "pi_enable": "0"
+      },
+      "portid": 0,
+      "referrals": [],
+      "subsystems": [
+        "{{ subsysnqn }}"
+      ]
+    }
+  ],
+  "subsystems": [
+    {
+      "allowed_hosts": [
+        "{{ allowed_hosts }}"
+      ],
+      "attr": {
+        "allow_any_host": "0",
+        "cntlid_max": "65519",
+        "cntlid_min": "1",
+        "firmware": "yada",
+        "ieee_oui": "0x000000",
+        "model": "Linux",
+        "pi_enable": "0",
+        "qid_max": "128",
+        "serial": "0c74361069d9db6c65ef",
+        "version": "1.3"
+      },
+      "namespaces": [
+        {
+          "ana": {
+            "grpid": "1"
+          },
+          "ana_grpid": 1,
+          "device": {
+            "nguid": "00000000-0000-0000-0000-000000000000",
+            "path": "{{ blkdev }}",
+            "uuid": "{{ subsys_uuid }}"
+          },
+          "enable": 1,
+          "nsid": 1
+        }
+      ],
+      "nqn": "{{ subsysnqn }}"
+    }
+  ]
+}
-- 
2.45.2

Re: [PATCH blktests v3 0/3] Add support to run against real target

[Hannes Reinecke]

On 6/27/24 11:10, Daniel Wagner wrote:
> I've added a new hook so that the default variables can be configured via the
> script. This simple overwrite of the defaults allows to use external configured
> setups (there is some trickery involved as it's not possible to do it only once
> due to include orders). The upside of this approach is that we don't have to add
> more environment variables.
> 
Round of applause ... Thanks for doing this!

> I've run blktests against a PowerStore. That worked fairly okay but there were
> some fallouts which is kind of expected at this stage:
> 
I guess we should blank out tests which don't make sense for a real target.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH blktests v3 1/3] nvme/rc: introduce remote target support

[Hannes Reinecke]

On 6/27/24 11:10, Daniel Wagner wrote:
> Most of the NVMEeoF tests are exercising the host code of the nvme
> subsystem. There is no real reason not to run these against a real
> target. We just have to skip the soft target setup and make it possible
> to setup a remote target.
> 
> Because all tests use now the common setup/cleanup helpers we just need
> to intercept this call and forward it to an external component.
> 
> As we already have various nvme variables to setup the target which we
> should allow to overwrite. Also introduce a NVME_TARGET_CONTROL variable
> which points to a script which gets executed whenever a targets needs to
> be created/destroyed.
> 
> Signed-off-by: Daniel Wagner <dwagner@suse.de>
> ---
>   Documentation/running-tests.md | 33 ++++++++++++++++++++
>   check                          |  4 +++
>   tests/nvme/rc                  | 57 ++++++++++++++++++++++++++++++++--
>   3 files changed, 92 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/running-tests.md b/Documentation/running-tests.md
> index 968702e76bb5..fe4f729bd037 100644
> --- a/Documentation/running-tests.md
> +++ b/Documentation/running-tests.md
> @@ -120,6 +120,9 @@ The NVMe tests can be additionally parameterized via environment variables.
>   - NVME_NUM_ITER: 1000 (default)
>     The number of iterations a test should do. This parameter had an old name
>     'nvme_num_iter'. The old name is still usable, but not recommended.
> +- NVME_TARGET_CONTROL: When defined, the generic target setup/cleanup code will
> +  be skipped and this script gets called. This makes it possible to run
> +  the fabric nvme tests against a real target.
>   
>   ### Running nvme-rdma and SRP tests
>   
> @@ -167,3 +170,33 @@ if ! findmnt -t configfs /sys/kernel/config > /dev/null; then
>   	mount -t configfs configfs /sys/kernel/config
>   fi
>   ```
> +### NVME_TARGET_CONTROL
> +
> +When NVME_TARGET_CONTROL is set, blktests will call the script which the
> +environment variable points to, to fetch the configuration values to be used for
> +the runs, e.g subsysnqn or hostnqn. This allows the blktest to be run against
> +external configured/setup targets.
> +
> +The blktests expects that the script interface implements following
> +commands:
> +
> +config:
> +  --show-blkdev-type
> +  --show-trtype
> +  --show-hostnqn
> +  --show-hostid
> +  --show-host-traddr
> +  --show-traddr
> +  --show-trsvid
> +  --show-subsys-uuid
> +  --show-subsysnqn
> +
> +setup:
> +  --subsysnqn SUBSYSNQN
> +  --subsys-uuid SUBSYS_UUID
> +  --hostnqn HOSTNQN
> +  --ctrlkey CTRLKEY
> +  --hostkey HOSTKEY
> +
> +cleanup:
> +  --subsysnqn SUBSYSNQN
> diff --git a/check b/check
> index 3ed4510f3f40..d0475629773d 100755
> --- a/check
> +++ b/check
> @@ -603,6 +603,10 @@ _run_group() {
>   	# shellcheck disable=SC1090
>   	. "tests/${group}/rc"
>   
> +	if declare -fF group_setup >/dev/null; then
> +		group_setup
> +	fi
> +
>   	if declare -fF group_requires >/dev/null; then
>   		group_requires
>   		if [[ -v SKIP_REASONS ]]; then
> diff --git a/tests/nvme/rc b/tests/nvme/rc
> index c1ddf412033b..4465dea0370b 100644
> --- a/tests/nvme/rc
> +++ b/tests/nvme/rc
> @@ -23,6 +23,7 @@ _check_conflict_and_set_default NVME_IMG_SIZE nvme_img_size 1G
>   _check_conflict_and_set_default NVME_NUM_ITER nvme_num_iter 1000
>   nvmet_blkdev_type=${nvmet_blkdev_type:-"device"}
>   NVMET_BLKDEV_TYPES=${NVMET_BLKDEV_TYPES:-"device file"}
> +nvme_target_control="${NVME_TARGET_CONTROL:-}"
>   
>   _NVMET_TRTYPES_is_valid() {
>   	local type
> @@ -135,6 +136,13 @@ _nvme_requires() {
>   	return 0
>   }
>   
> +group_setup() {
> +	if [[ -n "${nvme_target_control}" ]]; then
> +		NVMET_TRTYPES="$(${nvme_target_control} config --show-trtype)"
> +		NVMET_BLKDEV_TYPES="$(${nvme_target_control} config --show-blkdev-type)"
> +	fi
> +}
> +
>   group_requires() {
>   	_have_root
>   	_NVMET_TRTYPES_is_valid
> @@ -359,6 +367,10 @@ _cleanup_nvmet() {
>   		fi
>   	done
>   
> +	if [[ -n "${nvme_target_control}" ]]; then
> +		return
> +	fi
> +
>   	for port in "${NVMET_CFS}"/ports/*; do
>   		name=$(basename "${port}")
>   		echo "WARNING: Test did not clean up port: ${name}"
> @@ -403,11 +415,26 @@ _cleanup_nvmet() {
>   
>   _setup_nvmet() {
>   	_register_test_cleanup _cleanup_nvmet
> +
> +	if [[ -n "${nvme_target_control}" ]]; then
> +		def_hostnqn="$(${nvme_target_control} config --show-hostnqn)"
> +		def_hostid="$(${nvme_target_control} config --show-hostid)"
> +		def_host_traddr="$(${nvme_target_control} config --show-host-traddr)"
> +		def_traddr="$(${nvme_target_control} config --show-traddr)"
> +		def_trsvcid="$(${nvme_target_control} config --show-trsvid)"
> +		def_subsys_uuid="$(${nvme_target_control} config --show-subsys-uuid)"
> +		def_subsysnqn="$(${nvme_target_control} config --show-subsysnqn)"
> +		return
> +	fi
> +
>   	modprobe -q nvmet
> +
>   	if [[ "${nvme_trtype}" != "loop" ]]; then
>   		modprobe -q nvmet-"${nvme_trtype}"
>   	fi
> +
>   	modprobe -q nvme-"${nvme_trtype}"
> +
>   	if [[ "${nvme_trtype}" == "rdma" ]]; then
>   		start_soft_rdma
>   		for i in $(rdma_network_interfaces)
> @@ -425,6 +452,7 @@ _setup_nvmet() {
>   			fi
>   		done
>   	fi
> +
>   	if [[ "${nvme_trtype}" = "fc" ]]; then
>   		modprobe -q nvme-fcloop
>   		_setup_fcloop "${def_local_wwnn}" "${def_local_wwpn}" \
> @@ -873,11 +901,13 @@ _find_nvme_passthru_loop_dev() {
>   
>   _nvmet_target_setup() {
>   	local blkdev_type="${nvmet_blkdev_type}"
> +	local subsys_uuid="${def_subsys_uuid}"
> +	local subsysnqn="${def_subsysnqn}"
>   	local blkdev
> +	local ARGS=()
>   	local ctrlkey=""
>   	local hostkey=""
> -	local subsysnqn="${def_subsysnqn}"
> -	local subsys_uuid="${def_subsys_uuid}"
> +	local blkdev
>   	local port
>   
>   	while [[ $# -gt 0 ]]; do
> @@ -909,6 +939,22 @@ _nvmet_target_setup() {
>   		esac
>   	done
>   
> +	if [[ -n "${hostkey}" ]]; then
> +		ARGS+=(--hostkey "${hostkey}")
> +	fi
> +	if [[ -n "${ctrlkey}" ]]; then
> +		ARGS+=(--ctrkey "${ctrlkey}")
> +	fi
> +
> +	if [[ -n "${nvme_target_control}" ]]; then
> +		eval "${nvme_target_control}" setup \
> +			--subsysnqn "${subsysnqn}" \
> +			--subsys-uuid "${subsys_uuid}" \
> +			--hostnqn "${def_hostnqn}" \
> +			"${ARGS[@]}" &> /dev/null
> +		return
> +	fi
> +
>   	truncate -s "${NVME_IMG_SIZE}" "$(_nvme_def_file_path)"
>   	if [[ "${blkdev_type}" == "device" ]]; then
>   		blkdev="$(losetup -f --show "$(_nvme_def_file_path)")"
> @@ -948,6 +994,13 @@ _nvmet_target_cleanup() {
>   		esac
>   	done
>   
> +	if [[ -n "${nvme_target_control}" ]]; then
> +		eval "${nvme_target_control}" cleanup \
> +			--subsysnqn "${subsysnqn}" \
> +			> /dev/null
> +		return
> +	fi
> +
>   	_get_nvmet_ports "${subsysnqn}" ports
>   
>   	for port in "${ports[@]}"; do

Hmm. This wasn't quite what I had in mind; I think it'd be simpler
if we could just check if the requested controller is visible to the
host already (ie checking sysfs here), and then skip all the setup
steps at they are obviously not required anymore.
That would save quite a lot of issues, and we wouldn't need to specify
a target setup script (or whatever).

Quite a bit of churn, I agree, as that would mean we have to roll

	_setup_nvmet

	_nvmet_target_setup

	_nvme_connect_subsys

all into one function. But it might be easier in the long run.
Hmm?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH blktests v3 1/3] nvme/rc: introduce remote target support

[Daniel Wagner]

On Thu, Jun 27, 2024 at 11:59:11AM GMT, Hannes Reinecke wrote:
 > +	if [[ -n "${nvme_target_control}" ]]; then
> > +		eval "${nvme_target_control}" cleanup \
> > +			--subsysnqn "${subsysnqn}" \
> > +			> /dev/null
> > +		return
> > +	fi
> > +
> >   	_get_nvmet_ports "${subsysnqn}" ports
> >   	for port in "${ports[@]}"; do
> 
> Hmm. This wasn't quite what I had in mind; I think it'd be simpler
> if we could just check if the requested controller is visible to the
> host already (ie checking sysfs here), and then skip all the setup
> steps at they are obviously not required anymore.
> That would save quite a lot of issues, and we wouldn't need to specify
> a target setup script (or whatever).

What I like at the current approach is that it allows to support the
use case where the external target can be configured. For example this
works with a remote VM running Linux pretty well. With your idea only
static setups are supported.

I don't know what you mean with 'quite a lot of issues'. I've running
this against a VM which gets dynamically configured and it works well.

> Quite a bit of churn, I agree, as that would mean we have to roll
>
> 	_setup_nvmet
>
> 	_nvmet_target_setup
>
> 	_nvme_connect_subsys
>
> all into one function. But it might be easier in the long run.
> Hmm?

Not all tests have this pattern, e.g. nvme/031. This test works with
the approach in the version of the series though.

Sure, I'll don't see a problem to refactor a bit more and reduce the
boiler plate even more but I see this a different task.

Re: [PATCH blktests v3 0/3] Add support to run against real target

[Christoph Hellwig]

On Thu, Jun 27, 2024 at 11:10:13AM +0200, Daniel Wagner wrote:
> I've added a new hook so that the default variables can be configured via the

Please make your mails readable by following the normal 73 character
limit.

Also what is more "real" about these targets?  They mostly are
remote and more diverse.  Maybe say arbitrary?

Re: [PATCH blktests v3 1/3] nvme/rc: introduce remote target support

[Shinichiro Kawasaki]

CC+: Ofir

A heads up: this patch causes conflict with Ofir's patch to move tests/nvme/rc
helpers to common/nvme [*]. Depending on which comes first, Daniel or Ofir will
need patch rework.

[*] https://lore.kernel.org/linux-block/20240624104620.2156041-2-ofir.gal@volumez.com/


On Jun 27, 2024 / 11:10, Daniel Wagner wrote:
> Most of the NVMEeoF tests are exercising the host code of the nvme
> subsystem. There is no real reason not to run these against a real
> target. We just have to skip the soft target setup and make it possible
> to setup a remote target.
> 
> Because all tests use now the common setup/cleanup helpers we just need
> to intercept this call and forward it to an external component.
> 
> As we already have various nvme variables to setup the target which we
> should allow to overwrite. Also introduce a NVME_TARGET_CONTROL variable
> which points to a script which gets executed whenever a targets needs to
> be created/destroyed.
> 
> Signed-off-by: Daniel Wagner <dwagner@suse.de>

Daniel, thanks for this v3 patch. Please find some comments in line.
I ran "make check" and observed some shellcheck warnings:

$ make check
shellcheck -x -e SC2119 -f gcc check common/* \
        tests/*/rc tests/*/[0-9]*[0-9] src/*.sh
tests/nvme/rc:962:5: warning: eval negates the benefit of arrays. Drop eval to preserve whitespace/symbols (or eval as string). [SC2294]
tests/nvme/041:34:36: note: Double quote to prevent globbing and word splitting. [SC2086]
tests/nvme/042:40:52: note: Double quote to prevent globbing and word splitting. [SC2086]
tests/nvme/042:54:61: note: Double quote to prevent globbing and word splitting. [SC2086]
tests/nvme/043:37:36: note: Double quote to prevent globbing and word splitting. [SC2086]
tests/nvme/044:36:36: note: Double quote to prevent globbing and word splitting. [SC2086]
tests/nvme/044:42:36: note: Double quote to prevent globbing and word splitting. [SC2086]
tests/nvme/045:41:36: note: Double quote to prevent globbing and word splitting. [SC2086]
tests/nvme/045:47:36: note: Double quote to prevent globbing and word splitting. [SC2086]
tests/nvme/045:72:43: note: Double quote to prevent globbing and word splitting. [SC2086]
tests/nvme/045:82:43: note: Double quote to prevent globbing and word splitting. [SC2086]
tests/nvme/051:40:25: note: Double quote to prevent globbing and word splitting. [SC2086]
tests/nvme/051:41:25: note: Double quote to prevent globbing and word splitting. [SC2086]

> ---
>  Documentation/running-tests.md | 33 ++++++++++++++++++++
>  check                          |  4 +++
>  tests/nvme/rc                  | 57 ++++++++++++++++++++++++++++++++--
>  3 files changed, 92 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/running-tests.md b/Documentation/running-tests.md
> index 968702e76bb5..fe4f729bd037 100644
> --- a/Documentation/running-tests.md
> +++ b/Documentation/running-tests.md
> @@ -120,6 +120,9 @@ The NVMe tests can be additionally parameterized via environment variables.
>  - NVME_NUM_ITER: 1000 (default)
>    The number of iterations a test should do. This parameter had an old name
>    'nvme_num_iter'. The old name is still usable, but not recommended.
> +- NVME_TARGET_CONTROL: When defined, the generic target setup/cleanup code will
> +  be skipped and this script gets called. This makes it possible to run
> +  the fabric nvme tests against a real target.
>  
>  ### Running nvme-rdma and SRP tests
>  
> @@ -167,3 +170,33 @@ if ! findmnt -t configfs /sys/kernel/config > /dev/null; then
>  	mount -t configfs configfs /sys/kernel/config
>  fi
>  ```
> +### NVME_TARGET_CONTROL
> +
> +When NVME_TARGET_CONTROL is set, blktests will call the script which the
> +environment variable points to, to fetch the configuration values to be used for
> +the runs, e.g subsysnqn or hostnqn. This allows the blktest to be run against
> +external configured/setup targets.
> +
> +The blktests expects that the script interface implements following
> +commands:
> +
> +config:
> +  --show-blkdev-type
> +  --show-trtype
> +  --show-hostnqn
> +  --show-hostid
> +  --show-host-traddr
> +  --show-traddr
> +  --show-trsvid
> +  --show-subsys-uuid
> +  --show-subsysnqn
> +
> +setup:
> +  --subsysnqn SUBSYSNQN
> +  --subsys-uuid SUBSYS_UUID
> +  --hostnqn HOSTNQN
> +  --ctrlkey CTRLKEY
> +  --hostkey HOSTKEY
> +
> +cleanup:
> +  --subsysnqn SUBSYSNQN

Thanks. I think the NVME_TARGET_CONTROL script command line interface is well
documented.

> diff --git a/check b/check
> index 3ed4510f3f40..d0475629773d 100755
> --- a/check
> +++ b/check
> @@ -603,6 +603,10 @@ _run_group() {
>  	# shellcheck disable=SC1090
>  	. "tests/${group}/rc"
>  
> +	if declare -fF group_setup >/dev/null; then
> +		group_setup
> +	fi

This new hook adds some complexity, but I can not think of better way. So, I
agree to add this hook.

> +
>  	if declare -fF group_requires >/dev/null; then
>  		group_requires
>  		if [[ -v SKIP_REASONS ]]; then
> diff --git a/tests/nvme/rc b/tests/nvme/rc
> index c1ddf412033b..4465dea0370b 100644
> --- a/tests/nvme/rc
> +++ b/tests/nvme/rc
> @@ -23,6 +23,7 @@ _check_conflict_and_set_default NVME_IMG_SIZE nvme_img_size 1G
>  _check_conflict_and_set_default NVME_NUM_ITER nvme_num_iter 1000
>  nvmet_blkdev_type=${nvmet_blkdev_type:-"device"}
>  NVMET_BLKDEV_TYPES=${NVMET_BLKDEV_TYPES:-"device file"}
> +nvme_target_control="${NVME_TARGET_CONTROL:-}"
>  
>  _NVMET_TRTYPES_is_valid() {
>  	local type
> @@ -135,6 +136,13 @@ _nvme_requires() {
>  	return 0
>  }
>  
> +group_setup() {
> +	if [[ -n "${nvme_target_control}" ]]; then
> +		NVMET_TRTYPES="$(${nvme_target_control} config --show-trtype)"
> +		NVMET_BLKDEV_TYPES="$(${nvme_target_control} config --show-blkdev-type)"
> +	fi
> +}
> +
>  group_requires() {
>  	_have_root
>  	_NVMET_TRTYPES_is_valid
> @@ -359,6 +367,10 @@ _cleanup_nvmet() {
>  		fi
>  	done
>  
> +	if [[ -n "${nvme_target_control}" ]]; then
> +		return
> +	fi
> +
>  	for port in "${NVMET_CFS}"/ports/*; do
>  		name=$(basename "${port}")
>  		echo "WARNING: Test did not clean up port: ${name}"
> @@ -403,11 +415,26 @@ _cleanup_nvmet() {
>  
>  _setup_nvmet() {
>  	_register_test_cleanup _cleanup_nvmet
> +
> +	if [[ -n "${nvme_target_control}" ]]; then
> +		def_hostnqn="$(${nvme_target_control} config --show-hostnqn)"
> +		def_hostid="$(${nvme_target_control} config --show-hostid)"
> +		def_host_traddr="$(${nvme_target_control} config --show-host-traddr)"
> +		def_traddr="$(${nvme_target_control} config --show-traddr)"
> +		def_trsvcid="$(${nvme_target_control} config --show-trsvid)"
> +		def_subsys_uuid="$(${nvme_target_control} config --show-subsys-uuid)"
> +		def_subsysnqn="$(${nvme_target_control} config --show-subsysnqn)"

I guess that the lines above caused unpredictable values in $def_*, then
caused many of the shellcheck warnings in tests/nvme/*. I'm afraid that another
commit will be required to modify tests/nvme/* and address the warnings.

> +		return
> +	fi
> +
>  	modprobe -q nvmet
> +
>  	if [[ "${nvme_trtype}" != "loop" ]]; then
>  		modprobe -q nvmet-"${nvme_trtype}"
>  	fi
> +
>  	modprobe -q nvme-"${nvme_trtype}"
> +
>  	if [[ "${nvme_trtype}" == "rdma" ]]; then
>  		start_soft_rdma
>  		for i in $(rdma_network_interfaces)
> @@ -425,6 +452,7 @@ _setup_nvmet() {
>  			fi
>  		done
>  	fi
> +
>  	if [[ "${nvme_trtype}" = "fc" ]]; then
>  		modprobe -q nvme-fcloop
>  		_setup_fcloop "${def_local_wwnn}" "${def_local_wwpn}" \
> @@ -873,11 +901,13 @@ _find_nvme_passthru_loop_dev() {
>  
>  _nvmet_target_setup() {
>  	local blkdev_type="${nvmet_blkdev_type}"
> +	local subsys_uuid="${def_subsys_uuid}"
> +	local subsysnqn="${def_subsysnqn}"
>  	local blkdev
> +	local ARGS=()
>  	local ctrlkey=""
>  	local hostkey=""
> -	local subsysnqn="${def_subsysnqn}"
> -	local subsys_uuid="${def_subsys_uuid}"
> +	local blkdev
>  	local port
>  
>  	while [[ $# -gt 0 ]]; do
> @@ -909,6 +939,22 @@ _nvmet_target_setup() {
>  		esac
>  	done
>  
> +	if [[ -n "${hostkey}" ]]; then
> +		ARGS+=(--hostkey "${hostkey}")
> +	fi
> +	if [[ -n "${ctrlkey}" ]]; then
> +		ARGS+=(--ctrkey "${ctrlkey}")
> +	fi
> +
> +	if [[ -n "${nvme_target_control}" ]]; then
> +		eval "${nvme_target_control}" setup \
> +			--subsysnqn "${subsysnqn}" \
> +			--subsys-uuid "${subsys_uuid}" \
> +			--hostnqn "${def_hostnqn}" \
> +			"${ARGS[@]}" &> /dev/null

I suggest to replaces ${ARGS[@]} with ${ARGS[*]}. It avoids one of the
shellcheck warnings, hopefully.

> +		return
> +	fi
> +
>  	truncate -s "${NVME_IMG_SIZE}" "$(_nvme_def_file_path)"
>  	if [[ "${blkdev_type}" == "device" ]]; then
>  		blkdev="$(losetup -f --show "$(_nvme_def_file_path)")"
> @@ -948,6 +994,13 @@ _nvmet_target_cleanup() {
>  		esac
>  	done
>  
> +	if [[ -n "${nvme_target_control}" ]]; then
> +		eval "${nvme_target_control}" cleanup \
> +			--subsysnqn "${subsysnqn}" \
> +			> /dev/null
> +		return
> +	fi
> +
>  	_get_nvmet_ports "${subsysnqn}" ports
>  
>  	for port in "${ports[@]}"; do
> -- 
> 2.45.2
> 

Re: [PATCH blktests v3 1/3] nvme/rc: introduce remote target support

[Shinichiro Kawasaki]

On Jul 02, 2024 / 17:51, Shin'ichiro Kawasaki wrote:
> CC+: Ofir

Sorry, I forgot to add Ofir to the CC list. Let me resend this note.

> 
> A heads up: this patch causes conflict with Ofir's patch to move tests/nvme/rc
> helpers to common/nvme [*]. Depending on which comes first, Daniel or Ofir will
> need patch rework.
> 
> [*] https://lore.kernel.org/linux-block/20240624104620.2156041-2-ofir.gal@volumez.com/
> 
> 
> On Jun 27, 2024 / 11:10, Daniel Wagner wrote:
> > Most of the NVMEeoF tests are exercising the host code of the nvme
> > subsystem. There is no real reason not to run these against a real
> > target. We just have to skip the soft target setup and make it possible
> > to setup a remote target.
> > 
> > Because all tests use now the common setup/cleanup helpers we just need
> > to intercept this call and forward it to an external component.
> > 
> > As we already have various nvme variables to setup the target which we
> > should allow to overwrite. Also introduce a NVME_TARGET_CONTROL variable
> > which points to a script which gets executed whenever a targets needs to
> > be created/destroyed.
> > 
> > Signed-off-by: Daniel Wagner <dwagner@suse.de>
> 
> Daniel, thanks for this v3 patch. Please find some comments in line.
> I ran "make check" and observed some shellcheck warnings:
> 
> $ make check
> shellcheck -x -e SC2119 -f gcc check common/* \
>         tests/*/rc tests/*/[0-9]*[0-9] src/*.sh
> tests/nvme/rc:962:5: warning: eval negates the benefit of arrays. Drop eval to preserve whitespace/symbols (or eval as string). [SC2294]
> tests/nvme/041:34:36: note: Double quote to prevent globbing and word splitting. [SC2086]
> tests/nvme/042:40:52: note: Double quote to prevent globbing and word splitting. [SC2086]
> tests/nvme/042:54:61: note: Double quote to prevent globbing and word splitting. [SC2086]
> tests/nvme/043:37:36: note: Double quote to prevent globbing and word splitting. [SC2086]
> tests/nvme/044:36:36: note: Double quote to prevent globbing and word splitting. [SC2086]
> tests/nvme/044:42:36: note: Double quote to prevent globbing and word splitting. [SC2086]
> tests/nvme/045:41:36: note: Double quote to prevent globbing and word splitting. [SC2086]
> tests/nvme/045:47:36: note: Double quote to prevent globbing and word splitting. [SC2086]
> tests/nvme/045:72:43: note: Double quote to prevent globbing and word splitting. [SC2086]
> tests/nvme/045:82:43: note: Double quote to prevent globbing and word splitting. [SC2086]
> tests/nvme/051:40:25: note: Double quote to prevent globbing and word splitting. [SC2086]
> tests/nvme/051:41:25: note: Double quote to prevent globbing and word splitting. [SC2086]
> 
> > ---
> >  Documentation/running-tests.md | 33 ++++++++++++++++++++
> >  check                          |  4 +++
> >  tests/nvme/rc                  | 57 ++++++++++++++++++++++++++++++++--
> >  3 files changed, 92 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/running-tests.md b/Documentation/running-tests.md
> > index 968702e76bb5..fe4f729bd037 100644
> > --- a/Documentation/running-tests.md
> > +++ b/Documentation/running-tests.md
> > @@ -120,6 +120,9 @@ The NVMe tests can be additionally parameterized via environment variables.
> >  - NVME_NUM_ITER: 1000 (default)
> >    The number of iterations a test should do. This parameter had an old name
> >    'nvme_num_iter'. The old name is still usable, but not recommended.
> > +- NVME_TARGET_CONTROL: When defined, the generic target setup/cleanup code will
> > +  be skipped and this script gets called. This makes it possible to run
> > +  the fabric nvme tests against a real target.
> >  
> >  ### Running nvme-rdma and SRP tests
> >  
> > @@ -167,3 +170,33 @@ if ! findmnt -t configfs /sys/kernel/config > /dev/null; then
> >  	mount -t configfs configfs /sys/kernel/config
> >  fi
> >  ```
> > +### NVME_TARGET_CONTROL
> > +
> > +When NVME_TARGET_CONTROL is set, blktests will call the script which the
> > +environment variable points to, to fetch the configuration values to be used for
> > +the runs, e.g subsysnqn or hostnqn. This allows the blktest to be run against
> > +external configured/setup targets.
> > +
> > +The blktests expects that the script interface implements following
> > +commands:
> > +
> > +config:
> > +  --show-blkdev-type
> > +  --show-trtype
> > +  --show-hostnqn
> > +  --show-hostid
> > +  --show-host-traddr
> > +  --show-traddr
> > +  --show-trsvid
> > +  --show-subsys-uuid
> > +  --show-subsysnqn
> > +
> > +setup:
> > +  --subsysnqn SUBSYSNQN
> > +  --subsys-uuid SUBSYS_UUID
> > +  --hostnqn HOSTNQN
> > +  --ctrlkey CTRLKEY
> > +  --hostkey HOSTKEY
> > +
> > +cleanup:
> > +  --subsysnqn SUBSYSNQN
> 
> Thanks. I think the NVME_TARGET_CONTROL script command line interface is well
> documented.
> 
> > diff --git a/check b/check
> > index 3ed4510f3f40..d0475629773d 100755
> > --- a/check
> > +++ b/check
> > @@ -603,6 +603,10 @@ _run_group() {
> >  	# shellcheck disable=SC1090
> >  	. "tests/${group}/rc"
> >  
> > +	if declare -fF group_setup >/dev/null; then
> > +		group_setup
> > +	fi
> 
> This new hook adds some complexity, but I can not think of better way. So, I
> agree to add this hook.
> 
> > +
> >  	if declare -fF group_requires >/dev/null; then
> >  		group_requires
> >  		if [[ -v SKIP_REASONS ]]; then
> > diff --git a/tests/nvme/rc b/tests/nvme/rc
> > index c1ddf412033b..4465dea0370b 100644
> > --- a/tests/nvme/rc
> > +++ b/tests/nvme/rc
> > @@ -23,6 +23,7 @@ _check_conflict_and_set_default NVME_IMG_SIZE nvme_img_size 1G
> >  _check_conflict_and_set_default NVME_NUM_ITER nvme_num_iter 1000
> >  nvmet_blkdev_type=${nvmet_blkdev_type:-"device"}
> >  NVMET_BLKDEV_TYPES=${NVMET_BLKDEV_TYPES:-"device file"}
> > +nvme_target_control="${NVME_TARGET_CONTROL:-}"
> >  
> >  _NVMET_TRTYPES_is_valid() {
> >  	local type
> > @@ -135,6 +136,13 @@ _nvme_requires() {
> >  	return 0
> >  }
> >  
> > +group_setup() {
> > +	if [[ -n "${nvme_target_control}" ]]; then
> > +		NVMET_TRTYPES="$(${nvme_target_control} config --show-trtype)"
> > +		NVMET_BLKDEV_TYPES="$(${nvme_target_control} config --show-blkdev-type)"
> > +	fi
> > +}
> > +
> >  group_requires() {
> >  	_have_root
> >  	_NVMET_TRTYPES_is_valid
> > @@ -359,6 +367,10 @@ _cleanup_nvmet() {
> >  		fi
> >  	done
> >  
> > +	if [[ -n "${nvme_target_control}" ]]; then
> > +		return
> > +	fi
> > +
> >  	for port in "${NVMET_CFS}"/ports/*; do
> >  		name=$(basename "${port}")
> >  		echo "WARNING: Test did not clean up port: ${name}"
> > @@ -403,11 +415,26 @@ _cleanup_nvmet() {
> >  
> >  _setup_nvmet() {
> >  	_register_test_cleanup _cleanup_nvmet
> > +
> > +	if [[ -n "${nvme_target_control}" ]]; then
> > +		def_hostnqn="$(${nvme_target_control} config --show-hostnqn)"
> > +		def_hostid="$(${nvme_target_control} config --show-hostid)"
> > +		def_host_traddr="$(${nvme_target_control} config --show-host-traddr)"
> > +		def_traddr="$(${nvme_target_control} config --show-traddr)"
> > +		def_trsvcid="$(${nvme_target_control} config --show-trsvid)"
> > +		def_subsys_uuid="$(${nvme_target_control} config --show-subsys-uuid)"
> > +		def_subsysnqn="$(${nvme_target_control} config --show-subsysnqn)"
> 
> I guess that the lines above caused unpredictable values in $def_*, then
> caused many of the shellcheck warnings in tests/nvme/*. I'm afraid that another
> commit will be required to modify tests/nvme/* and address the warnings.
> 
> > +		return
> > +	fi
> > +
> >  	modprobe -q nvmet
> > +
> >  	if [[ "${nvme_trtype}" != "loop" ]]; then
> >  		modprobe -q nvmet-"${nvme_trtype}"
> >  	fi
> > +
> >  	modprobe -q nvme-"${nvme_trtype}"
> > +
> >  	if [[ "${nvme_trtype}" == "rdma" ]]; then
> >  		start_soft_rdma
> >  		for i in $(rdma_network_interfaces)
> > @@ -425,6 +452,7 @@ _setup_nvmet() {
> >  			fi
> >  		done
> >  	fi
> > +
> >  	if [[ "${nvme_trtype}" = "fc" ]]; then
> >  		modprobe -q nvme-fcloop
> >  		_setup_fcloop "${def_local_wwnn}" "${def_local_wwpn}" \
> > @@ -873,11 +901,13 @@ _find_nvme_passthru_loop_dev() {
> >  
> >  _nvmet_target_setup() {
> >  	local blkdev_type="${nvmet_blkdev_type}"
> > +	local subsys_uuid="${def_subsys_uuid}"
> > +	local subsysnqn="${def_subsysnqn}"
> >  	local blkdev
> > +	local ARGS=()
> >  	local ctrlkey=""
> >  	local hostkey=""
> > -	local subsysnqn="${def_subsysnqn}"
> > -	local subsys_uuid="${def_subsys_uuid}"
> > +	local blkdev
> >  	local port
> >  
> >  	while [[ $# -gt 0 ]]; do
> > @@ -909,6 +939,22 @@ _nvmet_target_setup() {
> >  		esac
> >  	done
> >  
> > +	if [[ -n "${hostkey}" ]]; then
> > +		ARGS+=(--hostkey "${hostkey}")
> > +	fi
> > +	if [[ -n "${ctrlkey}" ]]; then
> > +		ARGS+=(--ctrkey "${ctrlkey}")
> > +	fi
> > +
> > +	if [[ -n "${nvme_target_control}" ]]; then
> > +		eval "${nvme_target_control}" setup \
> > +			--subsysnqn "${subsysnqn}" \
> > +			--subsys-uuid "${subsys_uuid}" \
> > +			--hostnqn "${def_hostnqn}" \
> > +			"${ARGS[@]}" &> /dev/null
> 
> I suggest to replaces ${ARGS[@]} with ${ARGS[*]}. It avoids one of the
> shellcheck warnings, hopefully.
> 
> > +		return
> > +	fi
> > +
> >  	truncate -s "${NVME_IMG_SIZE}" "$(_nvme_def_file_path)"
> >  	if [[ "${blkdev_type}" == "device" ]]; then
> >  		blkdev="$(losetup -f --show "$(_nvme_def_file_path)")"
> > @@ -948,6 +994,13 @@ _nvmet_target_cleanup() {
> >  		esac
> >  	done
> >  
> > +	if [[ -n "${nvme_target_control}" ]]; then
> > +		eval "${nvme_target_control}" cleanup \
> > +			--subsysnqn "${subsysnqn}" \
> > +			> /dev/null
> > +		return
> > +	fi
> > +
> >  	_get_nvmet_ports "${subsysnqn}" ports
> >  
> >  	for port in "${ports[@]}"; do
> > -- 
> > 2.45.2
> >