[PATCH blktests v2 0/3] Test different queue counts

[PATCH blktests v2 0/3] Test different queue counts

[Daniel Wagner]

Setup different queues, e.g. read and poll queues.

There is still the problem that _require_nvme_trtype_is_fabrics also includes
the loop transport which has no support for different queue types.

See also https://lore.kernel.org/linux-nvme/20230322002350.4038048-1-kbusch@meta.com/

changes:
 v2:
   - make most arguements optional for _nvme_connect_subsys
   - move variable decleration at the beging of _nvme_connect_subsy
 v1:
   - initial version

Daniel Wagner (3):
  nvme/rc: Parse optional arguments in _nvme_connect_subsys()
  nvme/rc: Add nr queue parser arguments
  nvme/047: Test different queue counts

 tests/nvme/041     |  7 +++-
 tests/nvme/042     | 10 +++--
 tests/nvme/043     | 10 +++--
 tests/nvme/044     | 23 +++++++----
 tests/nvme/045     |  6 ++-
 tests/nvme/047     | 65 +++++++++++++++++++++++++++++++
 tests/nvme/047.out |  2 +
 tests/nvme/rc      | 96 +++++++++++++++++++++++++++++++++++++++++-----
 8 files changed, 190 insertions(+), 29 deletions(-)
 create mode 100755 tests/nvme/047
 create mode 100644 tests/nvme/047.out

-- 
2.40.0

[PATCH blktests v2 1/3] nvme/rc: Parse optional arguments in _nvme_connect_subsys()

[Daniel Wagner]

Extend the nvme_connect_subsys() function to parse optional arguments.
This avoids that all test have to pass in always all arguments.

Signed-off-by: Daniel Wagner <dwagner@suse.de>
---
 tests/nvme/041 |  7 ++++--
 tests/nvme/042 | 10 +++++---
 tests/nvme/043 | 10 +++++---
 tests/nvme/044 | 23 +++++++++++------
 tests/nvme/045 |  6 +++--
 tests/nvme/rc  | 68 +++++++++++++++++++++++++++++++++++++++++++-------
 6 files changed, 95 insertions(+), 29 deletions(-)

diff --git a/tests/nvme/041 b/tests/nvme/041
index 8ffcf13a500a..03e2dab25918 100755
--- a/tests/nvme/041
+++ b/tests/nvme/041
@@ -55,14 +55,17 @@ test() {
 	# Test unauthenticated connection (should fail)
 	echo "Test unauthenticated connection (should fail)"
 	_nvme_connect_subsys "${nvme_trtype}" "${subsys_name}" \
-			     "" "" "${hostnqn}" "${hostid}"
+			     --hostnqn "${hostnqn}" \
+			     --hostid "${hostid}"
 
 	_nvme_disconnect_subsys "${subsys_name}"
 
 	# Test authenticated connection
 	echo "Test authenticated connection"
 	_nvme_connect_subsys "${nvme_trtype}" "${subsys_name}" \
-			     "" "" "${hostnqn}" "${hostid}" "${hostkey}"
+			     --hostnqn "${hostnqn}" \
+			     --hostid "${hostid}" \
+			     --dhchap-secret "${hostkey}"
 
 	udevadm settle
 
diff --git a/tests/nvme/042 b/tests/nvme/042
index d581bce4a9ee..4ad726f72f5a 100755
--- a/tests/nvme/042
+++ b/tests/nvme/042
@@ -58,8 +58,9 @@ test() {
 		_set_nvmet_hostkey "${hostnqn}" "${hostkey}"
 
 		_nvme_connect_subsys "${nvme_trtype}" "${subsys_name}" \
-				     "" "" "${hostnqn}" "${hostid}" \
-				     "${hostkey}"
+				     --hostnqn "${hostnqn}" \
+				     --hostid "${hostid}" \
+				     --dhchap-secret "${hostkey}"
 		udevadm settle
 
 		_nvme_disconnect_subsys "${subsys_name}"
@@ -75,8 +76,9 @@ test() {
 		_set_nvmet_hostkey "${hostnqn}" "${hostkey}"
 
 		_nvme_connect_subsys "${nvme_trtype}" "${subsys_name}" \
-				     "" "" "${hostnqn}" "${hostid}" \
-				     "${hostkey}"
+				     --hostnqn "${hostnqn}" \
+				     --hostid "${hostid}" \
+				     --dhchap-secret "${hostkey}"
 
 		udevadm settle
 
diff --git a/tests/nvme/043 b/tests/nvme/043
index c8ce292ba2e7..c031cecf34a5 100755
--- a/tests/nvme/043
+++ b/tests/nvme/043
@@ -56,8 +56,9 @@ test() {
 		_set_nvmet_hash "${hostnqn}" "${hash}"
 
 		_nvme_connect_subsys "${nvme_trtype}" "${subsys_name}" \
-				     "" "" "${hostnqn}" "${hostid}" \
-				     "${hostkey}"
+				     --hostnqn "${hostnqn}" \
+				     --hostid "${hostid}" \
+				     --dhchap-secret "${hostkey}"
 
 		udevadm settle
 
@@ -71,8 +72,9 @@ test() {
 		_set_nvmet_dhgroup "${hostnqn}" "${dhgroup}"
 
 		_nvme_connect_subsys "${nvme_trtype}" "${subsys_name}" \
-				     "" "" "${hostnqn}" "${hostid}" \
-				     "${hostkey}"
+				      --hostnqn "${hostnqn}" \
+				      --hostid "${hostid}" \
+				      --dhchap-secret "${hostkey}"
 
 		udevadm settle
 
diff --git a/tests/nvme/044 b/tests/nvme/044
index c19a9c026711..f2406ecadf7d 100755
--- a/tests/nvme/044
+++ b/tests/nvme/044
@@ -66,8 +66,9 @@ test() {
 	# Step 1: Connect with host authentication only
 	echo "Test host authentication"
 	_nvme_connect_subsys "${nvme_trtype}" "${subsys_name}" \
-			     "" "" "${hostnqn}" "${hostid}" \
-			     "${hostkey}"
+			     --hostnqn "${hostnqn}" \
+			     --hostid "${hostid}" \
+			     --dhchap-secret "${hostkey}"
 
 	udevadm settle
 
@@ -77,8 +78,10 @@ test() {
 	# and invalid ctrl authentication
 	echo "Test invalid ctrl authentication (should fail)"
 	_nvme_connect_subsys "${nvme_trtype}" "${subsys_name}" \
-			     "" "" "${hostnqn}" "${hostid}" \
-			     "${hostkey}" "${hostkey}"
+			     --hostnqn "${hostnqn}" \
+			     --hostid "${hostid}" \
+			     --dhchap-secret "${hostkey}" \
+			     --dhchap-ctrl-secret "${hostkey}"
 
 	udevadm settle
 
@@ -88,8 +91,10 @@ test() {
 	# and valid ctrl authentication
 	echo "Test valid ctrl authentication"
 	_nvme_connect_subsys "${nvme_trtype}" "${subsys_name}" \
-			     "" "" "${hostnqn}" "${hostid}" \
-			     "${hostkey}" "${ctrlkey}"
+			     --hostnqn "${hostnqn}" \
+			     --hostid "${hostid}" \
+			     --dhchap-secret "${hostkey}" \
+			     --dhchap-ctrl-secret "${ctrlkey}"
 
 	udevadm settle
 
@@ -100,8 +105,10 @@ test() {
 	echo "Test invalid ctrl key (should fail)"
 	invkey="DHHC-1:00:Jc/My1o0qtLCWRp+sHhAVafdfaS7YQOMYhk9zSmlatobqB8C:"
 	_nvme_connect_subsys "${nvme_trtype}" "${subsys_name}" \
-			     "" "" "${hostnqn}" "${hostid}" \
-			     "${hostkey}" "${invkey}"
+			     --hostnqn "${hostnqn}" \
+			     --hostid "${hostid}" \
+			     --dhchap-secret "${hostkey}" \
+			     --dhchap-ctrl-secret "${invkey}"
 
 	udevadm settle
 
diff --git a/tests/nvme/045 b/tests/nvme/045
index a0e6e93ed3c7..612e5f168e3c 100755
--- a/tests/nvme/045
+++ b/tests/nvme/045
@@ -65,8 +65,10 @@ test() {
 	_set_nvmet_dhgroup "${hostnqn}" "ffdhe2048"
 
 	_nvme_connect_subsys "${nvme_trtype}" "${subsys_name}" \
-			     "" "" "${hostnqn}" "${hostid}" \
-			     "${hostkey}" "${ctrlkey}"
+			     --hostnqn "${hostnqn}" \
+			     --hostid "${hostid}" \
+			     --dhchap-secret "${hostkey}" \
+			     --dhchap-ctrl-secret "${ctrlkey}"
 
 	udevadm settle
 
diff --git a/tests/nvme/rc b/tests/nvme/rc
index 210a82aea384..1145fed2d14c 100644
--- a/tests/nvme/rc
+++ b/tests/nvme/rc
@@ -316,15 +316,65 @@ _nvme_disconnect_subsys() {
 }
 
 _nvme_connect_subsys() {
-	local trtype="$1"
-	local subsysnqn="$2"
-	local traddr="${3:-$def_traddr}"
-	local host_traddr="${4:-$def_host_traddr}"
-	local trsvcid="${4:-$def_trsvcid}"
-	local hostnqn="${5:-$def_hostnqn}"
-	local hostid="${6:-$def_hostid}"
-	local hostkey="${7}"
-	local ctrlkey="${8}"
+	local positional_args=()
+	local trtype=""
+	local subsysnqn=""
+	local traddr="$def_traddr"
+	local host_traddr="$def_host_traddr"
+	local trsvcid="$def_trsvcid"
+	local hostnqn="$def_hostnqn"
+	local hostid="$def_hostid"
+	local hostkey=""
+	local ctrlkey=""
+
+	while [[ $# -gt 0 ]]; do
+		case $1 in
+			-a|--traddr)
+				traddr="$2"
+				shift
+				shift
+				;;
+			-w|--host-traddr)
+				host_traddr="$2"
+				shift
+				shift
+				;;
+			-s|--trsvcid)
+				trsvcid="$2"
+				shift
+				shift
+				;;
+			-n|--hostnqn)
+				hostnqn="$2"
+				shift
+				shift
+				;;
+			-I|--hostid)
+				hostid="$2"
+				shift
+				shift
+				;;
+			-S|--dhchap-secret)
+				hostkey="$2"
+				shift
+				shift
+				;;
+			-C|--dhchap-ctrl-sectret)
+				ctrlkey="$2"
+				shift
+				shift
+				;;
+			*)
+				positional_args+=("$1")
+				shift
+				;;
+		esac
+	done
+
+	set -- "${positional_args[@]}"
+
+	trtype="$1"
+	subsysnqn="$2"
 
 	ARGS=(-t "${trtype}" -n "${subsysnqn}")
 	if [[ "${trtype}" == "fc" ]] ; then
-- 
2.40.0

[PATCH blktests v2 2/3] nvme/rc: Add nr queue parser arguments

[Daniel Wagner]

Signed-off-by: Daniel Wagner <dwagner@suse.de>
---
 tests/nvme/rc | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/tests/nvme/rc b/tests/nvme/rc
index 1145fed2d14c..ca3d995f7b0b 100644
--- a/tests/nvme/rc
+++ b/tests/nvme/rc
@@ -326,6 +326,9 @@ _nvme_connect_subsys() {
 	local hostid="$def_hostid"
 	local hostkey=""
 	local ctrlkey=""
+	local nr_io_queues=""
+	local nr_write_queues=""
+	local nr_poll_queues=""
 
 	while [[ $# -gt 0 ]]; do
 		case $1 in
@@ -364,6 +367,21 @@ _nvme_connect_subsys() {
 				shift
 				shift
 				;;
+			-i|--nr-io-queues)
+				nr_io_queues="$2"
+				shift
+				shift
+				;;
+			-W|--nr-write-queues)
+				nr_write_queues="$2"
+				shift
+				shift
+				;;
+			-P|--nr-poll-queues)
+				nr_poll_queues="$2"
+				shift
+				shift
+				;;
 			*)
 				positional_args+=("$1")
 				shift
@@ -394,6 +412,16 @@ _nvme_connect_subsys() {
 	if [[ -n "${ctrlkey}" ]]; then
 		ARGS+=(--dhchap-ctrl-secret="${ctrlkey}")
 	fi
+	if [[ -n "${nr_io_queues}" ]]; then
+		ARGS+=(--nr-io-queues="${nr_io_queues}")
+	fi
+	if [[ -n "${nr_write_queues}" ]]; then
+		ARGS+=(--nr-write-queues="${nr_write_queues}")
+	fi
+	if [[ -n "${nr_poll_queues}" ]]; then
+		ARGS+=(--nr-poll-queues="${nr_poll_queues}")
+	fi
+
 	nvme connect "${ARGS[@]}" 2> /dev/null
 }
 
-- 
2.40.0

[PATCH blktests v2 3/3] nvme/047: Test different queue counts

[Daniel Wagner]

Test if the transport are handling the different queues correctly.

We also issue some I/O to make sure that not just the plain connect
works. For this we have to use a file system which supports direct I/O
and hence we use a device backend.

Signed-off-by: Daniel Wagner <dwagner@suse.de>
---
 tests/nvme/047     | 65 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/nvme/047.out |  2 ++
 2 files changed, 67 insertions(+)
 create mode 100755 tests/nvme/047
 create mode 100644 tests/nvme/047.out

diff --git a/tests/nvme/047 b/tests/nvme/047
new file mode 100755
index 000000000000..6a37f7a569b7
--- /dev/null
+++ b/tests/nvme/047
@@ -0,0 +1,65 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0+
+# Copyright (C) 2023 SUSE LLC
+#
+# Test NVMe queue counts.
+
+. tests/nvme/rc
+. common/xfs
+
+DESCRIPTION="test NVMe queue counts"
+
+requires() {
+	_nvme_requires
+	_have_xfs
+	_have_fio
+	_require_nvme_trtype_is_fabrics
+}
+
+test() {
+	echo "Running ${TEST_NAME}"
+
+	_setup_nvmet
+
+	local port
+	local nvmedev
+	local loop_dev
+	local file_path="$TMPDIR/img"
+	local subsys_name="blktests-subsystem-1"
+
+	truncate -s 512M "${file_path}"
+
+	loop_dev="$(losetup -f --show "${file_path}")"
+
+	_create_nvmet_subsystem "${subsys_name}" "${loop_dev}" \
+		"91fdba0d-f87b-4c25-b80f-db7be1418b9e"
+	port="$(_create_nvmet_port "${nvme_trtype}")"
+	_add_nvmet_subsys_to_port "${port}" "${subsys_name}"
+
+	_nvme_connect_subsys "${nvme_trtype}" "${subsys_name}" \
+		--nr-write-queues 1
+
+	nvmedev=$(_find_nvme_dev "${subsys_name}")
+
+	_xfs_run_fio_verify_io /dev/"${nvmedev}n1" "1m"
+
+	_nvme_disconnect_subsys "${subsys_name}" >> "$FULL" 2>&1
+
+	_nvme_connect_subsys "${nvme_trtype}" "${subsys_name}" \
+		--nr-write-queues 1 \
+		--nr-poll-queues 1
+
+	_xfs_run_fio_verify_io /dev/"${nvmedev}n1" "1m"
+
+	_nvme_disconnect_subsys "${subsys_name}" >> "$FULL" 2>&1
+
+	_remove_nvmet_subsystem_from_port "${port}" "${subsys_name}"
+	_remove_nvmet_subsystem "${subsys_name}"
+	_remove_nvmet_port "${port}"
+
+	losetup -d "${loop_dev}"
+
+	rm "${file_path}"
+
+	echo "Test complete"
+}
diff --git a/tests/nvme/047.out b/tests/nvme/047.out
new file mode 100644
index 000000000000..915d0a2389ca
--- /dev/null
+++ b/tests/nvme/047.out
@@ -0,0 +1,2 @@
+Running nvme/047
+Test complete
-- 
2.40.0

Re: [PATCH blktests v2 1/3] nvme/rc: Parse optional arguments in _nvme_connect_subsys()

[Daniel Wagner]

On Wed, Mar 22, 2023 at 11:16:46AM +0100, Daniel Wagner wrote:
> +			-S|--dhchap-secret)
> +				hostkey="$2"
> +				shift
> +				shift
> +				;;
> +			-C|--dhchap-ctrl-sectret)

Urgh, typo. I wonder why I didn't caught this in my first round of testing.

Re: [PATCH blktests v2 1/3] nvme/rc: Parse optional arguments in _nvme_connect_subsys()

[Shinichiro Kawasaki]

On Mar 22, 2023 / 11:16, Daniel Wagner wrote:
> Extend the nvme_connect_subsys() function to parse optional arguments.
> This avoids that all test have to pass in always all arguments.
> 
> Signed-off-by: Daniel Wagner <dwagner@suse.de>
> ---
>  tests/nvme/041 |  7 ++++--
>  tests/nvme/042 | 10 +++++---
>  tests/nvme/043 | 10 +++++---
>  tests/nvme/044 | 23 +++++++++++------
>  tests/nvme/045 |  6 +++--
>  tests/nvme/rc  | 68 +++++++++++++++++++++++++++++++++++++++++++-------
>  6 files changed, 95 insertions(+), 29 deletions(-)
> 
> diff --git a/tests/nvme/041 b/tests/nvme/041
> index 8ffcf13a500a..03e2dab25918 100755
> --- a/tests/nvme/041
> +++ b/tests/nvme/041
> @@ -55,14 +55,17 @@ test() {
>  	# Test unauthenticated connection (should fail)
>  	echo "Test unauthenticated connection (should fail)"
>  	_nvme_connect_subsys "${nvme_trtype}" "${subsys_name}" \
> -			     "" "" "${hostnqn}" "${hostid}"
> +			     --hostnqn "${hostnqn}" \
> +			     --hostid "${hostid}"
>  
>  	_nvme_disconnect_subsys "${subsys_name}"
>  
>  	# Test authenticated connection
>  	echo "Test authenticated connection"
>  	_nvme_connect_subsys "${nvme_trtype}" "${subsys_name}" \
> -			     "" "" "${hostnqn}" "${hostid}" "${hostkey}"
> +			     --hostnqn "${hostnqn}" \
> +			     --hostid "${hostid}" \
> +			     --dhchap-secret "${hostkey}"
>  
>  	udevadm settle
>  
> diff --git a/tests/nvme/042 b/tests/nvme/042
> index d581bce4a9ee..4ad726f72f5a 100755
> --- a/tests/nvme/042
> +++ b/tests/nvme/042
> @@ -58,8 +58,9 @@ test() {
>  		_set_nvmet_hostkey "${hostnqn}" "${hostkey}"
>  
>  		_nvme_connect_subsys "${nvme_trtype}" "${subsys_name}" \
> -				     "" "" "${hostnqn}" "${hostid}" \
> -				     "${hostkey}"
> +				     --hostnqn "${hostnqn}" \
> +				     --hostid "${hostid}" \
> +				     --dhchap-secret "${hostkey}"
>  		udevadm settle
>  
>  		_nvme_disconnect_subsys "${subsys_name}"
> @@ -75,8 +76,9 @@ test() {
>  		_set_nvmet_hostkey "${hostnqn}" "${hostkey}"
>  
>  		_nvme_connect_subsys "${nvme_trtype}" "${subsys_name}" \
> -				     "" "" "${hostnqn}" "${hostid}" \
> -				     "${hostkey}"
> +				     --hostnqn "${hostnqn}" \
> +				     --hostid "${hostid}" \
> +				     --dhchap-secret "${hostkey}"
>  
>  		udevadm settle
>  
> diff --git a/tests/nvme/043 b/tests/nvme/043
> index c8ce292ba2e7..c031cecf34a5 100755
> --- a/tests/nvme/043
> +++ b/tests/nvme/043
> @@ -56,8 +56,9 @@ test() {
>  		_set_nvmet_hash "${hostnqn}" "${hash}"
>  
>  		_nvme_connect_subsys "${nvme_trtype}" "${subsys_name}" \
> -				     "" "" "${hostnqn}" "${hostid}" \
> -				     "${hostkey}"
> +				     --hostnqn "${hostnqn}" \
> +				     --hostid "${hostid}" \
> +				     --dhchap-secret "${hostkey}"
>  
>  		udevadm settle
>  
> @@ -71,8 +72,9 @@ test() {
>  		_set_nvmet_dhgroup "${hostnqn}" "${dhgroup}"
>  
>  		_nvme_connect_subsys "${nvme_trtype}" "${subsys_name}" \
> -				     "" "" "${hostnqn}" "${hostid}" \
> -				     "${hostkey}"
> +				      --hostnqn "${hostnqn}" \
> +				      --hostid "${hostid}" \
> +				      --dhchap-secret "${hostkey}"
>  
>  		udevadm settle
>  
> diff --git a/tests/nvme/044 b/tests/nvme/044
> index c19a9c026711..f2406ecadf7d 100755
> --- a/tests/nvme/044
> +++ b/tests/nvme/044
> @@ -66,8 +66,9 @@ test() {
>  	# Step 1: Connect with host authentication only
>  	echo "Test host authentication"
>  	_nvme_connect_subsys "${nvme_trtype}" "${subsys_name}" \
> -			     "" "" "${hostnqn}" "${hostid}" \
> -			     "${hostkey}"
> +			     --hostnqn "${hostnqn}" \
> +			     --hostid "${hostid}" \
> +			     --dhchap-secret "${hostkey}"
>  
>  	udevadm settle
>  
> @@ -77,8 +78,10 @@ test() {
>  	# and invalid ctrl authentication
>  	echo "Test invalid ctrl authentication (should fail)"
>  	_nvme_connect_subsys "${nvme_trtype}" "${subsys_name}" \
> -			     "" "" "${hostnqn}" "${hostid}" \
> -			     "${hostkey}" "${hostkey}"
> +			     --hostnqn "${hostnqn}" \
> +			     --hostid "${hostid}" \
> +			     --dhchap-secret "${hostkey}" \
> +			     --dhchap-ctrl-secret "${hostkey}"
>  
>  	udevadm settle
>  
> @@ -88,8 +91,10 @@ test() {
>  	# and valid ctrl authentication
>  	echo "Test valid ctrl authentication"
>  	_nvme_connect_subsys "${nvme_trtype}" "${subsys_name}" \
> -			     "" "" "${hostnqn}" "${hostid}" \
> -			     "${hostkey}" "${ctrlkey}"
> +			     --hostnqn "${hostnqn}" \
> +			     --hostid "${hostid}" \
> +			     --dhchap-secret "${hostkey}" \
> +			     --dhchap-ctrl-secret "${ctrlkey}"
>  
>  	udevadm settle
>  
> @@ -100,8 +105,10 @@ test() {
>  	echo "Test invalid ctrl key (should fail)"
>  	invkey="DHHC-1:00:Jc/My1o0qtLCWRp+sHhAVafdfaS7YQOMYhk9zSmlatobqB8C:"
>  	_nvme_connect_subsys "${nvme_trtype}" "${subsys_name}" \
> -			     "" "" "${hostnqn}" "${hostid}" \
> -			     "${hostkey}" "${invkey}"
> +			     --hostnqn "${hostnqn}" \
> +			     --hostid "${hostid}" \
> +			     --dhchap-secret "${hostkey}" \
> +			     --dhchap-ctrl-secret "${invkey}"
>  
>  	udevadm settle
>  
> diff --git a/tests/nvme/045 b/tests/nvme/045
> index a0e6e93ed3c7..612e5f168e3c 100755
> --- a/tests/nvme/045
> +++ b/tests/nvme/045
> @@ -65,8 +65,10 @@ test() {
>  	_set_nvmet_dhgroup "${hostnqn}" "ffdhe2048"
>  
>  	_nvme_connect_subsys "${nvme_trtype}" "${subsys_name}" \
> -			     "" "" "${hostnqn}" "${hostid}" \
> -			     "${hostkey}" "${ctrlkey}"
> +			     --hostnqn "${hostnqn}" \
> +			     --hostid "${hostid}" \
> +			     --dhchap-secret "${hostkey}" \
> +			     --dhchap-ctrl-secret "${ctrlkey}"
>  
>  	udevadm settle
>  
> diff --git a/tests/nvme/rc b/tests/nvme/rc
> index 210a82aea384..1145fed2d14c 100644
> --- a/tests/nvme/rc
> +++ b/tests/nvme/rc
> @@ -316,15 +316,65 @@ _nvme_disconnect_subsys() {
>  }
>  
>  _nvme_connect_subsys() {
> -	local trtype="$1"
> -	local subsysnqn="$2"
> -	local traddr="${3:-$def_traddr}"
> -	local host_traddr="${4:-$def_host_traddr}"
> -	local trsvcid="${4:-$def_trsvcid}"
> -	local hostnqn="${5:-$def_hostnqn}"
> -	local hostid="${6:-$def_hostid}"
> -	local hostkey="${7}"
> -	local ctrlkey="${8}"
> +	local positional_args=()
> +	local trtype=""
> +	local subsysnqn=""
> +	local traddr="$def_traddr"
> +	local host_traddr="$def_host_traddr"
> +	local trsvcid="$def_trsvcid"
> +	local hostnqn="$def_hostnqn"
> +	local hostid="$def_hostid"
> +	local hostkey=""
> +	local ctrlkey=""
> +
> +	while [[ $# -gt 0 ]]; do
> +		case $1 in
> +			-a|--traddr)
> +				traddr="$2"
> +				shift
> +				shift

This patch looks good other than the type you found.
One nit: the two lines above can be single line with "shift 2". Same comment
for below and the 2nd patch.

> +				;;
> +			-w|--host-traddr)
> +				host_traddr="$2"
> +				shift
> +				shift
> +				;;
> +			-s|--trsvcid)
> +				trsvcid="$2"
> +				shift
> +				shift
> +				;;
> +			-n|--hostnqn)
> +				hostnqn="$2"
> +				shift
> +				shift
> +				;;
> +			-I|--hostid)
> +				hostid="$2"
> +				shift
> +				shift
> +				;;
> +			-S|--dhchap-secret)
> +				hostkey="$2"
> +				shift
> +				shift
> +				;;
> +			-C|--dhchap-ctrl-sectret)
> +				ctrlkey="$2"
> +				shift
> +				shift
> +				;;
> +			*)
> +				positional_args+=("$1")
> +				shift
> +				;;
> +		esac
> +	done
> +
> +	set -- "${positional_args[@]}"
> +
> +	trtype="$1"
> +	subsysnqn="$2"
>  
>  	ARGS=(-t "${trtype}" -n "${subsysnqn}")
>  	if [[ "${trtype}" == "fc" ]] ; then
> -- 
> 2.40.0
> 

-- 
Shin'ichiro Kawasaki

Re: [PATCH blktests v2 2/3] nvme/rc: Add nr queue parser arguments

[Shinichiro Kawasaki]

I suggest to add "to _nvme_connect_subsys()" to the commit title. Other than
that, this patch looks good to me.

-- 
Shin'ichiro Kawasaki

Re: [PATCH blktests v2 3/3] nvme/047: Test different queue counts

[Shinichiro Kawasaki]

On Mar 22, 2023 / 11:16, Daniel Wagner wrote:
> Test if the transport are handling the different queues correctly.
> 
> We also issue some I/O to make sure that not just the plain connect
> works. For this we have to use a file system which supports direct I/O
> and hence we use a device backend.
> 
> Signed-off-by: Daniel Wagner <dwagner@suse.de>
> ---
>  tests/nvme/047     | 65 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/nvme/047.out |  2 ++
>  2 files changed, 67 insertions(+)
>  create mode 100755 tests/nvme/047
>  create mode 100644 tests/nvme/047.out
> 
> diff --git a/tests/nvme/047 b/tests/nvme/047
> new file mode 100755
> index 000000000000..6a37f7a569b7
> --- /dev/null
> +++ b/tests/nvme/047
> @@ -0,0 +1,65 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0+

FYI, GPL-2.0+ is ok for blktests files, but GPL-3.0+ is the majority.
If you do not have preference, I suggest GPL-3.0+.

> +# Copyright (C) 2023 SUSE LLC
> +#
> +# Test NVMe queue counts.

Can we have some more description in the comment block above? I suggest to
copy and paste the first line of the commit message.

> +
> +. tests/nvme/rc
> +. common/xfs
> +
> +DESCRIPTION="test NVMe queue counts"
> +
> +requires() {
> +	_nvme_requires
> +	_have_xfs
> +	_have_fio
> +	_require_nvme_trtype_is_fabrics
> +}

Do you think this test runs on older kernel versions? If you have any specific
kernel version for this test case, _have_kver can be added in requires().

> +
> +test() {
> +	echo "Running ${TEST_NAME}"
> +
> +	_setup_nvmet
> +
> +	local port
> +	local nvmedev
> +	local loop_dev
> +	local file_path="$TMPDIR/img"
> +	local subsys_name="blktests-subsystem-1"
> +
> +	truncate -s 512M "${file_path}"
> +
> +	loop_dev="$(losetup -f --show "${file_path}")"
> +
> +	_create_nvmet_subsystem "${subsys_name}" "${loop_dev}" \
> +		"91fdba0d-f87b-4c25-b80f-db7be1418b9e"
> +	port="$(_create_nvmet_port "${nvme_trtype}")"
> +	_add_nvmet_subsys_to_port "${port}" "${subsys_name}"
> +
> +	_nvme_connect_subsys "${nvme_trtype}" "${subsys_name}" \
> +		--nr-write-queues 1
> +
> +	nvmedev=$(_find_nvme_dev "${subsys_name}")
> +
> +	_xfs_run_fio_verify_io /dev/"${nvmedev}n1" "1m"
> +
> +	_nvme_disconnect_subsys "${subsys_name}" >> "$FULL" 2>&1
> +
> +	_nvme_connect_subsys "${nvme_trtype}" "${subsys_name}" \
> +		--nr-write-queues 1 \
> +		--nr-poll-queues 1
> +
> +	_xfs_run_fio_verify_io /dev/"${nvmedev}n1" "1m"
> +
> +	_nvme_disconnect_subsys "${subsys_name}" >> "$FULL" 2>&1
> +
> +	_remove_nvmet_subsystem_from_port "${port}" "${subsys_name}"
> +	_remove_nvmet_subsystem "${subsys_name}"
> +	_remove_nvmet_port "${port}"
> +
> +	losetup -d "${loop_dev}"
> +
> +	rm "${file_path}"
> +
> +	echo "Test complete"
> +}
> diff --git a/tests/nvme/047.out b/tests/nvme/047.out
> new file mode 100644
> index 000000000000..915d0a2389ca
> --- /dev/null
> +++ b/tests/nvme/047.out
> @@ -0,0 +1,2 @@
> +Running nvme/047
> +Test complete
> -- 
> 2.40.0
> 

-- 
Shin'ichiro Kawasaki

Re: [PATCH blktests v2 0/3] Test different queue counts

[Shinichiro Kawasaki]

On Mar 22, 2023 / 11:16, Daniel Wagner wrote:
> Setup different queues, e.g. read and poll queues.
> 
> There is still the problem that _require_nvme_trtype_is_fabrics also includes
> the loop transport which has no support for different queue types.
> 
> See also https://lore.kernel.org/linux-nvme/20230322002350.4038048-1-kbusch@meta.com/

Hi Daniel, thanks for the patches. The new test case catches some bugs. Looks
valuable.

I ran the test case using various nvme_trtype on kernel v6.2 and v6.3-rc3, and
observed hangs. I applied the 3rd patch in the link above on top of v6.3-rc3 and
confirmed the hang disappears. I would like to wait for the kernel fix patch
delivered to upstream, before adding this test case to blktests master.

When I ran the test case without setting nvme_trtype, kernel reported messages
below:

[  199.621431][ T1001] nvme_fabrics: invalid parameter 'nr_write_queues=%d'
[  201.271200][ T1030] nvme_fabrics: invalid parameter 'nr_write_queues=%d'
[  201.272155][ T1030] nvme_fabrics: invalid parameter 'nr_poll_queues=%d'

Is it useful to run the test case with default nvme_trtype=loop?

-- 
Shin'ichiro Kawasaki

Re: [PATCH blktests v2 0/3] Test different queue counts

[Daniel Wagner]

On Thu, Mar 23, 2023 at 11:06:53AM +0000, Shinichiro Kawasaki wrote:
> On Mar 22, 2023 / 11:16, Daniel Wagner wrote:
> > Setup different queues, e.g. read and poll queues.
> > 
> > There is still the problem that _require_nvme_trtype_is_fabrics also includes
> > the loop transport which has no support for different queue types.
> > 
> > See also https://lore.kernel.org/linux-nvme/20230322002350.4038048-1-kbusch@meta.com/
> 
> Hi Daniel, thanks for the patches. The new test case catches some bugs. Looks
> valuable.
> 
> I ran the test case using various nvme_trtype on kernel v6.2 and v6.3-rc3, and
> observed hangs. I applied the 3rd patch in the link above on top of v6.3-rc3 and
> confirmed the hang disappears. I would like to wait for the kernel fix patch
> delivered to upstream, before adding this test case to blktests master.

Okay makes sense.

> When I ran the test case without setting nvme_trtype, kernel reported messages
> below:
> 
> [  199.621431][ T1001] nvme_fabrics: invalid parameter 'nr_write_queues=%d'
> [  201.271200][ T1030] nvme_fabrics: invalid parameter 'nr_write_queues=%d'
> [  201.272155][ T1030] nvme_fabrics: invalid parameter 'nr_poll_queues=%d'

BTW, I've added a '|| echo FAIL' to catch those.

> Is it useful to run the test case with default nvme_trtype=loop?

No, we should run this test only for those transport which actually support the
different queue types. Christoph suggest to figure out before running the test
if it is actually supported. So my first idea was to check what options are
supported by reading /dev/nvme-fabrics. But this will return all options we are
parsed by fabrics.c but not the subset which each transport might only support.

So to figure this out we would need to do a full setup just to figure out if it
is supported. I think the currently best approach would just to limit this test
to tcp and rdma. Maybe we could add something like

rc:
_require_nvme_trtype() {
	local trtype
	for trtype in "$@"; do
		if [[ "${nvme_trtype}" == "$trtype" ]]; then
			return 0
		fi
	done
	SKIP_REASONS+=("nvme_trtype=${nvme_trtype} is not supported in this test")
	return 1
}

047:
requires() {
	_nvme_requires
	_have_xfs
	_have_fio
	_require_nvme_trtype tcp rdma
	_have_kver 4 21
}

What do you think?

Thanks,
Daniel

Re: [PATCH blktests v2 0/3] Test different queue counts

[Shinichiro Kawasaki]

On Mar 27, 2023 / 17:41, Daniel Wagner wrote:
> On Thu, Mar 23, 2023 at 11:06:53AM +0000, Shinichiro Kawasaki wrote:
> > On Mar 22, 2023 / 11:16, Daniel Wagner wrote:
> > > Setup different queues, e.g. read and poll queues.
> > > 
> > > There is still the problem that _require_nvme_trtype_is_fabrics also includes
> > > the loop transport which has no support for different queue types.
> > > 
> > > See also https://lore.kernel.org/linux-nvme/20230322002350.4038048-1-kbusch@meta.com/
> > 
> > Hi Daniel, thanks for the patches. The new test case catches some bugs. Looks
> > valuable.
> > 
> > I ran the test case using various nvme_trtype on kernel v6.2 and v6.3-rc3, and
> > observed hangs. I applied the 3rd patch in the link above on top of v6.3-rc3 and
> > confirmed the hang disappears. I would like to wait for the kernel fix patch
> > delivered to upstream, before adding this test case to blktests master.
> 
> Okay makes sense.
> 
> > When I ran the test case without setting nvme_trtype, kernel reported messages
> > below:
> > 
> > [  199.621431][ T1001] nvme_fabrics: invalid parameter 'nr_write_queues=%d'
> > [  201.271200][ T1030] nvme_fabrics: invalid parameter 'nr_write_queues=%d'
> > [  201.272155][ T1030] nvme_fabrics: invalid parameter 'nr_poll_queues=%d'
> 
> BTW, I've added a '|| echo FAIL' to catch those.
> 
> > Is it useful to run the test case with default nvme_trtype=loop?
> 
> No, we should run this test only for those transport which actually support the
> different queue types. Christoph suggest to figure out before running the test
> if it is actually supported. So my first idea was to check what options are
> supported by reading /dev/nvme-fabrics. But this will return all options we are
> parsed by fabrics.c but not the subset which each transport might only support.
> 
> So to figure this out we would need to do a full setup just to figure out if it
> is supported. I think the currently best approach would just to limit this test
> to tcp and rdma. Maybe we could add something like
> 
> rc:
> _require_nvme_trtype() {
> 	local trtype
> 	for trtype in "$@"; do
> 		if [[ "${nvme_trtype}" == "$trtype" ]]; then
> 			return 0
> 		fi
> 	done
> 	SKIP_REASONS+=("nvme_trtype=${nvme_trtype} is not supported in this test")
> 	return 1
> }
> 
> 047:
> requires() {
> 	_nvme_requires
> 	_have_xfs
> 	_have_fio
> 	_require_nvme_trtype tcp rdma
> 	_have_kver 4 21
> }
> 
> What do you think?

Thanks for the clarifications about the requirements. I think your approach will
work. Having said that, we may have another potentially simpler solution:

- Do not check _require_nvme_trtype and _have_kver in requires().
- After setting nr_write_queues in test(), check if dmesg contains the error
  "invalid parameter 'nr_write_queues" using the helper function
  _dmesg_since_test_start().
- If the error is reported, set SKIP_REASONS and return from test().
  Blktests will report the test case as "not run".

This approach assumes that the "invalid parameter" is printed when the test case
should be skipped (loop transport, older kernel version).

As a generic guide, SKIP_REASONS should be set in requires() before test().
However, if the SKIP_REASONS can not be checked before test(), blktests allows
to set it in test(). The test case block/030 is such an exception. I think your
new test case can be another exception. With this, we do not need to repeat the
full setup. And it might be more robust against future changes such as new
transport types.

Thoughts?

-- 
Shin'ichiro Kawasaki

Re: [PATCH blktests v2 0/3] Test different queue counts

[Chaitanya Kulkarni]

On 3/28/23 01:45, Shinichiro Kawasaki wrote:
> On Mar 27, 2023 / 17:41, Daniel Wagner wrote:
>> On Thu, Mar 23, 2023 at 11:06:53AM +0000, Shinichiro Kawasaki wrote:
>>> On Mar 22, 2023 / 11:16, Daniel Wagner wrote:
>>>> Setup different queues, e.g. read and poll queues.
>>>>
>>>> There is still the problem that _require_nvme_trtype_is_fabrics also includes
>>>> the loop transport which has no support for different queue types.
>>>>
>>>> See also https://lore.kernel.org/linux-nvme/20230322002350.4038048-1-kbusch@meta.com/
>>> Hi Daniel, thanks for the patches. The new test case catches some bugs. Looks
>>> valuable.
>>>
>>> I ran the test case using various nvme_trtype on kernel v6.2 and v6.3-rc3, and
>>> observed hangs. I applied the 3rd patch in the link above on top of v6.3-rc3 and
>>> confirmed the hang disappears. I would like to wait for the kernel fix patch
>>> delivered to upstream, before adding this test case to blktests master.
>> Okay makes sense.
>>
>>> When I ran the test case without setting nvme_trtype, kernel reported messages
>>> below:
>>>
>>> [  199.621431][ T1001] nvme_fabrics: invalid parameter 'nr_write_queues=%d'
>>> [  201.271200][ T1030] nvme_fabrics: invalid parameter 'nr_write_queues=%d'
>>> [  201.272155][ T1030] nvme_fabrics: invalid parameter 'nr_poll_queues=%d'
>> BTW, I've added a '|| echo FAIL' to catch those.
>>
>>> Is it useful to run the test case with default nvme_trtype=loop?
>> No, we should run this test only for those transport which actually support the
>> different queue types. Christoph suggest to figure out before running the test
>> if it is actually supported. So my first idea was to check what options are
>> supported by reading /dev/nvme-fabrics. But this will return all options we are
>> parsed by fabrics.c but not the subset which each transport might only support.
>>
>> So to figure this out we would need to do a full setup just to figure out if it
>> is supported. I think the currently best approach would just to limit this test
>> to tcp and rdma. Maybe we could add something like
>>
>> rc:
>> _require_nvme_trtype() {
>> 	local trtype
>> 	for trtype in "$@"; do
>> 		if [[ "${nvme_trtype}" == "$trtype" ]]; then
>> 			return 0
>> 		fi
>> 	done
>> 	SKIP_REASONS+=("nvme_trtype=${nvme_trtype} is not supported in this test")
>> 	return 1
>> }
>>
>> 047:
>> requires() {
>> 	_nvme_requires
>> 	_have_xfs
>> 	_have_fio
>> 	_require_nvme_trtype tcp rdma
>> 	_have_kver 4 21
>> }
>>
>> What do you think?
> Thanks for the clarifications about the requirements. I think your approach will
> work. Having said that, we may have another potentially simpler solution:
>
> - Do not check _require_nvme_trtype and _have_kver in requires().
> - After setting nr_write_queues in test(), check if dmesg contains the error
>    "invalid parameter 'nr_write_queues" using the helper function
>    _dmesg_since_test_start().
> - If the error is reported, set SKIP_REASONS and return from test().
>    Blktests will report the test case as "not run".
>
> This approach assumes that the "invalid parameter" is printed when the test case
> should be skipped (loop transport, older kernel version).


Is it possible to not rely on dmesg unless it is absolutely required ?

> As a generic guide, SKIP_REASONS should be set in requires() before test().
> However, if the SKIP_REASONS can not be checked before test(), blktests allows
> to set it in test(). The test case block/030 is such an exception. I think your
> new test case can be another exception. With this, we do not need to repeat the
> full setup. And it might be more robust against future changes such as new
> transport types.

Ummm should we avoid creating exceptions ? unless it is absolutely 
necessary ?
The problem with exception is it becomes problematic for long term 
maintenance.

I believe currently focusing on tcp/rdma only is sufficient ...

-ck

Re: [PATCH blktests v2 0/3] Test different queue counts

[Keith Busch]

On Wed, Mar 22, 2023 at 11:16:45AM +0100, Daniel Wagner wrote:
> Setup different queues, e.g. read and poll queues.

If you wanted to add a similar test for pci, you do it by echo'ing the desired
options to:

  /sys/modules/nvme/parameters/{poll_queues,write_queues}

Then do an 'nvme reset' on the target nvme pci device.

I'll just note that such a test will currently fail, and fixing that doesn't
look like fun. :)

Re: [PATCH blktests v2 0/3] Test different queue counts

[Shinichiro Kawasaki]

On Mar 28, 2023 / 18:20, Chaitanya Kulkarni wrote:
> On 3/28/23 01:45, Shinichiro Kawasaki wrote:
> > On Mar 27, 2023 / 17:41, Daniel Wagner wrote:
> >> On Thu, Mar 23, 2023 at 11:06:53AM +0000, Shinichiro Kawasaki wrote:
> >>> On Mar 22, 2023 / 11:16, Daniel Wagner wrote:
> >>>> Setup different queues, e.g. read and poll queues.
> >>>>
> >>>> There is still the problem that _require_nvme_trtype_is_fabrics also includes
> >>>> the loop transport which has no support for different queue types.
> >>>>
> >>>> See also https://lore.kernel.org/linux-nvme/20230322002350.4038048-1-kbusch@meta.com/
> >>> Hi Daniel, thanks for the patches. The new test case catches some bugs. Looks
> >>> valuable.
> >>>
> >>> I ran the test case using various nvme_trtype on kernel v6.2 and v6.3-rc3, and
> >>> observed hangs. I applied the 3rd patch in the link above on top of v6.3-rc3 and
> >>> confirmed the hang disappears. I would like to wait for the kernel fix patch
> >>> delivered to upstream, before adding this test case to blktests master.
> >> Okay makes sense.
> >>
> >>> When I ran the test case without setting nvme_trtype, kernel reported messages
> >>> below:
> >>>
> >>> [  199.621431][ T1001] nvme_fabrics: invalid parameter 'nr_write_queues=%d'
> >>> [  201.271200][ T1030] nvme_fabrics: invalid parameter 'nr_write_queues=%d'
> >>> [  201.272155][ T1030] nvme_fabrics: invalid parameter 'nr_poll_queues=%d'
> >> BTW, I've added a '|| echo FAIL' to catch those.
> >>
> >>> Is it useful to run the test case with default nvme_trtype=loop?
> >> No, we should run this test only for those transport which actually support the
> >> different queue types. Christoph suggest to figure out before running the test
> >> if it is actually supported. So my first idea was to check what options are
> >> supported by reading /dev/nvme-fabrics. But this will return all options we are
> >> parsed by fabrics.c but not the subset which each transport might only support.
> >>
> >> So to figure this out we would need to do a full setup just to figure out if it
> >> is supported. I think the currently best approach would just to limit this test
> >> to tcp and rdma. Maybe we could add something like
> >>
> >> rc:
> >> _require_nvme_trtype() {
> >> 	local trtype
> >> 	for trtype in "$@"; do
> >> 		if [[ "${nvme_trtype}" == "$trtype" ]]; then
> >> 			return 0
> >> 		fi
> >> 	done
> >> 	SKIP_REASONS+=("nvme_trtype=${nvme_trtype} is not supported in this test")
> >> 	return 1
> >> }
> >>
> >> 047:
> >> requires() {
> >> 	_nvme_requires
> >> 	_have_xfs
> >> 	_have_fio
> >> 	_require_nvme_trtype tcp rdma
> >> 	_have_kver 4 21
> >> }
> >>
> >> What do you think?
> > Thanks for the clarifications about the requirements. I think your approach will
> > work. Having said that, we may have another potentially simpler solution:
> >
> > - Do not check _require_nvme_trtype and _have_kver in requires().
> > - After setting nr_write_queues in test(), check if dmesg contains the error
> >    "invalid parameter 'nr_write_queues" using the helper function
> >    _dmesg_since_test_start().
> > - If the error is reported, set SKIP_REASONS and return from test().
> >    Blktests will report the test case as "not run".
> >
> > This approach assumes that the "invalid parameter" is printed when the test case
> > should be skipped (loop transport, older kernel version).
> 
> 
> Is it possible to not rely on dmesg unless it is absolutely required ?
> 
> > As a generic guide, SKIP_REASONS should be set in requires() before test().
> > However, if the SKIP_REASONS can not be checked before test(), blktests allows
> > to set it in test(). The test case block/030 is such an exception. I think your
> > new test case can be another exception. With this, we do not need to repeat the
> > full setup. And it might be more robust against future changes such as new
> > transport types.
> 
> Ummm should we avoid creating exceptions ? unless it is absolutely 
> necessary ?
> The problem with exception is it becomes problematic for long term 
> maintenance.
> 
> I believe currently focusing on tcp/rdma only is sufficient ...

I see, then let's go with Daniel's approach. My idea could be tricky too much.

-- 
Shin'ichiro Kawasaki

Re: [PATCH blktests v2 0/3] Test different queue counts

[Chaitanya Kulkarni]

On 3/28/23 11:35, Keith Busch wrote:
> On Wed, Mar 22, 2023 at 11:16:45AM +0100, Daniel Wagner wrote:
>> Setup different queues, e.g. read and poll queues.
> If you wanted to add a similar test for pci, you do it by echo'ing the desired
> options to:
>
>    /sys/modules/nvme/parameters/{poll_queues,write_queues}
>
> Then do an 'nvme reset' on the target nvme pci device.
>
> I'll just note that such a test will currently fail, and fixing that doesn't
> look like fun. :)

then we should definitely add it ;) ha ha.

I was actually wondering about pci based on the discussion on this thread
was mainly focused on tcp and rdam, thanks for the suggestion Keith.

-ck

Re: [PATCH blktests v2 0/3] Test different queue counts

[Daniel Wagner]

On Wed, Mar 29, 2023 at 03:30:17AM +0000, Chaitanya Kulkarni wrote:
> On 3/28/23 11:35, Keith Busch wrote:
> > On Wed, Mar 22, 2023 at 11:16:45AM +0100, Daniel Wagner wrote:
> >> Setup different queues, e.g. read and poll queues.
> > If you wanted to add a similar test for pci, you do it by echo'ing the desired
> > options to:
> >
> >    /sys/modules/nvme/parameters/{poll_queues,write_queues}
> >
> > Then do an 'nvme reset' on the target nvme pci device.
> >
> > I'll just note that such a test will currently fail, and fixing that doesn't
> > look like fun. :)
> 
> then we should definitely add it ;) ha ha.
> 
> I was actually wondering about pci based on the discussion on this thread
> was mainly focused on tcp and rdam, thanks for the suggestion Keith.

The test is fabric centric because we configure the queues via the 'nvme
connect' call, something we can't do for PCI. It look likes it needs another
new test for PCI. Let me get this one sorted out first though.