[PATCH blktests v4 0/5] Add support to run against arbitrary targets

[PATCH blktests v4 0/5] Add support to run against arbitrary targets

[Aurelien Aptel]

This series is based off Daniel's patches. It is rebased to work on
current blktests. It is also available on github:

    https://github.com/aaptel/blktests.git branch remote-target-v4

The last patch adds a test making explicit use of the remote target
support by testing the new nvme-tcp offloading.

I've added a 'blkdev' setting under [subsys_0] in the control script
config to specify the backing block device on the target, instead of
hardcoding '/dev/vdc'.

changes:
v4:
  - added nvme test 055 for nvme-tcp offload
  - added 'blkdev' setting under [subsys_0] in the control script config
  - added workaround for older python versions
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/20240627091016.12752-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
  - https://lore.kernel.org/all/20240612110444.4507-1-dwagner@suse.de/
v1:
  - initial version
  - https://lore.kernel.org/linux-nvme/20240318093856.22307-1-dwagner@suse.de/

Aurelien Aptel (2):
  common/nvme: add digest options to __nvme_connect_subsys()
  nvme/055: add test for nvme-tcp zero-copy offload

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 |  42 +++++
 README.md                      |   1 +
 check                          |   4 +
 common/nvme                    |  87 +++++++++-
 common/rc                      |   8 +
 contrib/nvme_target_control.py | 190 ++++++++++++++++++++++
 contrib/nvmet-subsys.jinja2    |  71 ++++++++
 tests/nvme/030                 |   1 +
 tests/nvme/055                 | 285 +++++++++++++++++++++++++++++++++
 tests/nvme/055.out             |  44 +++++
 tests/nvme/rc                  |  30 ++++
 11 files changed, 755 insertions(+), 8 deletions(-)
 create mode 100755 contrib/nvme_target_control.py
 create mode 100644 contrib/nvmet-subsys.jinja2
 create mode 100755 tests/nvme/055
 create mode 100644 tests/nvme/055.out

-- 
2.34.1

[PATCH blktests v4 1/5] nvme/rc: introduce remote target support

[Aurelien Aptel]

From: Daniel Wagner <dwagner@suse.de>

Most of the NVMEeoF tests are exercising the host code of the nvme
subsystem. There is no real reason not to run these against an arbitrary
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>
Signed-off-by: Aurelien Aptel <aaptel@nvidia.com>
---
 Documentation/running-tests.md | 33 ++++++++++++++++
 check                          |  4 ++
 common/nvme                    | 71 ++++++++++++++++++++++++++++++----
 tests/nvme/rc                  | 14 +++++++
 4 files changed, 114 insertions(+), 8 deletions(-)

diff --git a/Documentation/running-tests.md b/Documentation/running-tests.md
index 968702e..fe4f729 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 7f43a31..dad5e70 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/common/nvme b/common/nvme
index fd472fe..f99af5a 100644
--- a/common/nvme
+++ b/common/nvme
@@ -22,6 +22,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_CFS="/sys/kernel/config/nvmet/"
 nvme_trtype=${nvme_trtype:-}
 nvme_adrfam=${nvme_adrfam:-}
@@ -157,6 +158,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}"
@@ -208,6 +213,18 @@ _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}"
@@ -320,17 +337,23 @@ _nvme_connect_subsys() {
 		esac
 	done
 
-	if [[ -z "${port}" ]]; then
-		local ports
-
-		_get_nvmet_ports "${subsysnqn}" ports
-		port="${ports[0]##*/}"
+	if [[ -n "${nvme_target_control}" && -z "${port}" ]]; then
+		ARGS+=(--transport "$(${nvme_target_control} config --show-trtype)")
+		ARGS+=(--traddr "${def_traddr}")
+		ARGS+=(--trsvcid "${def_trsvcid}")
+	else
 		if [[ -z "${port}" ]]; then
-			echo "WARNING: no port found"
-			return 1
+			local ports
+
+			_get_nvmet_ports "${subsysnqn}" ports
+			port="${ports[0]##*/}"
+			if [[ -z "${port}" ]]; then
+				echo "WARNING: no port found"
+				return 1
+			fi
 		fi
+		_get_nvmet_port_params "${port}" ARGS
 	fi
-	_get_nvmet_port_params "${port}" ARGS
 	ARGS+=(--nqn "${subsysnqn}")
 	ARGS+=(--hostnqn="${hostnqn}")
 	ARGS+=(--hostid="${hostid}")
@@ -762,11 +785,13 @@ _find_nvme_ns() {
 _nvmet_target_setup() {
 	local blkdev_type="${nvmet_blkdev_type}"
 	local blkdev
+	local ARGS=()
 	local ctrlkey=""
 	local hostkey=""
 	local subsysnqn="${def_subsysnqn}"
 	local subsys_uuid
 	local port
+	local resv_enable=""
 	local -a ARGS
 
 	while [[ $# -gt 0 ]]; do
@@ -811,6 +836,29 @@ _nvmet_target_setup() {
 		fi
 	fi
 
+	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:-$def_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)")"
+	else
+		blkdev="$(_nvme_def_file_path)"
+	fi
+
 	ARGS=(--subsysnqn "${subsysnqn}")
 	if [[ -n "${blkdev}" ]]; then
 		ARGS+=(--blkdev "${blkdev}")
@@ -853,6 +901,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
diff --git a/tests/nvme/rc b/tests/nvme/rc
index d63afd1..9ad9a52 100644
--- a/tests/nvme/rc
+++ b/tests/nvme/rc
@@ -113,6 +113,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
@@ -392,6 +399,13 @@ _nvmet_passthru_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.34.1

[PATCH blktests v4 2/5] common/nvme: add digest options to __nvme_connect_subsys()

[Aurelien Aptel]

This commit lets tests connect nvme subsystems with data and header
digest.

Signed-off-by: Aurelien Aptel <aaptel@nvidia.com>
Reviewed-by: Daniel Wagner <dwagner@suse.de>
---
 common/nvme | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/common/nvme b/common/nvme
index f99af5a..e5e5344 100644
--- a/common/nvme
+++ b/common/nvme
@@ -272,6 +272,8 @@ _nvme_connect_subsys() {
 	local reconnect_delay=""
 	local ctrl_loss_tmo=""
 	local no_wait=false
+	local hdr_digest=false
+	local data_digest=false
 	local port
 	local i
 	local -a ARGS
@@ -330,6 +332,14 @@ _nvme_connect_subsys() {
 				no_wait=true
 				shift 1
 				;;
+			--hdr-digest)
+				hdr_digest=true
+				shift 1
+				;;
+			--data-digest)
+				data_digest=true
+				shift 1
+				;;
 			*)
 				echo "WARNING: unknown argument: $1"
 				shift
@@ -381,6 +391,12 @@ _nvme_connect_subsys() {
 	if [[ -n "${ctrl_loss_tmo}" ]]; then
 		ARGS+=(--ctrl-loss-tmo="${ctrl_loss_tmo}")
 	fi
+	if [[ ${hdr_digest} = true ]]; then
+		ARGS+=(--hdr-digest)
+	fi
+	if [[ ${data_digest} = true ]]; then
+		ARGS+=(--data-digest)
+	fi
 	ARGS+=(-o json)
 	connect=$(nvme connect "${ARGS[@]}" 2> /dev/null)
 
-- 
2.34.1

[PATCH blktests v4 3/5] nvme/030: only run against kernel soft target

[Aurelien Aptel]

From: Daniel Wagner <dwagner@suse.de>

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 an arbitrary target. Just skip it when we run against a
arbitrary 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 596e411..fe74849 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 9ad9a52..d1a4c01 100644
--- a/tests/nvme/rc
+++ b/tests/nvme/rc
@@ -191,6 +191,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.34.1

[PATCH blktests v4 4/5] contrib: add remote target setup/cleanup script

[Aurelien Aptel]

From: Daniel Wagner <dwagner@suse.de>

Use nvmetcli to setup/cleanup a remote soft target.

Signed-off-by: Daniel Wagner <dwagner@suse.de>
Signed-off-by: Aurelien Aptel <aaptel@nvidia.com>
---
 contrib/nvme_target_control.py | 190 +++++++++++++++++++++++++++++++++
 contrib/nvmet-subsys.jinja2    |  71 ++++++++++++
 2 files changed, 261 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 0000000..db77fe3
--- /dev/null
+++ b/contrib/nvme_target_control.py
@@ -0,0 +1,190 @@
+#!/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'
+# blkdev='/dev/nullb0'
+#
+# 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
+
+# workaround for python<3.11
+TOML_OPEN_MODE="rb"
+try:
+    import tomllib
+except ModuleNotFoundError:
+    import pip._vendor.tomli as tomllib
+    TOML_OPEN_MODE="r"
+
+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', TOML_OPEN_MODE) 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': config['subsys_0']['blkdev'],
+    }
+
+    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 0000000..a446fbd
--- /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.34.1

[PATCH blktests v4 5/5] nvme/055: add test for nvme-tcp zero-copy offload

[Aurelien Aptel]

This commit adds a new test for the kernel ULP DDP (Direct Data
Placement) feature with NVMe-TCP.

Configuration of DDP is per NIC and is done through a script in the
kernel source. For this reason we add 2 new config vars:
- KERNELSRC: path to the running kernel sources
- NVME_IFACE: name of the network interface to configure the offload on

Signed-off-by: Aurelien Aptel <aaptel@nvidia.com>
Signed-off-by: Shai Malin smalin@nvidia.com
Reviewed-by: Daniel Wagner <dwagner@suse.de>
---
 Documentation/running-tests.md |   9 ++
 README.md                      |   1 +
 common/rc                      |   8 +
 tests/nvme/055                 | 285 +++++++++++++++++++++++++++++++++
 tests/nvme/055.out             |  44 +++++
 tests/nvme/rc                  |   8 +
 6 files changed, 355 insertions(+)
 create mode 100755 tests/nvme/055
 create mode 100644 tests/nvme/055.out

diff --git a/Documentation/running-tests.md b/Documentation/running-tests.md
index fe4f729..a42fc91 100644
--- a/Documentation/running-tests.md
+++ b/Documentation/running-tests.md
@@ -124,6 +124,15 @@ The NVMe tests can be additionally parameterized via environment variables.
   be skipped and this script gets called. This makes it possible to run
   the fabric nvme tests against a real target.
 
+#### NVMe-TCP zero-copy offload
+
+The NVMe-TCP ZC offload tests use a couple more variables.
+
+- KERNELSRC: Path to running kernel sources.
+  Needed for the script to configure the offload.
+- NVME_IFACE: Name of the interface the offload should be enabled on.
+  This should be the same interface the NVMe connection is made with.
+
 ### Running nvme-rdma and SRP tests
 
 These tests will use the siw (soft-iWARP) driver by default. The rdma_rxe
diff --git a/README.md b/README.md
index 55227d9..5073510 100644
--- a/README.md
+++ b/README.md
@@ -30,6 +30,7 @@ Some tests require the following:
 - nbd-client and nbd-server (Debian) or nbd (Fedora, openSUSE, Arch Linux)
 - dmsetup (Debian) or device-mapper (Fedora, openSUSE, Arch Linux)
 - rublk (`cargo install --version=^0.1 rublk`) for ublk test
+- python3, ethtool, iproute2 for nvme-tcp zero-copy offload test
 
 Build blktests with `make`. Optionally, install it to a known location with
 `make install` (`/usr/local/blktests` by default, but this can be changed by
diff --git a/common/rc b/common/rc
index b2e68b2..0c8b51f 100644
--- a/common/rc
+++ b/common/rc
@@ -148,6 +148,14 @@ _have_loop() {
 	_have_driver loop && _have_program losetup
 }
 
+_have_kernel_source() {
+	if [ -z "${KERNELSRC}" ]; then
+		SKIP_REASONS+=("KERNELSRC not set")
+		return 1
+	fi
+	return 0
+}
+
 _have_blktrace() {
 	# CONFIG_BLK_DEV_IO_TRACE might still be disabled, but this is easier
 	# to check. We can fix it if someone complains.
diff --git a/tests/nvme/055 b/tests/nvme/055
new file mode 100755
index 0000000..7e76126
--- /dev/null
+++ b/tests/nvme/055
@@ -0,0 +1,285 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2024 Aurelien Aptel <aaptel@nvidia.com>
+#
+# zero-copy offload
+
+. tests/nvme/rc
+
+DESCRIPTION="enable zero copy offload and run rw traffic"
+TIMED=1
+
+iface_idx=""
+
+# these vars get updated after each call to connect_run_disconnect()
+nb_packets=0
+nb_bytes=0
+nb_offload_packets=0
+nb_offload_bytes=0
+offload_bytes_ratio=0
+offload_packets_ratio=0
+
+requires() {
+	_nvme_requires
+	_require_remote_nvme_target
+	_require_nvme_trtype tcp
+	_have_kernel_option ULP_DDP
+	# require nvme-tcp as a module to be able to change the ddp_offload param
+	_have_module nvme_tcp && _have_module_param nvme_tcp ddp_offload
+	_have_fio
+	_have_program ip
+	_have_program ethtool
+	_have_kernel_source && have_netlink_cli && _have_program python3
+	have_iface
+}
+
+have_netlink_cli() {
+	local cli
+	cli="${KERNELSRC}/tools/net/ynl/cli.py"
+
+	if ! [ -f "$cli" ]; then
+		SKIP_REASONS+=("Kernel sources do not have tools/net/ynl/cli.py")
+		return 1
+	fi
+
+	if ! "$cli" -h &> /dev/null; then
+		SKIP_REASONS+=("Cannot run the kernel tools/net/ynl/cli.py")
+		return 1;
+	fi
+
+	if ! [ -f "${KERNELSRC}/Documentation/netlink/specs/ulp_ddp.yaml" ]; then
+		SKIP_REASONS+=("Kernel sources do not have the ULP DDP netlink specs")
+		return 1
+	fi
+}
+
+have_iface() {
+	if [ -z "${NVME_IFACE}" ]; then
+		SKIP_REASONS+=("NVME_IFACE not set")
+		return 1
+	fi
+	return 0
+}
+
+set_conditions() {
+	_set_nvme_trtype "$@"
+}
+
+netlink_cli() {
+	"${KERNELSRC}/tools/net/ynl/cli.py" \
+		--spec "${KERNELSRC}/Documentation/netlink/specs/ulp_ddp.yaml" \
+		"$@"
+}
+
+eth_stat() {
+	ethtool -S "${NVME_IFACE}" | awk "/ $1:/ { print \$2 }"
+}
+
+ddp_stat() {
+	netlink_cli --do stats-get --json "{\"ifindex\": $iface_idx}" \
+		| awk -F: "/'$1'/{print \$2;}" | tr -d '{},'
+}
+
+ddp_caps() {
+	local out
+	out="$(netlink_cli --do caps-get --json "{\"ifindex\": $iface_idx}")"
+	echo "$out" | tr '{},' '\n' | tr -d ' '| awk -F: "/$1/ { print \$2 }"
+}
+
+configure_ddp() {
+	local mod_param
+	local cap
+
+	mod_param=$1
+	cap=$2
+
+	echo "=== configured with ddp_offload=$mod_param and caps=$cap ==="
+
+	# set ddp_offload module param
+	modprobe -q -r nvme-tcp
+	modprobe -q nvme-tcp ddp_offload=$mod_param
+
+	# set capabilities
+	netlink_cli --do caps-set --json "{\"ifindex\": $iface_idx, \"wanted\": $cap, \"wanted_mask\": 3}" >> "$FULL" 2>&1
+}
+
+connect_run_disconnect() {
+	local io_size
+	local nvme_dev
+	local nb_drop
+	local drop_ratio
+	local nb_resync
+	local resync_ratio
+
+	# offload stat counters
+	local start_sk_add
+	local start_sk_add_fail
+	local start_sk_del
+	local start_setup
+	local start_setup_fail
+	local start_teardown
+	local start_off_bytes
+	local start_eth_bytes
+	local start_off_packets
+	local start_eth_packets
+	local end_sk_add
+	local end_sk_add_fail
+	local end_sk_del
+	local end_setup
+	local end_setup_fail
+	local end_teardown
+	local end_drop
+	local end_resync
+	local end_off_bytes
+	local end_eth_bytes
+	local end_off_packets
+	local end_eth_packets
+
+	io_size=$1
+
+	start_sk_add=$(ddp_stat rx-nvme-tcp-sk-add)
+	start_sk_add_fail=$(ddp_stat rx-nvme-tcp-sk-add-fail)
+	start_sk_del=$(ddp_stat rx-nvme-tcp-sk-del)
+	start_setup=$(ddp_stat rx-nvme-tcp-setup)
+	start_setup_fail=$(ddp_stat rx-nvme-tcp-setup-fail)
+	start_teardown=$(ddp_stat rx-nvme-tcp-teardown)
+	start_drop=$(ddp_stat rx-nvme-tcp-drop)
+	start_resync=$(ddp_stat rx-nvme-tcp-resync)
+	start_off_packets=$(ddp_stat rx-nvme-tcp-packets)
+	start_off_bytes=$(ddp_stat rx-nvme-tcp-bytes)
+	start_eth_packets=$(eth_stat rx_packets)
+	start_eth_bytes=$(eth_stat rx_bytes)
+	_nvme_connect_subsys --hdr-digest --data-digest --nr-io-queues 8
+
+	nvme_dev="/dev/$(_find_nvme_ns "${def_subsys_uuid}")"
+
+	local common_args=(
+		--blocksize_range=$io_size
+		--rw=randrw
+		--numjobs=8
+		--iodepth=128
+		--name=randrw
+		--ioengine=libaio
+		--time_based
+		--runtime="$TIMEOUT"
+		--direct=1
+		--invalidate=1
+		--randrepeat=1
+		--norandommap
+		--filename="$nvme_dev"
+	)
+
+	echo "IO size: $io_size"
+
+	_run_fio "${common_args[@]}"
+	_nvme_disconnect_subsys >> "$FULL" 2>&1
+
+	end_sk_add=$(ddp_stat rx-nvme-tcp-sk-add)
+	end_sk_add_fail=$(ddp_stat rx-nvme-tcp-sk-add-fail)
+	end_sk_del=$(ddp_stat rx-nvme-tcp-sk-del)
+	end_setup=$(ddp_stat rx-nvme-tcp-setup)
+	end_setup_fail=$(ddp_stat rx-nvme-tcp-setup-fail)
+	end_teardown=$(ddp_stat rx-nvme-tcp-teardown)
+	end_drop=$(ddp_stat rx-nvme-tcp-drop)
+	end_resync=$(ddp_stat rx-nvme-tcp-resync)
+	end_off_packets=$(ddp_stat rx-nvme-tcp-packets)
+	end_eth_packets=$(eth_stat rx_packets)
+	end_off_bytes=$(ddp_stat rx-nvme-tcp-bytes)
+	end_eth_bytes=$(eth_stat rx_bytes)
+
+	echo "Offloaded sockets: $((end_sk_add - start_sk_add))"
+	echo "Failed sockets:    $((end_sk_add_fail - start_sk_add_fail))"
+	echo "Unoffloaded sockets:   $((end_sk_del - start_sk_del))"
+	echo "Offload packet leaked: $((end_setup - end_teardown))"
+	echo "Failed packet setup:   $((end_setup_fail - start_setup_fail))"
+
+	# global var results
+	nb_drop=$(( end_drop - start_drop ))
+	nb_resync=$(( end_resync - start_resync ))
+	nb_packets=$(( end_eth_packets - start_eth_packets ))
+	nb_offload_packets=$(( end_off_packets - start_off_packets ))
+	nb_bytes=$(( end_eth_bytes - start_eth_bytes ))
+	nb_offload_bytes=$(( end_off_bytes - start_off_bytes ))
+
+	offload_packets_ratio=0
+	offload_bytes_ratio=0
+
+	# sanity check and avoid div by zero in ratio calculation
+	if [[ nb_bytes -eq 0 || nb_packets -eq 0 ]]; then
+		echo "No traffic: $nb_bytes bytes, $nb_packets packets"
+		return
+	fi
+
+	offload_packets_ratio=$(( nb_offload_packets*100/nb_packets ))
+	offload_bytes_ratio=$(( nb_offload_bytes*100/nb_bytes ))
+
+	drop_ratio=$(( nb_drop*100/nb_packets ))
+	resync_ratio=$(( nb_resync*100/nb_packets ))
+	[[ drop_ratio -gt 5 ]] && echo "High drop ratio: $drop_ratio %"
+	[[ resync_ratio -gt 5 ]] && echo "High resync ratio: $resync_ratio %"
+}
+
+test() {
+	local starting_ddp_config
+
+	: "${TIMEOUT:=30}"
+
+	echo "Running ${TEST_NAME}"
+
+	# get iface index
+	iface_idx=$(ip address | awk -F: "/${NVME_IFACE}/ { print \$1; exit; }")
+
+	# check hw supports ddp
+	if [[ $(( $(ddp_caps hw) & 3)) -ne 3 ]]; then
+		SKIP_REASONS+=("${NVME_IFACE} does not support nvme-tcp ddp offload")
+		return
+	fi
+
+	_setup_nvmet
+	_nvmet_target_setup
+
+	if [ "$(cat "/sys/module/nvme_tcp/parameters/ddp_offload")" = Y ]; then
+		starting_ddp_config="1 $(ddp_caps active)"
+	else
+		starting_ddp_config="0 $(ddp_caps active)"
+	fi
+
+	# if any of the offload knobs are disabled, no offload should occur
+	# and offloaded packets & bytes should be zero
+
+	configure_ddp 0 0
+	connect_run_disconnect 32k-1M
+	echo "Offloaded packets: $nb_offload_packets"
+	echo "Offloaded bytes: $nb_offload_bytes"
+
+	configure_ddp 0 3
+	connect_run_disconnect 32k-1M
+	echo "Offloaded packets: $nb_offload_packets"
+	echo "Offloaded bytes: $nb_offload_bytes"
+
+	configure_ddp 1 0
+	connect_run_disconnect 32k-1M
+	echo "Offloaded packets: $nb_offload_packets"
+	echo "Offloaded bytes: $nb_offload_bytes"
+
+	# if everything is enabled, the offload should happen for large IOs only
+	configure_ddp 1 3
+
+	connect_run_disconnect 32k-1M
+	[[ nb_offload_packets -lt 100 ]] && echo "Low offloaded packets: $nb_offload_packets"
+	[[ nb_offload_bytes -lt 32768 ]] && echo "Low offloaded bytes: $nb_offload_bytes"
+	[[ offload_bytes_ratio -lt 90 ]] && echo "Low offloaded bytes ratio: $offload_bytes_ratio %"
+	[[ offload_packets_ratio -lt 95 ]] && echo "Low offloaded packets ratio: $offload_packets_ratio %"
+
+	# small IO should be under the offload threshold, ratio should be zero
+	connect_run_disconnect 4k-16k
+	echo "Offload bytes ratio: $offload_bytes_ratio %"
+	echo "Offload packets ratio: $offload_packets_ratio %"
+
+	_nvmet_target_cleanup
+
+	# restore starting config
+	configure_ddp $starting_ddp_config > /dev/null
+
+	echo "Test complete"
+}
diff --git a/tests/nvme/055.out b/tests/nvme/055.out
new file mode 100644
index 0000000..06706a6
--- /dev/null
+++ b/tests/nvme/055.out
@@ -0,0 +1,44 @@
+Running nvme/055
+=== configured with ddp_offload=0 and caps=0 ===
+IO size: 32k-1M
+Offloaded sockets: 0
+Failed sockets:    0
+Unoffloaded sockets:   0
+Offload packet leaked: 0
+Failed packet setup:   0
+Offloaded packets: 0
+Offloaded bytes: 0
+=== configured with ddp_offload=0 and caps=3 ===
+IO size: 32k-1M
+Offloaded sockets: 0
+Failed sockets:    0
+Unoffloaded sockets:   0
+Offload packet leaked: 0
+Failed packet setup:   0
+Offloaded packets: 0
+Offloaded bytes: 0
+=== configured with ddp_offload=1 and caps=0 ===
+IO size: 32k-1M
+Offloaded sockets: 0
+Failed sockets:    0
+Unoffloaded sockets:   0
+Offload packet leaked: 0
+Failed packet setup:   0
+Offloaded packets: 0
+Offloaded bytes: 0
+=== configured with ddp_offload=1 and caps=3 ===
+IO size: 32k-1M
+Offloaded sockets: 8
+Failed sockets:    0
+Unoffloaded sockets:   8
+Offload packet leaked: 0
+Failed packet setup:   0
+IO size: 4k-16k
+Offloaded sockets: 8
+Failed sockets:    0
+Unoffloaded sockets:   8
+Offload packet leaked: 0
+Failed packet setup:   0
+Offload bytes ratio: 0 %
+Offload packets ratio: 0 %
+Test complete
diff --git a/tests/nvme/rc b/tests/nvme/rc
index d1a4c01..4a43e43 100644
--- a/tests/nvme/rc
+++ b/tests/nvme/rc
@@ -199,6 +199,14 @@ _require_kernel_nvme_target() {
 	return 0
 }
 
+_require_remote_nvme_target() {
+	if [ -z "${nvme_target_control}" ]; then
+		SKIP_REASONS+=("Remote target required but NVME_TARGET_CONTROL is not set")
+		return 1
+	fi
+	return 0
+}
+
 _test_dev_nvme_ctrl() {
 	echo "/dev/char/$(cat "${TEST_DEV_SYSFS}/device/dev")"
 }
-- 
2.34.1

Re: [PATCH blktests v4 1/5] nvme/rc: introduce remote target support

[Shinichiro Kawasaki]

[-- Attachment #1: Type: text/plain, Size: 3613 bytes --]

Hello Aurelien,

Thank you for rebasing the series. Please find my comments in line.

When I ran "make check", I observed some ShellCheck warnings. This check before
posts will be appreciated. As for the two ShellCheck warnings, please find my
comments below. This patch also triggered ShellCheck warnings in other files.
For those warnings, I created a fix patch and attached for your reference.

On Nov 26, 2024 / 22:38, Aurelien Aptel wrote:
[...]
> diff --git a/common/nvme b/common/nvme
> index fd472fe..f99af5a 100644
> --- a/common/nvme
> +++ b/common/nvme
[...]
> @@ -208,6 +213,18 @@ _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)"

I suggest to remove the line above. It caused ShellCheck warning SC2034. I think
def_host_traddr is not used anywhere.

> +		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}"
[...]
> @@ -811,6 +836,29 @@ _nvmet_target_setup() {
>  		fi
>  	fi
>  
> +	if [[ -n "${hostkey}" ]]; then
> +		ARGS+=(--hostkey "${hostkey}")
> +	fi
> +	if [[ -n "${ctrlkey}" ]]; then
> +		ARGS+=(--ctrkey "${ctrlkey}")
> +	fi

This part above sets arguments --hostkey and --ctrkey in ARGS to pass to
_create_nvmet_subsystem(), but I find that _create_nvmet_subsystem() does not
refer to the arguments. Though I know this part was in v3 also, I suggest drop
this part.

> +
> +	if [[ -n "${nvme_target_control}" ]]; then
> +		eval "${nvme_target_control}" setup \
> +			--subsysnqn "${subsysnqn}" \
> +			--subsys-uuid "${subsys_uuid:-$def_subsys_uuid}" \
> +			--hostnqn "${def_hostnqn}" \
> +			"${ARGS[@]}" &> /dev/null

The line above causes the ShellCheck warning SC 2294. Let's replace ${ARGS[@]}
with ${ARGS[*]}.

> +		return
> +	fi
> +
> +	truncate -s "${NVME_IMG_SIZE}" "$(_nvme_def_file_path)"
> +	if [[ "${blkdev_type}" == "device" ]]; then
> +		blkdev="$(losetup -f --show "$(_nvme_def_file_path)")"
> +	else
> +		blkdev="$(_nvme_def_file_path)"
> +	fi

This truncate and blkdev setup part causes failure of nvme/052:

nvme/052 (tr=loop) (Test file-ns creation/deletion under one subsystem) [failed]
    runtime  5.728s  ...  5.267s
    --- tests/nvme/052.out      2024-11-05 17:04:40.596903603 +0900
    +++ /home/shin/Blktests/blktests/results/nodev_tr_loop/nvme/052.out.bad     2024-11-29 14:45:23.065861316 +0900
    @@ -1,2 +1,4 @@
     Running nvme/052
    +mkdir: cannot create directory ‘/sys/kernel/config/nvmet//subsystems/blktests-subsystem-1/namespaces/1’: File exists
    +common/nvme: line 554: printf: write error: Device or resource busy
     Test complete

Also, this part looks duplicated with the other part in _nvmet_target_setup().
Please see the 'if [[ "${blkdev_type}" != "none" ]]' block.

I guess this is the part you added "to specify the backing block device on the
target, instead of hardcoding '/dev/vdc'". If so, I think such changes should
be done under 'if [[ -n "${nvme_target_control}" ]]' condition.


[-- Attachment #2: 0001-nvme-041-045-051-double-quote-def_-variable-referenc.patch --]
[-- Type: text/plain, Size: 4856 bytes --]

From b86f26b4772ef5a458685fbf7dca14a50551a815 Mon Sep 17 00:00:00 2001
From: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Date: Fri, 29 Nov 2024 10:48:31 +0900
Subject: [PATCH] nvme/{041-045,051}: double quote def_* variable references

The following commit will add the code to set nvme command output to
def_* variables. This will trigger the ShellCheck warning SC2086. To
prepare for the change, double quote the references to the def_*
variables. As for nvme/051, the local variable ns is initialized with
def_subsysnqn, then it requires double quote also.

Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 tests/nvme/041 | 2 +-
 tests/nvme/042 | 4 ++--
 tests/nvme/043 | 2 +-
 tests/nvme/044 | 4 ++--
 tests/nvme/045 | 8 ++++----
 tests/nvme/051 | 4 ++--
 6 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/tests/nvme/041 b/tests/nvme/041
index aa44f04..94f84f1 100755
--- a/tests/nvme/041
+++ b/tests/nvme/041
@@ -31,7 +31,7 @@ test() {
 	local hostkey
 	local ctrldev
 
-	hostkey="$(nvme gen-dhchap-key -n ${def_subsysnqn} 2> /dev/null)"
+	hostkey="$(nvme gen-dhchap-key -n "${def_subsysnqn}" 2> /dev/null)"
 	if [ -z "$hostkey" ] ; then
 		echo "nvme gen-dhchap-key failed"
 		return 1
diff --git a/tests/nvme/042 b/tests/nvme/042
index 70c9056..17d8a73 100755
--- a/tests/nvme/042
+++ b/tests/nvme/042
@@ -37,7 +37,7 @@ test() {
 
 	for hmac in 0 1 2 3; do
 		echo "Testing hmac ${hmac}"
-		hostkey="$(nvme gen-dhchap-key --hmac=${hmac} -n ${def_subsysnqn} 2> /dev/null)"
+		hostkey="$(nvme gen-dhchap-key --hmac=${hmac} -n "${def_subsysnqn}" 2> /dev/null)"
 		if [ -z "$hostkey" ] ; then
 			echo "couldn't generate host key for hmac ${hmac}"
 			return 1
@@ -51,7 +51,7 @@ test() {
 
 	for key_len in 32 48 64; do
 		echo "Testing key length ${key_len}"
-		hostkey="$(nvme gen-dhchap-key --key-length=${key_len} -n ${def_subsysnqn} 2> /dev/null)"
+		hostkey="$(nvme gen-dhchap-key --key-length=${key_len} -n "${def_subsysnqn}" 2> /dev/null)"
 		if [ -z "$hostkey" ] ; then
 			echo "couldn't generate host key for length ${key_len}"
 			return 1
diff --git a/tests/nvme/043 b/tests/nvme/043
index cf99865..7f9e67e 100755
--- a/tests/nvme/043
+++ b/tests/nvme/043
@@ -34,7 +34,7 @@ test() {
 	local hostkey
 	local ctrldev
 
-	hostkey="$(nvme gen-dhchap-key -n ${def_hostnqn} 2> /dev/null)"
+	hostkey="$(nvme gen-dhchap-key -n "${def_hostnqn}" 2> /dev/null)"
 	if [ -z "$hostkey" ] ; then
 		echo "nvme gen-dhchap-key failed"
 		return 1
diff --git a/tests/nvme/044 b/tests/nvme/044
index 9ed46c9..7c08328 100755
--- a/tests/nvme/044
+++ b/tests/nvme/044
@@ -33,13 +33,13 @@ test() {
 	local ctrlkey
 	local ctrldev
 
-	hostkey="$(nvme gen-dhchap-key -n ${def_subsysnqn} 2> /dev/null)"
+	hostkey="$(nvme gen-dhchap-key -n "${def_subsysnqn}" 2> /dev/null)"
 	if [ -z "$hostkey" ] ; then
 		echo "failed to generate host key"
 		return 1
 	fi
 
-	ctrlkey="$(nvme gen-dhchap-key -n ${def_subsysnqn} 2> /dev/null)"
+	ctrlkey="$(nvme gen-dhchap-key -n "${def_subsysnqn}" 2> /dev/null)"
 	if [ -z "$ctrlkey" ] ; then
 		echo "failed to generate ctrl key"
 		return 1
diff --git a/tests/nvme/045 b/tests/nvme/045
index be81316..4dd0f94 100755
--- a/tests/nvme/045
+++ b/tests/nvme/045
@@ -38,13 +38,13 @@ test() {
 	local rand_io_size
 	local ns
 
-	hostkey="$(nvme gen-dhchap-key -n ${def_subsysnqn} 2> /dev/null)"
+	hostkey="$(nvme gen-dhchap-key -n "${def_subsysnqn}" 2> /dev/null)"
 	if [ -z "$hostkey" ] ; then
 		echo "failed to generate host key"
 		return 1
 	fi
 
-	ctrlkey="$(nvme gen-dhchap-key -n ${def_subsysnqn} 2> /dev/null)"
+	ctrlkey="$(nvme gen-dhchap-key -n "${def_subsysnqn}" 2> /dev/null)"
 	if [ -z "$ctrlkey" ] ; then
 		echo "failed to generate ctrl key"
 		return 1
@@ -69,7 +69,7 @@ test() {
 
 	echo "Renew host key on the controller"
 
-	new_hostkey="$(nvme gen-dhchap-key --nqn ${def_subsysnqn} 2> /dev/null)"
+	new_hostkey="$(nvme gen-dhchap-key --nqn "${def_subsysnqn}" 2> /dev/null)"
 
 	_set_nvmet_hostkey "${def_hostnqn}" "${new_hostkey}"
 
@@ -79,7 +79,7 @@ test() {
 
 	echo "Renew ctrl key on the controller"
 
-	new_ctrlkey="$(nvme gen-dhchap-key --nqn ${def_subsysnqn} 2> /dev/null)"
+	new_ctrlkey="$(nvme gen-dhchap-key --nqn "${def_subsysnqn}" 2> /dev/null)"
 
 	_set_nvmet_ctrlkey "${def_hostnqn}" "${new_ctrlkey}"
 
diff --git a/tests/nvme/051 b/tests/nvme/051
index 4757b80..323fac7 100755
--- a/tests/nvme/051
+++ b/tests/nvme/051
@@ -37,8 +37,8 @@ test() {
 
 	# fire off two enable/disable loops concurrently and wait
 	# for them to complete...
-	ns_enable_disable_loop $ns &
-	ns_enable_disable_loop $ns &
+	ns_enable_disable_loop "$ns" &
+	ns_enable_disable_loop "$ns" &
 	wait
 
 	_nvmet_target_cleanup
-- 
2.46.2

Re: [PATCH blktests v4 5/5] nvme/055: add test for nvme-tcp zero-copy offload

[Shinichiro Kawasaki]

On Nov 26, 2024 / 22:38, Aurelien Aptel wrote:
> This commit adds a new test for the kernel ULP DDP (Direct Data
> Placement) feature with NVMe-TCP.
> 
> Configuration of DDP is per NIC and is done through a script in the
> kernel source. For this reason we add 2 new config vars:
> - KERNELSRC: path to the running kernel sources
> - NVME_IFACE: name of the network interface to configure the offload on
> 
> Signed-off-by: Aurelien Aptel <aaptel@nvidia.com>
> Signed-off-by: Shai Malin smalin@nvidia.com
> Reviewed-by: Daniel Wagner <dwagner@suse.de>

This test is interesting!

[...]

> diff --git a/tests/nvme/055 b/tests/nvme/055
> new file mode 100755
> index 0000000..7e76126
> --- /dev/null
> +++ b/tests/nvme/055
> @@ -0,0 +1,285 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright (C) 2024 Aurelien Aptel <aaptel@nvidia.com>
> +#
> +# zero-copy offload

My understanding is that this test case requires the target set up by
NVME_TARGET_CONTROL. Is it beneficial to explain what kind of target set
up is required here?

Does this test case require specific hardware for nvme-tcp and zero-copy?
If so, it can be described here also, probably.

> +
> +. tests/nvme/rc
> +
> +DESCRIPTION="enable zero copy offload and run rw traffic"
> +TIMED=1
> +
> +iface_idx=""
> +
> +# these vars get updated after each call to connect_run_disconnect()
> +nb_packets=0
> +nb_bytes=0
> +nb_offload_packets=0
> +nb_offload_bytes=0
> +offload_bytes_ratio=0
> +offload_packets_ratio=0
> +
> +requires() {
> +	_nvme_requires
> +	_require_remote_nvme_target
> +	_require_nvme_trtype tcp
> +	_have_kernel_option ULP_DDP
> +	# require nvme-tcp as a module to be able to change the ddp_offload param
> +	_have_module nvme_tcp && _have_module_param nvme_tcp ddp_offload

I checked the latest kernel source code but could not find the ddp_offload
parameter. Do I miss anything? or Do you plan to post kernel patches for it?

> +	_have_fio
> +	_have_program ip
> +	_have_program ethtool
> +	_have_kernel_source && have_netlink_cli && _have_program python3
> +	have_iface
> +}
> +
[...]
> +
> +connect_run_disconnect() {
> +	local io_size
> +	local nvme_dev
> +	local nb_drop
> +	local drop_ratio
> +	local nb_resync
> +	local resync_ratio

Nit: some local variables misses declarations here: nb_packets,
nb_offload_packets, etc. It might be good to declare multiple variables
in one line, like "local nb_drop nb_resync nb_packets ..." to reduce number
of lines.

Re: [PATCH blktests v4 5/5] nvme/055: add test for nvme-tcp zero-copy offload

[Aurelien Aptel]

Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com> writes:
> My understanding is that this test case requires the target set up by
> NVME_TARGET_CONTROL. Is it beneficial to explain what kind of target set
> up is required here?

Any target should do, but the host needs to use a NIC that supports DDP
offload. This means the loop/localhost target cannot be used.

> Does this test case require specific hardware for nvme-tcp and zero-copy?
> If so, it can be described here also, probably.

It requires hardware that supports the ULP DDP infrastructure
(specifically nvme-tcp). This includes ConnectX 7 NIC and above or
BlueField 3 DPU and above.

>> +requires() {
>> +     _nvme_requires
>> +     _require_remote_nvme_target
>> +     _require_nvme_trtype tcp
>> +     _have_kernel_option ULP_DDP
>> +     # require nvme-tcp as a module to be able to change the ddp_offload param
>> +     _have_module nvme_tcp && _have_module_param nvme_tcp ddp_offload
>
> I checked the latest kernel source code but could not find the ddp_offload
> parameter. Do I miss anything? or Do you plan to post kernel patches for it?

The DDP offload is part of the nvme-tcp offload series [1], and the only
missing part is the netdev maintainer (Jakub) request of including the
tests for the feature. We agreed to have those tests as part of blktests,
which we will then run in a CI.

>> +     _have_fio
>> +     _have_program ip
>> +     _have_program ethtool
>> +     _have_kernel_source && have_netlink_cli && _have_program python3
>> +     have_iface
>> +}
>> +
> [...]
>> +
>> +connect_run_disconnect() {
>> +     local io_size
>> +     local nvme_dev
>> +     local nb_drop
>> +     local drop_ratio
>> +     local nb_resync
>> +     local resync_ratio
>
> Nit: some local variables misses declarations here: nb_packets,
> nb_offload_packets, etc. It might be good to declare multiple variables
> in one line, like "local nb_drop nb_resync nb_packets ..." to reduce number
> of lines.

These are not declared as local because they are global, see the top of
the file.

I will declare the local ones on line when it makes it more readable.

Thanks.

1: https://lore.kernel.org/netdev/20240529160053.111531-1-aaptel@nvidia.com/

Re: [PATCH blktests v4 1/5] nvme/rc: introduce remote target support

[Aurelien Aptel]

Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com> writes:
>> @@ -208,6 +213,18 @@ _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)"
>
> I suggest to remove the line above. It caused ShellCheck warning SC2034. I think
> def_host_traddr is not used anywhere.

Ok.

>> @@ -811,6 +836,29 @@ _nvmet_target_setup() {
>>               fi
>>       fi
>>
>> +     if [[ -n "${hostkey}" ]]; then
>> +             ARGS+=(--hostkey "${hostkey}")
>> +     fi
>> +     if [[ -n "${ctrlkey}" ]]; then
>> +             ARGS+=(--ctrkey "${ctrlkey}")
>> +     fi
>
> This part above sets arguments --hostkey and --ctrkey in ARGS to pass to
> _create_nvmet_subsystem(), but I find that _create_nvmet_subsystem() does not
> refer to the arguments. Though I know this part was in v3 also, I suggest drop
> this part.

Good point.

>> +
>> +     if [[ -n "${nvme_target_control}" ]]; then
>> +             eval "${nvme_target_control}" setup \
>> +                     --subsysnqn "${subsysnqn}" \
>> +                     --subsys-uuid "${subsys_uuid:-$def_subsys_uuid}" \
>> +                     --hostnqn "${def_hostnqn}" \
>> +                     "${ARGS[@]}" &> /dev/null
>
> The line above causes the ShellCheck warning SC 2294. Let's replace ${ARGS[@]}
> with ${ARGS[*]}.

I think using quoting [*] will merge the args as one and is not what we
want. I will remove the eval instead. It came from v3, not sure why it is
needed.

>> +             return
>> +     fi
>> +
>> +     truncate -s "${NVME_IMG_SIZE}" "$(_nvme_def_file_path)"
>> +     if [[ "${blkdev_type}" == "device" ]]; then
>> +             blkdev="$(losetup -f --show "$(_nvme_def_file_path)")"
>> +     else
>> +             blkdev="$(_nvme_def_file_path)"
>> +     fi
>
> This truncate and blkdev setup part causes failure of nvme/052:
> [...]
> Also, this part looks duplicated with the other part in _nvmet_target_setup().
> Please see the 'if [[ "${blkdev_type}" != "none" ]]' block.

You're correct, this was a rebasing mistake. I will remove it.

> I guess this is the part you added "to specify the backing block device on the
> target, instead of hardcoding '/dev/vdc'". If so, I think such changes should
> be done under 'if [[ -n "${nvme_target_control}" ]]' condition.

No that part is done in the contrib/ script and the template.

Thanks