[PATCH blktests v3 0/2] Test the NVMe reservation feature

[PATCH blktests v3 0/2] Test the NVMe reservation feature

[Guixin Liu]

Changes from v2 to v3:
- Put calling "resv-report --help" to the beginning of test_resv().
- Introduce a hlper resv_report.
- Fit newest blktests version. 

Changes from v1 to v2:
- Add a new patch to add a optinal argument --resv_enable.

- Make resv-report fit the new --eds argument.

- Filter the hosid output to make the new test case independent from
hostid.

- Change the new test case name to "Test the NVMe reservation feature".

Guixin Liu (2):
  nvme/{md/001,rc,002,016,017,030,052}: introduce --resv_enable argument
  nvme: test the nvme reservation feature

 common/nvme        |  89 +++++++++++++++++++++++++++++++------
 tests/md/001       |   4 +-
 tests/nvme/002     |   3 +-
 tests/nvme/016     |   7 ++-
 tests/nvme/017     |  10 +++--
 tests/nvme/030     |   6 ++-
 tests/nvme/052     |   5 ++-
 tests/nvme/054     |  99 +++++++++++++++++++++++++++++++++++++++++
 tests/nvme/054.out | 108 +++++++++++++++++++++++++++++++++++++++++++++
 tests/nvme/rc      |  11 ++++-
 10 files changed, 316 insertions(+), 26 deletions(-)
 create mode 100644 tests/nvme/054
 create mode 100644 tests/nvme/054.out

-- 
2.43.0

[PATCH blktests v3 1/2] nvme/{md/001,rc,002,016,017,030,052}: introduce --resv_enable argument

[Guixin Liu]

Add an optional argument --resv_enable to _nvmet_target_setup() and
propagate it to _create_nvmet_subsystem() and _create_nvmet_ns().

One can call functions with --resv_enable to enable reservation
feature on a specific namespace.

And also make _create_nvmet_ns and _create_nvmet_subsystem to parse
for arguments, this makes these functions more flexible to use.

Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---
 common/nvme    | 89 ++++++++++++++++++++++++++++++++++++++++++--------
 tests/md/001   |  4 ++-
 tests/nvme/002 |  3 +-
 tests/nvme/016 |  7 ++--
 tests/nvme/017 | 10 +++---
 tests/nvme/030 |  6 ++--
 tests/nvme/052 |  5 ++-
 tests/nvme/rc  | 11 +++++--
 8 files changed, 109 insertions(+), 26 deletions(-)

diff --git a/common/nvme b/common/nvme
index 9e78f3e..c1aa8d6 100644
--- a/common/nvme
+++ b/common/nvme
@@ -452,32 +452,95 @@ _remove_nvmet_port() {
 }
 
 _create_nvmet_ns() {
-	local nvmet_subsystem="$1"
-	local nsid="$2"
-	local blkdev="$3"
+	local nvmet_subsystem=""
+	local nsid=""
+	local blkdev=""
 	local uuid="00000000-0000-0000-0000-000000000000"
-	local subsys_path="${NVMET_CFS}/subsystems/${nvmet_subsystem}"
-	local ns_path="${subsys_path}/namespaces/${nsid}"
+	local subsys_path=""
+	local ns_path=""
+	local resv_enable=false
 
-	if [[ $# -eq 4 ]]; then
-		uuid="$4"
-	fi
+	while [[ $# -gt 0 ]]; do
+		case $1 in
+			--subsysnqn)
+				nvmet_subsystem="$2"
+				shift 2
+				;;
+			--nsid)
+				nsid="$2"
+				shift 2
+				;;
+			--blkdev)
+				blkdev="$2"
+				shift 2
+				;;
+			--uuid)
+				uuid="$2"
+				shift 2
+				;;
+			--resv_enable)
+				resv_enable=true
+				shift 1
+				;;
+			*)
+				echo "WARNING: unknown argument: $1"
+				shift
+				;;
+		esac
+	done
+
+	subsys_path="${NVMET_CFS}/subsystems/${nvmet_subsystem}"
+	ns_path="${subsys_path}/namespaces/${nsid}"
 
 	mkdir "${ns_path}"
 	printf "%s" "${blkdev}" > "${ns_path}/device_path"
 	printf "%s" "${uuid}" > "${ns_path}/device_uuid"
+	if [[ -f "${ns_path}/resv_enable" && "${resv_enable}" = true ]] ; then
+		printf 1 > "${ns_path}/resv_enable"
+	fi
 	printf 1 > "${ns_path}/enable"
 }
 
 _create_nvmet_subsystem() {
-	local nvmet_subsystem="$1"
-	local blkdev="$2"
-	local uuid=$3
-	local cfs_path="${NVMET_CFS}/subsystems/${nvmet_subsystem}"
+	local nvmet_subsystem=""
+	local blkdev=""
+	local uuid="00000000-0000-0000-0000-000000000000"
+	local resv_enable=""
+	local cfs_path=""
+
+	while [[ $# -gt 0 ]]; do
+		case $1 in
+			--subsysnqn)
+				nvmet_subsystem="$2"
+				shift 2
+				;;
+			--blkdev)
+				blkdev="$2"
+				shift 2
+				;;
+			--uuid)
+				uuid="$2"
+				shift 2
+				;;
+			--resv_enable)
+				resv_enable="--resv_enable";
+				shift 1
+				;;
+			*)
+				echo "WARNING: unknown argument: $1"
+				shift
+				;;
+		esac
+	done
 
+	cfs_path="${NVMET_CFS}/subsystems/${nvmet_subsystem}"
 	mkdir -p "${cfs_path}"
 	echo 0 > "${cfs_path}/attr_allow_any_host"
-	_create_nvmet_ns "${nvmet_subsystem}" "1" "${blkdev}" "${uuid}"
+	_create_nvmet_ns --subsysnqn "${nvmet_subsystem}" \
+			 --nsid "1" \
+			 --blkdev "${blkdev}" \
+			 --uuid "${uuid}" \
+			 ${resv_enable}
 }
 
 _add_nvmet_allow_hosts() {
diff --git a/tests/md/001 b/tests/md/001
index 27df7b3..98da51d 100755
--- a/tests/md/001
+++ b/tests/md/001
@@ -52,7 +52,9 @@ setup_nvme_over_tcp() {
 	local port
 	port="$(_create_nvmet_port "${nvme_trtype}")"
 
-	_create_nvmet_subsystem "${def_subsysnqn}" "/dev/mapper/ram0_big_optio" "${def_subsys_uuid}"
+	_create_nvmet_subsystem --subsysnqn "${def_subsysnqn}" \
+				--blkdev "/dev/mapper/ram0_big_optio" \
+				--uuid "${def_subsys_uuid}"
 	_add_nvmet_subsys_to_port "${port}" "${def_subsysnqn}"
 
 	_create_nvmet_host "${def_subsysnqn}" "${def_hostnqn}"
diff --git a/tests/nvme/002 b/tests/nvme/002
index f613c78..043ab1c 100755
--- a/tests/nvme/002
+++ b/tests/nvme/002
@@ -34,7 +34,8 @@ test() {
 	local genctr=1
 
 	for ((i = 0; i < iterations; i++)); do
-		_create_nvmet_subsystem "blktests-subsystem-$i" "${loop_dev}"
+		_create_nvmet_subsystem --subsysnqn "blktests-subsystem-$i" \
+					--blkdev "${loop_dev}"
 		_add_nvmet_subsys_to_port "${port}" "blktests-subsystem-$i"
 	done
 
diff --git a/tests/nvme/016 b/tests/nvme/016
index d1fdb35..1143cab 100755
--- a/tests/nvme/016
+++ b/tests/nvme/016
@@ -29,10 +29,13 @@ test() {
 	loop_dev="$(losetup -f)"
 	local genctr=1
 
-	_create_nvmet_subsystem "${def_subsysnqn}" "${loop_dev}"
+	_create_nvmet_subsystem --subsysnqn "${def_subsysnqn}" \
+				--blkdev "${loop_dev}"
 
 	for ((i = 2; i <= iterations; i++)); do
-		_create_nvmet_ns "${def_subsysnqn}" "${i}" "${loop_dev}"
+		_create_nvmet_ns --subsysnqn "${def_subsysnqn}" \
+				 --nsid "${i}" \
+				 --blkdev "${loop_dev}"
 	done
 
 	port="$(_create_nvmet_port "${nvme_trtype}")"
diff --git a/tests/nvme/017 b/tests/nvme/017
index 114be60..5721000 100755
--- a/tests/nvme/017
+++ b/tests/nvme/017
@@ -29,12 +29,14 @@ test() {
 
 	local genctr=1
 
-	_create_nvmet_subsystem "${def_subsysnqn}" "$(_nvme_def_file_path)" \
-		"${def_subsys_uuid}"
+	_create_nvmet_subsystem --subsysnqn "${def_subsysnqn}" \
+				--blkdev "$(_nvme_def_file_path)" \
+				--uuid "${def_subsys_uuid}"
 
 	for ((i = 2; i <= iterations; i++)); do
-		_create_nvmet_ns "${def_subsysnqn}" "${i}" \
-				 "$(_nvme_def_file_path)"
+		_create_nvmet_ns --subsysnqn "${def_subsysnqn}" \
+				 --nsid "${i}" \
+				 --blkdev "$(_nvme_def_file_path)"
 	done
 
 	port="$(_create_nvmet_port "${nvme_trtype}")"
diff --git a/tests/nvme/030 b/tests/nvme/030
index b1ed8bc..5db20c0 100755
--- a/tests/nvme/030
+++ b/tests/nvme/030
@@ -30,13 +30,15 @@ test() {
 
 	port="$(_create_nvmet_port "${nvme_trtype}")"
 
-	_create_nvmet_subsystem "${subsys}1" "$(losetup -f)"
+	_create_nvmet_subsystem --subsysnqn "${subsys}1" \
+				--blkdev "$(losetup -f)"
 	_add_nvmet_subsys_to_port "${port}" "${subsys}1"
 	_create_nvmet_host "${subsys}1" "${def_hostnqn}"
 
 	genctr=$(_discovery_genctr)
 
-	_create_nvmet_subsystem "${subsys}2" "$(losetup -f)"
+	_create_nvmet_subsystem --subsysnqn "${subsys}2" \
+				--blkdev "$(losetup -f)"
 	_add_nvmet_subsys_to_port "${port}" "${subsys}2"
 
 	genctr=$(_check_genctr "${genctr}" "adding a subsystem to a port")
diff --git a/tests/nvme/052 b/tests/nvme/052
index 401f043..1dcda23 100755
--- a/tests/nvme/052
+++ b/tests/nvme/052
@@ -64,7 +64,10 @@ test() {
 		truncate -s "${NVME_IMG_SIZE}" "$(_nvme_def_file_path).$i"
 		uuid="$(uuidgen -r)"
 
-		_create_nvmet_ns "${def_subsysnqn}" "${i}" "$(_nvme_def_file_path).$i" "${uuid}"
+		_create_nvmet_ns --subsysnqn "${def_subsysnqn}" \
+				--nsid "${i}" \
+				--blkdev "$(_nvme_def_file_path).$i" \
+				--uuid "${uuid}"
 
 		# wait until async request is processed and ns is created
 		if ! nvmf_wait_for_ns "${uuid}" created; then
diff --git a/tests/nvme/rc b/tests/nvme/rc
index 671012e..357cab9 100644
--- a/tests/nvme/rc
+++ b/tests/nvme/rc
@@ -324,6 +324,7 @@ _nvmet_target_setup() {
 	local subsysnqn="${def_subsysnqn}"
 	local subsys_uuid="${def_subsys_uuid}"
 	local port
+	local resv_enable=""
 
 	while [[ $# -gt 0 ]]; do
 		case $1 in
@@ -347,6 +348,10 @@ _nvmet_target_setup() {
 				subsys_uuid="$2"
 				shift 2
 				;;
+			--resv_enable)
+				resv_enable="--resv_enable"
+				shift 1
+				;;
 			*)
 				echo "WARNING: unknown argument: $1"
 				shift
@@ -361,8 +366,10 @@ _nvmet_target_setup() {
 		blkdev="$(_nvme_def_file_path)"
 	fi
 
-	_create_nvmet_subsystem "${subsysnqn}" "${blkdev}" \
-				"${subsys_uuid}"
+	_create_nvmet_subsystem --subsysnqn "${subsysnqn}" \
+				--blkdev "${blkdev}" \
+				--uuid "${subsys_uuid}" \
+				${resv_enable}
 	port="$(_create_nvmet_port "${nvme_trtype}")"
 	_add_nvmet_subsys_to_port "${port}" "${subsysnqn}"
 	_create_nvmet_host "${subsysnqn}" "${def_hostnqn}" \
-- 
2.43.0

[PATCH blktests v3 2/2] nvme: test the nvme reservation feature

[Guixin Liu]

Test the NVMe reservation feature, including register, acquire,
release and report.

Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
---
 tests/nvme/054     |  99 +++++++++++++++++++++++++++++++++++++++++
 tests/nvme/054.out | 108 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 207 insertions(+)
 create mode 100644 tests/nvme/054
 create mode 100644 tests/nvme/054.out

diff --git a/tests/nvme/054 b/tests/nvme/054
new file mode 100644
index 0000000..f352c73
--- /dev/null
+++ b/tests/nvme/054
@@ -0,0 +1,99 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2024 Guixin Liu
+# Copyright (C) 2024 Alibaba Group.
+#
+# Test the NVMe reservation feature
+#
+. tests/nvme/rc
+
+DESCRIPTION="Test the NVMe reservation feature"
+QUICK=1
+nvme_trtype="loop"
+
+requires() {
+	_nvme_requires
+}
+
+resv_report() {
+	local nvmedev=$1
+	local report_arg=$2
+
+	nvme resv-report "/dev/${nvmedev}n1" "${report_arg}" | grep -v "hostid"
+}
+
+test_resv() {
+	local nvmedev=$1
+	local report_arg="--cdw11=1"
+
+	if nvme resv-report --help 2>&1 | grep -- '--eds' > /dev/null; then
+		report_arg="--eds"
+	fi
+
+	echo "Register"
+	resv_report "${nvmedev}" "${report_arg}"
+	nvme resv-register "/dev/${nvmedev}n1" --nrkey=4 --rrega=0
+	resv_report "${nvmedev}" "${report_arg}"
+
+	echo "Replace"
+	nvme resv-register "/dev/${nvmedev}n1" --crkey=4 --nrkey=5 --rrega=2
+	resv_report "${nvmedev}" "${report_arg}"
+
+	echo "Unregister"
+	nvme resv-register "/dev/${nvmedev}n1" --crkey=5 --rrega=1
+	resv_report "${nvmedev}" "${report_arg}"
+
+	echo "Acquire"
+	nvme resv-register "/dev/${nvmedev}n1" --nrkey=4 --rrega=0
+	nvme resv-acquire "/dev/${nvmedev}n1" --crkey=4 --rtype=1 --racqa=0
+	resv_report "${nvmedev}" "${report_arg}"
+
+	echo "Preempt"
+	nvme resv-acquire "/dev/${nvmedev}n1" --crkey=4 --rtype=2 --racqa=1
+	resv_report "${nvmedev}" "${report_arg}"
+
+	echo "Release"
+	nvme resv-release "/dev/${nvmedev}n1" --crkey=4 --rtype=2 --rrela=0
+	resv_report "${nvmedev}" "${report_arg}"
+
+	echo "Clear"
+	nvme resv-register "/dev/${nvmedev}n1" --nrkey=4 --rrega=0
+	nvme resv-acquire "/dev/${nvmedev}n1" --crkey=4 --rtype=1 --racqa=0
+	resv_report "${nvmedev}" "${report_arg}"
+	nvme resv-release "/dev/${nvmedev}n1" --crkey=4 --rrela=1
+}
+
+test() {
+	echo "Running ${TEST_NAME}"
+
+	_setup_nvmet
+
+	local nvmedev
+	local skipped=false
+	local subsys_path=""
+	local ns_path=""
+
+	_nvmet_target_setup --blkdev file --resv_enable
+	subsys_path="${NVMET_CFS}/subsystems/${def_subsysnqn}"
+	ns_path="${subsys_path}/namespaces/1"
+
+	if [[ -f "${ns_path}/resv_enable" ]] ; then
+		_nvme_connect_subsys
+
+		nvmedev=$(_find_nvme_dev "${def_subsysnqn}")
+
+		test_resv "${nvmedev}"
+		_nvme_disconnect_subsys
+	else
+		SKIP_REASONS+=("missing reservation feature")
+		skipped=true
+	fi
+
+	_nvmet_target_cleanup
+
+	if [[ "${skipped}" = true ]] ; then
+		return 1
+	fi
+
+	echo "Test complete"
+}
diff --git a/tests/nvme/054.out b/tests/nvme/054.out
new file mode 100644
index 0000000..66e51dd
--- /dev/null
+++ b/tests/nvme/054.out
@@ -0,0 +1,108 @@
+Running nvme/054
+Register
+
+NVME Reservation status:
+
+gen       : 0
+rtype     : 0
+regctl    : 0
+ptpls     : 0
+
+NVME Reservation  success
+
+NVME Reservation status:
+
+gen       : 1
+rtype     : 0
+regctl    : 1
+ptpls     : 0
+regctlext[0] :
+  cntlid     : ffff
+  rcsts      : 0
+  rkey       : 4
+
+Replace
+NVME Reservation  success
+
+NVME Reservation status:
+
+gen       : 2
+rtype     : 0
+regctl    : 1
+ptpls     : 0
+regctlext[0] :
+  cntlid     : ffff
+  rcsts      : 0
+  rkey       : 5
+
+Unregister
+NVME Reservation  success
+
+NVME Reservation status:
+
+gen       : 3
+rtype     : 0
+regctl    : 0
+ptpls     : 0
+
+Acquire
+NVME Reservation  success
+NVME Reservation Acquire success
+
+NVME Reservation status:
+
+gen       : 4
+rtype     : 1
+regctl    : 1
+ptpls     : 0
+regctlext[0] :
+  cntlid     : ffff
+  rcsts      : 1
+  rkey       : 4
+
+Preempt
+NVME Reservation Acquire success
+
+NVME Reservation status:
+
+gen       : 5
+rtype     : 2
+regctl    : 1
+ptpls     : 0
+regctlext[0] :
+  cntlid     : ffff
+  rcsts      : 1
+  rkey       : 4
+
+Release
+NVME Reservation Release success
+
+NVME Reservation status:
+
+gen       : 5
+rtype     : 0
+regctl    : 1
+ptpls     : 0
+regctlext[0] :
+  cntlid     : ffff
+  rcsts      : 0
+  rkey       : 4
+
+Clear
+NVME Reservation  success
+NVME Reservation Acquire success
+
+NVME Reservation status:
+
+gen       : 6
+rtype     : 1
+regctl    : 1
+ptpls     : 0
+regctlext[0] :
+  cntlid     : ffff
+  rcsts      : 1
+  rkey       : 4
+
+NVME Reservation Release success
+disconnected 1 controller(s)
+Test complete
-- 
2.43.0

Re: [PATCH blktests v3 1/2] nvme/{md/001,rc,002,016,017,030,052}: introduce --resv_enable argument

[Chaitanya Kulkarni]

On 10/12/24 04:11, Guixin Liu wrote:
> Add an optional argument --resv_enable to _nvmet_target_setup() and
> propagate it to _create_nvmet_subsystem() and _create_nvmet_ns().
>
> One can call functions with --resv_enable to enable reservation
> feature on a specific namespace.
>
> And also make _create_nvmet_ns and _create_nvmet_subsystem to parse
> for arguments, this makes these functions more flexible to use.
>
> Signed-off-by: Guixin Liu<kanie@linux.alibaba.com>

Thanks for the test, looks good.

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

-ck

Re: [PATCH blktests v3 2/2] nvme: test the nvme reservation feature

[Chaitanya Kulkarni]

On 10/12/24 04:11, Guixin Liu wrote:
> Test the NVMe reservation feature, including register, acquire,
> release and report.
>
> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
> ---
>   tests/nvme/054     |  99 +++++++++++++++++++++++++++++++++++++++++
>   tests/nvme/054.out | 108 +++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 207 insertions(+)
>   create mode 100644 tests/nvme/054
>   create mode 100644 tests/nvme/054.out
>
> diff --git a/tests/nvme/054 b/tests/nvme/054
> new file mode 100644
> index 0000000..f352c73
> --- /dev/null
> +++ b/tests/nvme/054
> @@ -0,0 +1,99 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright (C) 2024 Guixin Liu
> +# Copyright (C) 2024 Alibaba Group.
> +#
> +# Test the NVMe reservation feature
> +#
> +. tests/nvme/rc
> +
> +DESCRIPTION="Test the NVMe reservation feature"
> +QUICK=1
> +nvme_trtype="loop"
> +
> +requires() {
> +	_nvme_requires
> +}
> +
> +resv_report() {
> +	local nvmedev=$1
> +	local report_arg=$2
> +
> +	nvme resv-report "/dev/${nvmedev}n1" "${report_arg}" | grep -v "hostid"
> +}
> +
> +test_resv() {
> +	local nvmedev=$1
> +	local report_arg="--cdw11=1"
> +
> +	if nvme resv-report --help 2>&1 | grep -- '--eds' > /dev/null; then
> +		report_arg="--eds"
> +	fi
> +
> +	echo "Register"
> +	resv_report "${nvmedev}" "${report_arg}"
> +	nvme resv-register "/dev/${nvmedev}n1" --nrkey=4 --rrega=0
> +	resv_report "${nvmedev}" "${report_arg}"
> +
> +	echo "Replace"
> +	nvme resv-register "/dev/${nvmedev}n1" --crkey=4 --nrkey=5 --rrega=2
> +	resv_report "${nvmedev}" "${report_arg}"
> +
> +	echo "Unregister"
> +	nvme resv-register "/dev/${nvmedev}n1" --crkey=5 --rrega=1
> +	resv_report "${nvmedev}" "${report_arg}"
> +
> +	echo "Acquire"
> +	nvme resv-register "/dev/${nvmedev}n1" --nrkey=4 --rrega=0
> +	nvme resv-acquire "/dev/${nvmedev}n1" --crkey=4 --rtype=1 --racqa=0
> +	resv_report "${nvmedev}" "${report_arg}"
> +
> +	echo "Preempt"
> +	nvme resv-acquire "/dev/${nvmedev}n1" --crkey=4 --rtype=2 --racqa=1
> +	resv_report "${nvmedev}" "${report_arg}"
> +
> +	echo "Release"
> +	nvme resv-release "/dev/${nvmedev}n1" --crkey=4 --rtype=2 --rrela=0
> +	resv_report "${nvmedev}" "${report_arg}"
> +
> +	echo "Clear"
> +	nvme resv-register "/dev/${nvmedev}n1" --nrkey=4 --rrega=0
> +	nvme resv-acquire "/dev/${nvmedev}n1" --crkey=4 --rtype=1 --racqa=0
> +	resv_report "${nvmedev}" "${report_arg}"
> +	nvme resv-release "/dev/${nvmedev}n1" --crkey=4 --rrela=1
> +}
> +
> +

make it easier to debug totally untested :-

test_resv() {
         local nvmedev=$1
         local report_arg="--cdw11=1"
         test_dev="/dev/${nvmedev}n1"

         if nvme resv-report --help 2>&1 | grep -- '--eds' > /dev/null; then
                 report_arg="--eds"
         fi

         echo "Register"
         resv_report "${nvmedev}" "${report_arg}"
         nvme resv-register "${test_dev}" --nrkey=4 --rrega=0
         resv_report "${nvmedev}" "${report_arg}"

         echo "Replace"
         nvme resv-register "${test_dev}" --crkey=4 --nrkey=5 --rrega=2
         resv_report "${nvmedev}" "${report_arg}"

         echo "Unregister"
         nvme resv-register "${test_dev}" --crkey=5 --rrega=1
         resv_report "${nvmedev}" "${report_arg}"

         echo "Acquire"
         nvme resv-register "${test_dev}" --nrkey=4 --rrega=0
         nvme resv-acquire "${test_dev}" --crkey=4 --rtype=1 --racqa=0
         resv_report "${nvmedev}" "${report_arg}"

         echo "Preempt"
         nvme resv-acquire "${test_dev}" --crkey=4 --rtype=2 --racqa=1
         resv_report "${nvmedev}" "${report_arg}"

         echo "Release"
         nvme resv-release "${test_dev}" --crkey=4 --rtype=2 --rrela=0
         resv_report "${nvmedev}" "${report_arg}"

         echo "Clear"
         nvme resv-register "${test_dev}" --nrkey=4 --rrega=0
         nvme resv-acquire "${test_dev}" --crkey=4 --rtype=1 --racqa=0
         resv_report "${nvmedev}" "${report_arg}"
         nvme resv-release "${test_dev}" --crkey=4 --rrela=1
}


irrespective of that looks good :-

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

-ck

Re: [PATCH blktests v3 1/2] nvme/{md/001,rc,002,016,017,030,052}: introduce --resv_enable argument

[Daniel Wagner]

On Sat, Oct 12, 2024 at 07:11:56PM GMT, Guixin Liu wrote:
> Add an optional argument --resv_enable to _nvmet_target_setup() and
> propagate it to _create_nvmet_subsystem() and _create_nvmet_ns().
> 
> One can call functions with --resv_enable to enable reservation
> feature on a specific namespace.
> 
> And also make _create_nvmet_ns and _create_nvmet_subsystem to parse
> for arguments, this makes these functions more flexible to use.
> 
> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>

Reviewed-by: Daniel Wagner <dwagner@suse.de>

Re: [PATCH blktests v3 2/2] nvme: test the nvme reservation feature

[Daniel Wagner]

On Sat, Oct 12, 2024 at 07:11:57PM GMT, Guixin Liu wrote:
> +requires() {
> +	_nvme_requires
> +}

It's probably a good idea to test if the reservation feature is
available, otherwise this test just fails on older kernels.

> +Running nvme/054
> +Register
> +
> +NVME Reservation status:
> +
> +gen       : 0
> +rtype     : 0
> +regctl    : 0
> +ptpls     : 0
> +
> +NVME Reservation  success

I think this is a bit fragile, because the output of the nvme command
might be extended or reformatted (obviously not on purpose but happens).
I'd recommend to explicitly check for the expected values in the test.

Re: [PATCH blktests v3 2/2] nvme: test the nvme reservation feature

[Guixin Liu]

在 2024/10/14 14:43, Chaitanya Kulkarni 写道:
> On 10/12/24 04:11, Guixin Liu wrote:
>> Test the NVMe reservation feature, including register, acquire,
>> release and report.
>>
>> Signed-off-by: Guixin Liu <kanie@linux.alibaba.com>
>> ---
>>    tests/nvme/054     |  99 +++++++++++++++++++++++++++++++++++++++++
>>    tests/nvme/054.out | 108 +++++++++++++++++++++++++++++++++++++++++++++
>>    2 files changed, 207 insertions(+)
>>    create mode 100644 tests/nvme/054
>>    create mode 100644 tests/nvme/054.out
>>
>> diff --git a/tests/nvme/054 b/tests/nvme/054
>> new file mode 100644
>> index 0000000..f352c73
>> --- /dev/null
>> +++ b/tests/nvme/054
>> @@ -0,0 +1,99 @@
>> +#!/bin/bash
>> +# SPDX-License-Identifier: GPL-3.0+
>> +# Copyright (C) 2024 Guixin Liu
>> +# Copyright (C) 2024 Alibaba Group.
>> +#
>> +# Test the NVMe reservation feature
>> +#
>> +. tests/nvme/rc
>> +
>> +DESCRIPTION="Test the NVMe reservation feature"
>> +QUICK=1
>> +nvme_trtype="loop"
>> +
>> +requires() {
>> +	_nvme_requires
>> +}
>> +
>> +resv_report() {
>> +	local nvmedev=$1
>> +	local report_arg=$2
>> +
>> +	nvme resv-report "/dev/${nvmedev}n1" "${report_arg}" | grep -v "hostid"
>> +}
>> +
>> +test_resv() {
>> +	local nvmedev=$1
>> +	local report_arg="--cdw11=1"
>> +
>> +	if nvme resv-report --help 2>&1 | grep -- '--eds' > /dev/null; then
>> +		report_arg="--eds"
>> +	fi
>> +
>> +	echo "Register"
>> +	resv_report "${nvmedev}" "${report_arg}"
>> +	nvme resv-register "/dev/${nvmedev}n1" --nrkey=4 --rrega=0
>> +	resv_report "${nvmedev}" "${report_arg}"
>> +
>> +	echo "Replace"
>> +	nvme resv-register "/dev/${nvmedev}n1" --crkey=4 --nrkey=5 --rrega=2
>> +	resv_report "${nvmedev}" "${report_arg}"
>> +
>> +	echo "Unregister"
>> +	nvme resv-register "/dev/${nvmedev}n1" --crkey=5 --rrega=1
>> +	resv_report "${nvmedev}" "${report_arg}"
>> +
>> +	echo "Acquire"
>> +	nvme resv-register "/dev/${nvmedev}n1" --nrkey=4 --rrega=0
>> +	nvme resv-acquire "/dev/${nvmedev}n1" --crkey=4 --rtype=1 --racqa=0
>> +	resv_report "${nvmedev}" "${report_arg}"
>> +
>> +	echo "Preempt"
>> +	nvme resv-acquire "/dev/${nvmedev}n1" --crkey=4 --rtype=2 --racqa=1
>> +	resv_report "${nvmedev}" "${report_arg}"
>> +
>> +	echo "Release"
>> +	nvme resv-release "/dev/${nvmedev}n1" --crkey=4 --rtype=2 --rrela=0
>> +	resv_report "${nvmedev}" "${report_arg}"
>> +
>> +	echo "Clear"
>> +	nvme resv-register "/dev/${nvmedev}n1" --nrkey=4 --rrega=0
>> +	nvme resv-acquire "/dev/${nvmedev}n1" --crkey=4 --rtype=1 --racqa=0
>> +	resv_report "${nvmedev}" "${report_arg}"
>> +	nvme resv-release "/dev/${nvmedev}n1" --crkey=4 --rrela=1
>> +}
>> +
>> +
> make it easier to debug totally untested :-
>
> test_resv() {
>           local nvmedev=$1
>           local report_arg="--cdw11=1"
>           test_dev="/dev/${nvmedev}n1"
>
>           if nvme resv-report --help 2>&1 | grep -- '--eds' > /dev/null; then
>                   report_arg="--eds"
>           fi
>
>           echo "Register"
>           resv_report "${nvmedev}" "${report_arg}"
>           nvme resv-register "${test_dev}" --nrkey=4 --rrega=0
>           resv_report "${nvmedev}" "${report_arg}"
>
>           echo "Replace"
>           nvme resv-register "${test_dev}" --crkey=4 --nrkey=5 --rrega=2
>           resv_report "${nvmedev}" "${report_arg}"
>
>           echo "Unregister"
>           nvme resv-register "${test_dev}" --crkey=5 --rrega=1
>           resv_report "${nvmedev}" "${report_arg}"
>
>           echo "Acquire"
>           nvme resv-register "${test_dev}" --nrkey=4 --rrega=0
>           nvme resv-acquire "${test_dev}" --crkey=4 --rtype=1 --racqa=0
>           resv_report "${nvmedev}" "${report_arg}"
>
>           echo "Preempt"
>           nvme resv-acquire "${test_dev}" --crkey=4 --rtype=2 --racqa=1
>           resv_report "${nvmedev}" "${report_arg}"
>
>           echo "Release"
>           nvme resv-release "${test_dev}" --crkey=4 --rtype=2 --rrela=0
>           resv_report "${nvmedev}" "${report_arg}"
>
>           echo "Clear"
>           nvme resv-register "${test_dev}" --nrkey=4 --rrega=0
>           nvme resv-acquire "${test_dev}" --crkey=4 --rtype=1 --racqa=0
>           resv_report "${nvmedev}" "${report_arg}"
>           nvme resv-release "${test_dev}" --crkey=4 --rrela=1
> }
>
Thanks, changed in v4, and also change resv_report()'s firt param to

test_dev instead of nvmedev.

Best Regards,

Guixin Liu

> irrespective of that looks good :-
>
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
>
> -ck
>
>

Re: [PATCH blktests v3 2/2] nvme: test the nvme reservation feature

[Guixin Liu]

在 2024/10/14 15:23, Daniel Wagner 写道:
> On Sat, Oct 12, 2024 at 07:11:57PM GMT, Guixin Liu wrote:
>> +requires() {
>> +	_nvme_requires
>> +}
> It's probably a good idea to test if the reservation feature is
> available, otherwise this test just fails on older kernels.

I checked the exists of resv_enable,

"if [[ -f "${ns_path}/resv_enable" ]] ; then", if not exist, skip this

test case.

>> +Running nvme/054
>> +Register
>> +
>> +NVME Reservation status:
>> +
>> +gen       : 0
>> +rtype     : 0
>> +regctl    : 0
>> +ptpls     : 0
>> +
>> +NVME Reservation  success
> I think this is a bit fragile, because the output of the nvme command
> might be extended or reformatted (obviously not on purpose but happens).
> I'd recommend to explicitly check for the expected values in the test.

OK, I will add a "grep -E "xxx|xxx|xxx" " to filter the result we cared now.

Best Regards,

Guixin Liu