[PATCH blktests v3 0/3] More fixes for FC enabling

[PATCH blktests v3 0/3] More fixes for FC enabling

[Daniel Wagner]

I've rebased the series on the current HEAD and moved the def_hostnqn bits from
the second patch to the third. This should make it more consistent in commit
history.

Also retested and found a bug in the context matching code in libnvme which I
post a fix too.

changes:
v3:
  - rebased
  - moved def_hostnqn bits from second patch to third
  
v2:
  - https://lore.kernel.org/linux-nvme/20230628151623.11340-1-dwagner@suse.de/ 
  - nvme/048:
    - untaggle waiting for state change and queue count check
    - make all variables local
    - compare numbers not strings
  - nvme/rc:
    - rename _nvme_cli_supports_context to _have_nvme_cli_context
    - only add --context for fc
    - reordered setup/cleanup
  - nvme/{041,042,043,044,045,048}:
    - move all changes related to this patch
v1:
  - https://lore.kernel.org/linux-nvme/20230620132703.20648-1-dwagner@suse.de/

Daniel Wagner (3):
  nvme/048: Check for queue count check directly
  nvme/rc: Avoid triggering host nvme-cli autoconnect
  nvme/{041,042,043,044,045,048}: Use default hostnqn and hostid

 tests/nvme/041 |  8 ++---
 tests/nvme/042 |  8 ++---
 tests/nvme/043 |  8 ++---
 tests/nvme/044 |  8 ++---
 tests/nvme/045 |  8 ++---
 tests/nvme/048 | 32 +++++++++++++-------
 tests/nvme/rc  | 81 +++++++++++++++++++++++++++++++++++++++-----------
 7 files changed, 96 insertions(+), 57 deletions(-)

-- 
2.41.0

[PATCH blktests v3 1/3] nvme/048: Check for queue count check directly

[Daniel Wagner]

The test monitored the state changes live -> resetting -> connecting ->
live, to figure out the queue count change was successful.

The fc transport is reconnecting very fast and the state transitions
are not observed by the current approach.

So instead trying to monitor the state changes, let's just wait for the
live state and the correct queue number.

As queue count is depending on the number of online CPUs we explicitly
use 1 and 2 for the max_queue count. This means the queue_count value
needs to reach either 2 or 3 (admin queue included).

Signed-off-by: Daniel Wagner <dwagner@suse.de>
---
 tests/nvme/048 | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/tests/nvme/048 b/tests/nvme/048
index 81084f0440c2..bbfb9873b5e8 100755
--- a/tests/nvme/048
+++ b/tests/nvme/048
@@ -42,6 +42,24 @@ nvmf_wait_for_state() {
 	return 0
 }
 
+nvmf_check_queue_count() {
+	local subsys_name="$1"
+	local queue_count="$2"
+	local nvmedev
+	local queue_count_file
+
+	nvmedev=$(_find_nvme_dev "${subsys_name}")
+	queue_count_file=$(cat /sys/class/nvme-fabrics/ctl/"${nvmedev}"/queue_count)
+
+	queue_count=$((queue_count + 1))
+	if [[ "${queue_count}" -ne "${queue_count_file}" ]]; then
+		echo "expected queue count ${queue_count} not set"
+		return 1
+	fi
+
+	return 0
+}
+
 set_nvmet_attr_qid_max() {
 	local nvmet_subsystem="$1"
 	local qid_max="$2"
@@ -56,10 +74,8 @@ set_qid_max() {
 	local qid_max="$3"
 
 	set_nvmet_attr_qid_max "${subsys_name}" "${qid_max}"
-
-	# Setting qid_max forces a disconnect and the reconntect attempt starts
-	nvmf_wait_for_state "${subsys_name}" "connecting" || return 1
 	nvmf_wait_for_state "${subsys_name}" "live" || return 1
+	nvmf_check_queue_count "${subsys_name}" "${qid_max}" || return 1
 
 	return 0
 }
@@ -103,7 +119,7 @@ test() {
 			echo FAIL
 		else
 			set_qid_max "${port}" "${subsys_name}" 1 || echo FAIL
-			set_qid_max "${port}" "${subsys_name}" 128 || echo FAIL
+			set_qid_max "${port}" "${subsys_name}" 2 || echo FAIL
 		fi
 
 		_nvme_disconnect_subsys "${subsys_name}"
-- 
2.41.0

[PATCH blktests v3 2/3] nvme/rc: Avoid triggering host nvme-cli autoconnect

[Daniel Wagner]

When the host has enabled the udev/systemd autoconnect services for the
fc transport it interacts with blktests and make tests break.

nvme-cli learned to ignore connects attemps when using the --context
command line option paired with a volatile configuration. Thus we can
mark all the resources created by blktests and avoid any interaction
with the systemd autoconnect scripts.

Only enabled this for the fc transport.

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

diff --git a/tests/nvme/rc b/tests/nvme/rc
index 1c2c2fab62c1..9b5442b2c07b 100644
--- a/tests/nvme/rc
+++ b/tests/nvme/rc
@@ -176,6 +176,57 @@ _nvme_calc_rand_io_size() {
 	echo "${io_size_kb}k"
 }
 
+_have_nvme_cli_context() {
+	# ignore all non-fc transports for now
+	if [[ "${nvme_trtype}" != "fc" ]] ||
+	   ! nvme connect --help 2>&1 | grep -q -- '--context=<STR>' > /dev/null; then
+		return 1
+	fi
+	return 0
+}
+
+_setup_nvme_cli() {
+	local local_wwnn="${1}"
+	local local_wwpn="${2}"
+	local remote_wwnn="${3}"
+	local remote_wwpn="${4}"
+
+	if ! _have_nvme_cli_context; then
+		return
+	fi
+
+	mkdir -p /run/nvme
+	cat >> /run/nvme/blktests.json <<-EOF
+	[
+	  {
+	    "hostnqn": "${def_hostnqn}",
+	    "hostid": "${def_hostid}",
+	    "subsystems": [
+	      {
+	        "application": "blktests",
+	        "nqn": "blktests-subsystem-1",
+	        "ports": [
+	          {
+	            "transport": "fc",
+	            "traddr": "nn-${remote_wwnn}:pn-${remote_wwpn}",
+	            "host_traddr": "nn-${local_wwnn}:pn-${local_wwpn}"
+	          }
+	        ]
+	      }
+	    ]
+	  }
+	]
+	EOF
+}
+
+_cleanup_nvme_cli() {
+	if ! _have_nvme_cli_context; then
+		return
+	fi
+
+	rm -f /run/nvme/blktests.json
+}
+
 _nvme_fcloop_add_rport() {
 	local local_wwnn="$1"
 	local local_wwpn="$2"
@@ -208,6 +259,9 @@ _setup_fcloop() {
 	local remote_wwnn="${3:-$def_remote_wwnn}"
 	local remote_wwpn="${4:-$def_remote_wwpn}"
 
+	_setup_nvme_cli "${local_wwnn}" "${local_wwpn}" \
+			"${remote_wwnn}" "${remote_wwpn}"
+
 	_nvme_fcloop_add_tport "${remote_wwnn}" "${remote_wwpn}"
 	_nvme_fcloop_add_lport "${local_wwnn}" "${local_wwpn}"
 	_nvme_fcloop_add_rport "${local_wwnn}" "${local_wwpn}" \
@@ -250,6 +304,8 @@ _cleanup_fcloop() {
 	_nvme_fcloop_del_lport "${local_wwnn}" "${local_wwpn}"
 	_nvme_fcloop_del_rport "${local_wwnn}" "${local_wwpn}" \
 			       "${remote_wwnn}" "${remote_wwpn}"
+
+	_cleanup_nvme_cli
 }
 
 _cleanup_nvmet() {
@@ -452,6 +508,9 @@ _nvme_connect_subsys() {
 	subsysnqn="$2"
 
 	ARGS=(-t "${trtype}" -n "${subsysnqn}")
+	if _have_nvme_cli_context; then
+		ARGS+=(--context="blktests")
+	fi
 	if [[ "${trtype}" == "fc" ]] ; then
 		ARGS+=(-a "${traddr}" -w "${host_traddr}")
 	elif [[ "${trtype}" != "loop" ]]; then
@@ -496,6 +555,9 @@ _nvme_discover() {
 	ARGS=(-t "${trtype}")
 	ARGS+=(--hostnqn="${def_hostnqn}")
 	ARGS+=(--hostid="${def_hostid}")
+	if _have_nvme_cli_context; then
+		ARGS+=(--context="blktests")
+	fi
 	if [[ "${trtype}" = "fc" ]]; then
 		ARGS+=(-a "${traddr}" -w "${host_traddr}")
 	elif [[ "${trtype}" != "loop" ]]; then
-- 
2.41.0

[PATCH blktests v3 3/3] nvme/{041,042,043,044,045,048}: Use default hostnqn and hostid

[Daniel Wagner]

The host might have enabled the udev/systemd auto connect feature.
This disturbs the blktests for the fc transport. nvme-cli is able
to distinguish between the different invocations via the --context
option.

Instead creating random generated IDs or reuse the hostnqn, it's safer
to always use the same hostnqn to reduce the risk that the matching
logic doesn't work.

Signed-off-by: Daniel Wagner <dwagner@suse.de>
---
 tests/nvme/041 |  8 ++------
 tests/nvme/042 |  8 ++------
 tests/nvme/043 |  8 ++------
 tests/nvme/044 |  8 ++------
 tests/nvme/045 |  8 ++------
 tests/nvme/048 |  8 ++------
 tests/nvme/rc  | 19 ++-----------------
 7 files changed, 14 insertions(+), 53 deletions(-)

diff --git a/tests/nvme/041 b/tests/nvme/041
index 308655dd6090..5b04b99b128e 100755
--- a/tests/nvme/041
+++ b/tests/nvme/041
@@ -30,12 +30,8 @@ test() {
 
 	echo "Running ${TEST_NAME}"
 
-	hostid="$(uuidgen)"
-	if [ -z "$hostid" ] ; then
-		echo "uuidgen failed"
-		return 1
-	fi
-	hostnqn="nqn.2014-08.org.nvmexpress:uuid:${hostid}"
+	hostid="${def_hostid}"
+	hostnqn="${def_hostnqn}"
 	hostkey="$(nvme gen-dhchap-key -n ${subsys_name} 2> /dev/null)"
 	if [ -z "$hostkey" ] ; then
 		echo "nvme gen-dhchap-key failed"
diff --git a/tests/nvme/042 b/tests/nvme/042
index fed2efead013..8df5ed37aacc 100755
--- a/tests/nvme/042
+++ b/tests/nvme/042
@@ -32,12 +32,8 @@ test() {
 
 	echo "Running ${TEST_NAME}"
 
-	hostid="$(uuidgen)"
-	if [ -z "$hostid" ] ; then
-		echo "uuidgen failed"
-		return 1
-	fi
-	hostnqn="nqn.2014-08.org.nvmexpress:uuid:${hostid}"
+	hostid="${def_hostid}"
+	hostnqn="${def_hostnqn}"
 
 	_setup_nvmet
 
diff --git a/tests/nvme/043 b/tests/nvme/043
index a030884aa4ed..b591e39d0706 100755
--- a/tests/nvme/043
+++ b/tests/nvme/043
@@ -33,12 +33,8 @@ test() {
 
 	echo "Running ${TEST_NAME}"
 
-	hostid="$(uuidgen)"
-	if [ -z "$hostid" ] ; then
-		echo "uuidgen failed"
-		return 1
-	fi
-	hostnqn="nqn.2014-08.org.nvmexpress:uuid:${hostid}"
+	hostid="${def_hostid}"
+	hostnqn="${def_hostnqn}"
 
 	_setup_nvmet
 
diff --git a/tests/nvme/044 b/tests/nvme/044
index 9928bcc55397..fca0897af27b 100755
--- a/tests/nvme/044
+++ b/tests/nvme/044
@@ -32,12 +32,8 @@ test() {
 
 	echo "Running ${TEST_NAME}"
 
-	hostid="$(uuidgen)"
-	if [ -z "$hostid" ] ; then
-		echo "uuidgen failed"
-		return 1
-	fi
-	hostnqn="nqn.2014-08.org.nvmexpress:uuid:${hostid}"
+	hostid="${def_hostid}"
+	hostnqn="${def_hostnqn}"
 
 	hostkey="$(nvme gen-dhchap-key -n ${subsys_name} 2> /dev/null)"
 	if [ -z "$hostkey" ] ; then
diff --git a/tests/nvme/045 b/tests/nvme/045
index 26a55335a92c..eca629a18691 100755
--- a/tests/nvme/045
+++ b/tests/nvme/045
@@ -36,12 +36,8 @@ test() {
 
 	echo "Running ${TEST_NAME}"
 
-	hostid="$(uuidgen)"
-	if [ -z "$hostid" ] ; then
-		echo "uuidgen failed"
-		return 1
-	fi
-	hostnqn="nqn.2014-08.org.nvmexpress:uuid:${hostid}"
+	hostid="${def_hostid}"
+	hostnqn="${def_hostnqn}"
 
 	hostkey="$(nvme gen-dhchap-key -n ${subsys_name} 2> /dev/null)"
 	if [ -z "$hostkey" ] ; then
diff --git a/tests/nvme/048 b/tests/nvme/048
index bbfb9873b5e8..a6ebb8927865 100755
--- a/tests/nvme/048
+++ b/tests/nvme/048
@@ -93,12 +93,8 @@ test() {
 
 	_setup_nvmet
 
-	hostid="$(uuidgen)"
-	if [ -z "$hostid" ] ; then
-		echo "uuidgen failed"
-		return 1
-	fi
-	hostnqn="nqn.2014-08.org.nvmexpress:uuid:${hostid}"
+	hostid="${def_hostid}"
+	hostnqn="${def_hostnqn}"
 
 	truncate -s "${nvme_img_size}" "${file_path}"
 
diff --git a/tests/nvme/rc b/tests/nvme/rc
index 9b5442b2c07b..4f3a994d75ce 100644
--- a/tests/nvme/rc
+++ b/tests/nvme/rc
@@ -14,23 +14,8 @@ def_remote_wwnn="0x10001100aa000001"
 def_remote_wwpn="0x20001100aa000001"
 def_local_wwnn="0x10001100aa000002"
 def_local_wwpn="0x20001100aa000002"
-
-if [ -f "/etc/nvme/hostid" ]; then
-	def_hostid="$(cat /etc/nvme/hostid 2> /dev/null)"
-else
-	def_hostid="$(uuidgen)"
-fi
-if [ -z "$def_hostid" ] ; then
-	def_hostid="0f01fb42-9f7f-4856-b0b3-51e60b8de349"
-fi
-
-if [ -f "/etc/nvme/hostnqn" ]; then
-	def_hostnqn="$(cat /etc/nvme/hostnqn 2> /dev/null)"
-fi
-if [ -z "$def_hostnqn" ] ; then
-	def_hostnqn="nqn.2014-08.org.nvmexpress:uuid:${def_hostid}"
-fi
-
+def_hostid="0f01fb42-9f7f-4856-b0b3-51e60b8de349"
+def_hostnqn="nqn.2014-08.org.nvmexpress:uuid:${def_hostid}"
 nvme_trtype=${nvme_trtype:-"loop"}
 nvme_img_size=${nvme_img_size:-"1G"}
 nvme_num_iter=${nvme_num_iter:-"1000"}
-- 
2.41.0

Re: [PATCH blktests v3 0/3] More fixes for FC enabling

[Shinichiro Kawasaki]

On Jul 03, 2023 / 11:16, Daniel Wagner wrote:
> I've rebased the series on the current HEAD and moved the def_hostnqn bits from
> the second patch to the third. This should make it more consistent in commit
> history.
> 
> Also retested and found a bug in the context matching code in libnvme which I
> post a fix too.

Thanks Daniel.

One thing I noticed is that now the 3rd patch removes the references from
nvme/rc to /etc/nvme/hostid and /etc/nvme/hostnqn. I'm ok with this change since
I see the motivation discussed here [1]. I will wait for several days to make
sure it's ok for others. In case anyone relies on those files to set up blktests
environment, please speak up.

[1] https://lore.kernel.org/linux-nvme/ajcm6yupguickaucansiuzjqatyz5qijnnp4topxv64cisbblc@4sgv3bd3jl4q/

Other that, changes in v3 looks good to me. If there is no voice, I'll apply
the patches as they are.

Re: [PATCH blktests v3 0/3] More fixes for FC enabling

[Chaitanya Kulkarni]

On 7/4/2023 12:08 AM, Shinichiro Kawasaki wrote:
> On Jul 03, 2023 / 11:16, Daniel Wagner wrote:
>> I've rebased the series on the current HEAD and moved the def_hostnqn bits from
>> the second patch to the third. This should make it more consistent in commit
>> history.
>>
>> Also retested and found a bug in the context matching code in libnvme which I
>> post a fix too.
> 
> Thanks Daniel.
> 
> One thing I noticed is that now the 3rd patch removes the references from
> nvme/rc to /etc/nvme/hostid and /etc/nvme/hostnqn. I'm ok with this change since
> I see the motivation discussed here [1]. I will wait for several days to make
> sure it's ok for others. In case anyone relies on those files to set up blktests
> environment, please speak up.
> 
> [1] https://lore.kernel.org/linux-nvme/ajcm6yupguickaucansiuzjqatyz5qijnnp4topxv64cisbblc@4sgv3bd3jl4q/
> 
> Other that, changes in v3 looks good to me. If there is no voice, I'll apply
> the patches as they are.

Adding Max and Yi who were part of original discussion [1].

-ck

Re: [PATCH blktests v3 0/3] More fixes for FC enabling

[Shinichiro Kawasaki]

On Jul 06, 2023 / 06:51, Chaitanya Kulkarni wrote:
> 
> On 7/4/2023 12:08 AM, Shinichiro Kawasaki wrote:
> > On Jul 03, 2023 / 11:16, Daniel Wagner wrote:
> >> I've rebased the series on the current HEAD and moved the def_hostnqn bits from
> >> the second patch to the third. This should make it more consistent in commit
> >> history.
> >>
> >> Also retested and found a bug in the context matching code in libnvme which I
> >> post a fix too.
> > 
> > Thanks Daniel.
> > 
> > One thing I noticed is that now the 3rd patch removes the references from
> > nvme/rc to /etc/nvme/hostid and /etc/nvme/hostnqn. I'm ok with this change since
> > I see the motivation discussed here [1]. I will wait for several days to make
> > sure it's ok for others. In case anyone relies on those files to set up blktests
> > environment, please speak up.
> > 
> > [1] https://lore.kernel.org/linux-nvme/ajcm6yupguickaucansiuzjqatyz5qijnnp4topxv64cisbblc@4sgv3bd3jl4q/
> > 
> > Other that, changes in v3 looks good to me. If there is no voice, I'll apply
> > the patches as they are.
> 
> Adding Max and Yi who were part of original discussion [1].

I saw no response to this v3 series, so I've applied it. Of note is that I
resolved a simple conflict of the 3rd patch and added a note about /etc/nvme/*
in its commit message. Daniel, thanks for this work.

However, after this action, I noticed that Max made a question in the e-mail
thread of the v1 series. I might have been going too fast. I hope Daniel can
answer that question.