diff mbox series

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

Message ID 20241126203857.27210-2-aaptel@nvidia.com (mailing list archive)
State New
Headers show
Series Add support to run against arbitrary targets | expand

Commit Message

Aurelien Aptel Nov. 26, 2024, 8:38 p.m. UTC 
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(-)

Comments

Shinichiro Kawasaki Nov. 29, 2024, 10:01 a.m. UTC | #1 
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.
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
diff mbox series

Patch

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