[PATCH blktests v5 0/4] test queue count changes on reconnect

[PATCH blktests v5 0/4] test queue count changes on reconnect

[Daniel Wagner]

The target is allowed to change the number of i/o queues. test if the
host is able to reconnect in this scenario.

I was able to test this successfully with tcp and rdma. fc still fails due to
the race condition in the target code.

Next attempt to get this series upstream :)

This version is based on my previous posted nvme/047 patches [1]

[1] https://lore.kernel.org/linux-nvme/20230329090202.8351-1-dwagner@suse.de/

v5:
 - moved generic parts to nvme/rc
 - renamed test to 048
 - rebased ontop of nvme/047
v4:
 - do not remove ports instead depend on host removing
   controllers, see
   https://lore.kernel.org/linux-nvme/20220927143157.3659-1-dwagner@suse.de/
 - https://lore.kernel.org/linux-nvme/20220927143719.4214-1-dwagner@suse.de/
v3:
 - Added comment why at least 2 CPUs are needed for the test
 - Fixed shell quoting in _set_nvmet_attr_qid_max
 - https://lore.kernel.org/linux-nvme/20220913065758.134668-1-dwagner@suse.de/
v2:
 - detect if attr_qid_max is available
 - https://lore.kernel.org/linux-block/20220831153506.28234-1-dwagner@suse.de/
v1:
 - https://lore.kernel.org/linux-block/20220831120900.13129-1-dwagner@suse.de/


Daniel Wagner (4):
  nvme/rc: Add setter for attr_qid_max
  nvme/rc: Add nvmet attribute feature detection function
  nvme/rc: Add helper to wait for nvme ctrl state
  nvme/048: test queue count changes on reconnect

 tests/nvme/048     | 81 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/nvme/048.out |  3 ++
 tests/nvme/rc      | 58 +++++++++++++++++++++++++++++++++
 3 files changed, 142 insertions(+)
 create mode 100755 tests/nvme/048
 create mode 100644 tests/nvme/048.out

-- 
2.40.0

[PATCH blktests v5 1/4] nvme/rc: Add setter for attr_qid_max

[Daniel Wagner]

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

diff --git a/tests/nvme/rc b/tests/nvme/rc
index a3c9b42d91e6..ec7678dbe09e 100644
--- a/tests/nvme/rc
+++ b/tests/nvme/rc
@@ -617,6 +617,16 @@ _set_nvmet_dhgroup() {
 	     "${cfs_path}/dhchap_dhgroup"
 }
 
+_set_nvmet_attr_qid_max() {
+	local nvmet_subsystem="$1"
+	local qid_max="$2"
+	local cfs_path="${NVMET_CFS}/subsystems/${nvmet_subsystem}"
+
+	if [[ -f "${cfs_path}/attr_qid_max" ]]; then
+		echo $qid_max > "${cfs_path}/attr_qid_max"
+	fi
+}
+
 _find_nvme_dev() {
 	local subsys=$1
 	local subsysnqn
-- 
2.40.0

Re: [PATCH blktests v5 1/4] nvme/rc: Add setter for attr_qid_max

[Chaitanya Kulkarni]

On 4/5/23 08:46, Daniel Wagner wrote:
> Signed-off-by: Daniel Wagner <dwagner@suse.de>
> ---
>   tests/nvme/rc | 10 ++++++++++
>   1 file changed, 10 insertions(+)
>
> diff --git a/tests/nvme/rc b/tests/nvme/rc
> index a3c9b42d91e6..ec7678dbe09e 100644
> --- a/tests/nvme/rc
> +++ b/tests/nvme/rc
> @@ -617,6 +617,16 @@ _set_nvmet_dhgroup() {
>   	     "${cfs_path}/dhchap_dhgroup"
>   }
>   
> +_set_nvmet_attr_qid_max() {
> +	local nvmet_subsystem="$1"
> +	local qid_max="$2"
> +	local cfs_path="${NVMET_CFS}/subsystems/${nvmet_subsystem}"
> +
> +	if [[ -f "${cfs_path}/attr_qid_max" ]]; then
> +		echo $qid_max > "${cfs_path}/attr_qid_max"
> +	fi
> +}
> +
>

this is only used in the testcase patch #4, until we get a second
user move it to patch #4 ?

in case this was already discussed and decision made to keep it
in rc, please ignore this comment.

-ck

Re: [PATCH blktests v5 1/4] nvme/rc: Add setter for attr_qid_max

[Daniel Wagner]

On Wed, Apr 05, 2023 at 06:39:43PM +0000, Chaitanya Kulkarni wrote:
> this is only used in the testcase patch #4, until we get a second
> user move it to patch #4 ?
> 
> in case this was already discussed and decision made to keep it
> in rc, please ignore this comment.

There wasn't any decision on this topic. I was not sure if I should put it in rc
but I saw there are already _set_nvmet_*() functions. Thus I came to the
conclusion it makes maintaining these helper function simpler in future because
they are all in one file. If someone touches all _set_nvmet_*() function this
one is not forgotten.

The same goes for the other rc helpers (patch 1-3).

That said, I really do not insit in putiting in rc.

Re: [PATCH blktests v5 1/4] nvme/rc: Add setter for attr_qid_max

[Chaitanya Kulkarni]

On 4/5/23 23:12, Daniel Wagner wrote:
> On Wed, Apr 05, 2023 at 06:39:43PM +0000, Chaitanya Kulkarni wrote:
>> this is only used in the testcase patch #4, until we get a second
>> user move it to patch #4 ?
>>
>> in case this was already discussed and decision made to keep it
>> in rc, please ignore this comment.
> There wasn't any decision on this topic. I was not sure if I should put it in rc
> but I saw there are already _set_nvmet_*() functions. Thus I came to the
> conclusion it makes maintaining these helper function simpler in future because
> they are all in one file. If someone touches all _set_nvmet_*() function this
> one is not forgotten.
>
> The same goes for the other rc helpers (patch 1-3).
>
> That said, I really do not insit in putiting in rc.

if there are functions in rc which are only testcase specific let's keep
those there, we can always do cleanup later ..

-ck

Re: [PATCH blktests v5 1/4] nvme/rc: Add setter for attr_qid_max

[Daniel Wagner]

On Thu, Apr 06, 2023 at 07:05:45AM +0000, Chaitanya Kulkarni wrote:
 if there are functions in rc which are only testcase specific let's keep
> those there, we can always do cleanup later ..

Sure, works for me too.

[PATCH blktests v5 2/4] nvme/rc: Add nvmet attribute feature detection function

[Daniel Wagner]

In order to detect if the target supports a attribute, we have to setup
a target and check if the attribute file appears.

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

diff --git a/tests/nvme/rc b/tests/nvme/rc
index ec7678dbe09e..e720a35113aa 100644
--- a/tests/nvme/rc
+++ b/tests/nvme/rc
@@ -627,6 +627,31 @@ _set_nvmet_attr_qid_max() {
 	fi
 }
 
+_detect_nvmet_subsys_attr() {
+	local attr="$1"
+	local file_path="${TMPDIR}/img"
+	local subsys_name="blktests-feature-detect"
+	local cfs_path="${NVMET_CFS}/subsystems/${subsys_name}"
+	local port
+
+	truncate -s 1M "${file_path}"
+
+	_create_nvmet_subsystem "${subsys_name}" "${file_path}" \
+		"b92842df-a394-44b1-84a4-92ae7d112332"
+	port="$(_create_nvmet_port "${nvme_trtype}")"
+
+	local val=1
+	[[ -f "${cfs_path}/${attr}" ]] && val=0
+
+	_remove_nvmet_subsystem "${subsys_name}"
+
+	_remove_nvmet_port "${port}"
+
+	rm "${file_path}"
+
+	return "${val}"
+}
+
 _find_nvme_dev() {
 	local subsys=$1
 	local subsysnqn
-- 
2.40.0

[PATCH blktests v5 3/4] nvme/rc: Add helper to wait for nvme ctrl state

[Daniel Wagner]

The function waits until it sees a given state (or timeouts).
Unfortunately, we can't use /sys/class/nvme/nvme%d because this symlink
will be removed if the controller is going through resetting state. Thus
use the real nvme%d directly.

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

diff --git a/tests/nvme/rc b/tests/nvme/rc
index e720a35113aa..ae4a69fe8438 100644
--- a/tests/nvme/rc
+++ b/tests/nvme/rc
@@ -16,6 +16,7 @@ def_local_wwnn="0x10001100aa000002"
 def_local_wwpn="0x20001100aa000002"
 def_hostnqn="$(cat /etc/nvme/hostnqn 2> /dev/null)"
 def_hostid="$(cat /etc/nvme/hostid 2> /dev/null)"
+def_state_timeout=20
 nvme_trtype=${nvme_trtype:-"loop"}
 
 _nvme_requires() {
@@ -198,6 +199,28 @@ _nvme_fcloop_del_tport() {
 	echo "wwnn=${wwnn},wwpn=${wwpn}" > ${loopctl}/del_target_port 2> /dev/null
 }
 
+_nvmf_wait_for_state() {
+	local subsys_name="$1"
+	local state="$2"
+	local timeout="${3:-$def_state_timeout}"
+
+	local nvmedev=$(_find_nvme_dev "${subsys_name}")
+	local state_file="/sys/class/nvme-fabrics/ctl/${nvmedev}/state"
+
+	local start_time=$(date +%s)
+	local end_time
+
+	while ! grep -q "${state}" "${state_file}"; do
+		sleep 1
+		end_time=$(date +%s)
+                if (( end_time - start_time > timeout )); then
+                        echo "expected state \"${state}\" not " \
+			     "reached within ${timeout} seconds"
+                        break
+                fi
+	done
+}
+
 _cleanup_fcloop() {
 	local local_wwnn="${1:-$def_local_wwnn}"
 	local local_wwpn="${2:-$def_local_wwpn}"
-- 
2.40.0

Re: [PATCH blktests v5 3/4] nvme/rc: Add helper to wait for nvme ctrl state

[Chaitanya Kulkarni]

On 4/5/23 08:46, Daniel Wagner wrote:
> The function waits until it sees a given state (or timeouts).
> Unfortunately, we can't use /sys/class/nvme/nvme%d because this symlink
> will be removed if the controller is going through resetting state. Thus
> use the real nvme%d directly.
>
> Signed-off-by: Daniel Wagner <dwagner@suse.de>
> ---
>

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

[PATCH blktests v5 4/4] nvme/048: test queue count changes on reconnect

[Daniel Wagner]

The target is allowed to change the number of I/O queues. Test if the
host is able to reconnect in this scenario.

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

diff --git a/tests/nvme/048 b/tests/nvme/048
new file mode 100755
index 000000000000..bc4766e82159
--- /dev/null
+++ b/tests/nvme/048
@@ -0,0 +1,81 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2023 Daniel Wagner, SUSE Labs
+#
+# Test queue count changes on reconnect
+
+. tests/nvme/rc
+
+DESCRIPTION="Test queue count changes on reconnect"
+QUICK=1
+
+requires() {
+	_nvme_requires
+	_have_loop
+	_require_nvme_trtype tcp rdma fc
+	_require_min_cpus 2
+}
+
+set_max_qid() {
+	local port="$1"
+	local subsys_name="$2"
+	local max_qid="$3"
+
+	_set_nvmet_attr_qid_max "${subsys_name}" "${max_qid}"
+
+	# Setting qid_max forces a disconnect and the reconntect attempt starts
+	# 10s after connection lost, thus we will see the connecting state.
+	_nvmf_wait_for_state "${subsys_name}" "connecting"
+	_nvmf_wait_for_state "${subsys_name}" "live"
+}
+
+test() {
+	local port
+	local subsys_name="blktests-subsystem-1"
+	local hostid
+	local hostnqn="nqn.2014-08.org.nvmexpress:uuid:${hostid}"
+	local file_path="${TMPDIR}/img"
+
+	echo "Running ${TEST_NAME}"
+
+	hostid="$(uuidgen)"
+	if [ -z "$hostid" ] ; then
+		echo "uuidgen failed"
+		return 1
+	fi
+
+	if ! _detect_nvmet_subsys_attr "attr_qid_max"; then
+		SKIP_REASONS+=("missing attr_qid_max feature")
+		return 1
+	fi
+
+	truncate -s 512M "${file_path}"
+
+	_create_nvmet_subsystem "${subsys_name}" "${file_path}" \
+		"b92842df-a394-44b1-84a4-92ae7d112861"
+	port="$(_create_nvmet_port "${nvme_trtype}")"
+	_add_nvmet_subsys_to_port "${port}" "${subsys_name}"
+	_create_nvmet_host "${subsys_name}" "${hostnqn}"
+
+	_nvme_connect_subsys "${nvme_trtype}" "${subsys_name}" \
+			     --hostnqn "${hostnqn}" \
+			     --hostid "${hostid}"
+
+	_nvmf_wait_for_state "${subsys_name}" "live"
+
+	set_max_qid "${port}" "${subsys_name}" 1
+	set_max_qid "${port}" "${subsys_name}" 128
+
+	_nvme_disconnect_subsys "${subsys_name}"
+
+	_remove_nvmet_subsystem_from_port "${port}" "${subsys_name}"
+	_remove_nvmet_subsystem "${subsys_name}"
+
+	_remove_nvmet_port "${port}"
+
+	_remove_nvmet_host "${hostnqn}"
+
+	rm "${file_path}"
+
+	echo "Test complete"
+}
diff --git a/tests/nvme/048.out b/tests/nvme/048.out
new file mode 100644
index 000000000000..7f986ef9637d
--- /dev/null
+++ b/tests/nvme/048.out
@@ -0,0 +1,3 @@
+Running nvme/048
+NQN:blktests-subsystem-1 disconnected 1 controller(s)
+Test complete
-- 
2.40.0

Re: [PATCH blktests v5 4/4] nvme/048: test queue count changes on reconnect

[Chaitanya Kulkarni]

On 4/5/23 08:46, Daniel Wagner wrote:
> The target is allowed to change the number of I/O queues. Test if the
> host is able to reconnect in this scenario.
>
> Signed-off-by: Daniel Wagner <dwagner@suse.de>
> ---
>

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH blktests v5 4/4] nvme/048: test queue count changes on reconnect

[Chaitanya Kulkarni]

> +	if ! _detect_nvmet_subsys_attr "attr_qid_max"; then
> +		SKIP_REASONS+=("missing attr_qid_max feature")
> +		return 1
> +	fi
> +
> +	truncate -s 512M "${file_path}"
> +
> +	_create_nvmet_subsystem "${subsys_name}" "${file_path}" \
> +		"b92842df-a394-44b1-84a4-92ae7d112861"

by checking following after create subsystem in testcase itself
we avoid whole process of creating and deleting subsystem and
additional function in the rc file, because we are already creating
subsystem as a part of the testcase :-

local attr="${NVMET_CFS}/subsystems/${subsys_name}/attr_qid_max"

#above tow vars go top of this function

if [ -f "${attr}" ];then
     SKIP_REASONS+=("missing attr_qid_max feature")
     #do appropriate error handling and jump to unwind code
fi

again please ignore this comment if decision has been made to
keep it this way for some reason...

> +	port="$(_create_nvmet_port "${nvme_trtype}")"
> +	_add_nvmet_subsys_to_port "${port}" "${subsys_name}"
> +	_create_nvmet_host "${subsys_name}" "${hostnqn}"
> +

-ck

Re: [PATCH blktests v5 4/4] nvme/048: test queue count changes on reconnect

[Daniel Wagner]

On Wed, Apr 05, 2023 at 06:57:49PM +0000, Chaitanya Kulkarni wrote:
> 
> > +	if ! _detect_nvmet_subsys_attr "attr_qid_max"; then
> > +		SKIP_REASONS+=("missing attr_qid_max feature")
> > +		return 1
> > +	fi
> > +
> > +	truncate -s 512M "${file_path}"
> > +
> > +	_create_nvmet_subsystem "${subsys_name}" "${file_path}" \
> > +		"b92842df-a394-44b1-84a4-92ae7d112861"
> 
> by checking following after create subsystem in testcase itself
> we avoid whole process of creating and deleting subsystem and
> additional function in the rc file, because we are already creating
> subsystem as a part of the testcase :-
> 
> local attr="${NVMET_CFS}/subsystems/${subsys_name}/attr_qid_max"
> 
> #above tow vars go top of this function
> 
> if [ -f "${attr}" ];then
>      SKIP_REASONS+=("missing attr_qid_max feature")
>      #do appropriate error handling and jump to unwind code
> fi
> 
> again please ignore this comment if decision has been made to
> keep it this way for some reason...

Again, no decision here. I think I overengineered this part slightly. Indeed if
we are goint to setup a controller anyway we should try to avoid double work.
This should also speed up the test slightly.

Talking about execution time, I was thinking on reducing the timeout value
to reduce the overall runtime.

Re: [PATCH blktests v5 4/4] nvme/048: test queue count changes on reconnect

[Chaitanya Kulkarni]

On 4/5/23 23:17, Daniel Wagner wrote:
> On Wed, Apr 05, 2023 at 06:57:49PM +0000, Chaitanya Kulkarni wrote:
>>> +	if ! _detect_nvmet_subsys_attr "attr_qid_max"; then
>>> +		SKIP_REASONS+=("missing attr_qid_max feature")
>>> +		return 1
>>> +	fi
>>> +
>>> +	truncate -s 512M "${file_path}"
>>> +
>>> +	_create_nvmet_subsystem "${subsys_name}" "${file_path}" \
>>> +		"b92842df-a394-44b1-84a4-92ae7d112861"
>> by checking following after create subsystem in testcase itself
>> we avoid whole process of creating and deleting subsystem and
>> additional function in the rc file, because we are already creating
>> subsystem as a part of the testcase :-
>>
>> local attr="${NVMET_CFS}/subsystems/${subsys_name}/attr_qid_max"
>>
>> #above tow vars go top of this function
>>
>> if [ -f "${attr}" ];then
>>       SKIP_REASONS+=("missing attr_qid_max feature")
>>       #do appropriate error handling and jump to unwind code
>> fi
>>
>> again please ignore this comment if decision has been made to
>> keep it this way for some reason...
> Again, no decision here. I think I overengineered this part slightly. Indeed if
> we are goint to setup a controller anyway we should try to avoid double work.
> This should also speed up the test slightly.
>
> Talking about execution time, I was thinking on reducing the timeout value
> to reduce the overall runtime.

okay let's go with the above suggestion than ? and overall let's only
add to rc when there are multiple users to the function with prep
patches moving the code, otherwise it gets bloated for no reason..

overall speed of the testcase is also very important since this gets
run with bunch of other testcases/testsuite(s) so keeping the testcase speed
minimum is always desirable ...

-ck