Hi, Shinichiro:
Thanks for the patient reply.
在 2024/1/15 13:15, Shinichiro Kawasaki 写道:
Hello Guixin, thanks for the patch. Good to have new tests for new features :)
I think this test depends on the kernel side patches you posted [1]. So the test
case should be skipped when the kernel does not have the patches. I suggest to
check it using "resv_enable" configfs file. Please refer to nvme/048 which does
similar check and sets SKIP_REASONS.
Agree, it will be changed in v2.
[1] https://lore.kernel.org/linux-nvme/20240114092314.63694-1-kanie@xxxxxxxxxxxxxxxxx/
When I ran the test case on my system using the kernel v6.7 + the [1] patches,
I observed the kernel BUG below. It looks kmalloc() in nvmet_execute_pr_report()
needs care.
OK, I will take a look.
[ 262.402130] run blktests nvme/050 at 2024-01-15 13:21:44
[ 262.465068] nvmet: adding nsid 1 to subsystem blktests-subsystem-1
[ 262.520959] nvmet: creating nvm controller 1 for subsystem blktests-subsystem-1 for NQN nqn.2014-08.org.nvmexpress:uuid:0f01fb42-9f7f-4856-b0b3-51e60b8de349.
[ 262.525398] nvme nvme1: Please enable CONFIG_NVME_MULTIPATH for full support of multi-port devices.
[ 262.528334] nvme nvme1: creating 4 I/O queues.
[ 262.531269] nvme nvme1: new ctrl: "blktests-subsystem-1"
[ 262.655310] BUG: sleeping function called from invalid context at include/linux/sched/mm.h:306
[ 262.657489] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 233, name: kworker/1:3
[ 262.658976] preempt_count: 1, expected: 0
[ 262.660158] RCU nest depth: 0, expected: 0
[ 262.661372] 4 locks held by kworker/1:3/233:
[ 262.662526] #0: ffff888135f04538 ((wq_completion)nvmet-wq){+.+.}-{0:0}, at: process_one_work+0x679/0x1260
[ 262.664235] #1: ffff88812524fd90 ((work_completion)(&iod->work)){+.+.}-{0:0}, at: process_one_work+0x6da/0x1260
[ 262.665963] #2: ffff888133caa870 (&subsys->lock#2){+.+.}-{3:3}, at: nvmet_execute_pr_report+0x197/0xbd0 [nvmet]
[ 262.667622] #3: ffff888136cdd230 (&ns->pr.pr_lock){.+.+}-{2:2}, at: nvmet_execute_pr_report+0x1b4/0xbd0 [nvmet]
[ 262.669369] Preemption disabled at:
[ 262.669371] [<0000000000000000>] 0x0
[ 262.671748] CPU: 1 PID: 233 Comm: kworker/1:3 Tainted: G W 6.7.0+ #141
[ 262.673226] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-1.fc39 04/01/2014
[ 262.674789] Workqueue: nvmet-wq nvme_loop_execute_work [nvme_loop]
[ 262.676142] Call Trace:
[ 262.677234] <TASK>
[ 262.678241] dump_stack_lvl+0x71/0x90
[ 262.679336] __might_resched+0x3d5/0x5f0
[ 262.680482] ? __pfx___might_resched+0x10/0x10
[ 262.681675] __kmem_cache_alloc_node+0x2ef/0x340
[ 262.682960] ? nvmet_execute_pr_report+0x313/0xbd0 [nvmet]
[ 262.684275] ? nvmet_execute_pr_report+0x313/0xbd0 [nvmet]
[ 262.685638] __kmalloc+0x50/0x160
[ 262.686779] nvmet_execute_pr_report+0x313/0xbd0 [nvmet]
[ 262.688040] ? lock_is_held_type+0xce/0x120
[ 262.689221] process_one_work+0x74c/0x1260
[ 262.690480] ? __pfx_lock_acquire+0x10/0x10
[ 262.691644] ? __pfx_process_one_work+0x10/0x10
[ 262.692893] ? assign_work+0x16c/0x240
[ 262.694063] worker_thread+0x723/0x1300
[ 262.695216] ? lockdep_hardirqs_on+0x7d/0x100
[ 262.696461] ? __kthread_parkme+0xbd/0x1f0
[ 262.697587] ? __pfx_worker_thread+0x10/0x10
[ 262.698812] kthread+0x2f1/0x3d0
[ 262.700000] ? _raw_spin_unlock_irq+0x24/0x50
[ 262.701179] ? __pfx_kthread+0x10/0x10
[ 262.702340] ret_from_fork+0x30/0x70
[ 262.703427] ? __pfx_kthread+0x10/0x10
[ 262.704560] ret_from_fork_asm+0x1b/0x30
[ 262.705718] </TASK>
[ 262.891548] nvme nvme1: Removing ctrl: NQN "blktests-subsystem-1"
Also please find my comments in line.
On Jan 14, 2024 / 17:26, Guixin Liu wrote:
Test the reservation feature, includes register, acquire, release
and report.
Signed-off-by: Guixin Liu <kanie@xxxxxxxxxxxxxxxxx>
---
tests/nvme/050 | 67 ++++++++++++++++++++++++++
tests/nvme/050.out | 114 +++++++++++++++++++++++++++++++++++++++++++++
tests/nvme/rc | 3 ++
3 files changed, 184 insertions(+)
create mode 100644 tests/nvme/050
create mode 100644 tests/nvme/050.out
diff --git a/tests/nvme/050 b/tests/nvme/050
new file mode 100644
index 0000000..a499f66
--- /dev/null
+++ b/tests/nvme/050
@@ -0,0 +1,67 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2024 Guixin Liu
+# Copyright (C) 2024 Alibaba Group.
+#
+# Test the reservation
Nit: looks too short. How about "Test the NVMe reservaction feature"?
OK.
+#
+. tests/nvme/rc
+
+DESCRIPTION="test the reservation"
Nit: How about "test the reservation feature"?
Same sure.
+QUICK=1
+
+requires() {
+ _nvme_requires
+}
+
+test() {
+ echo "Running ${TEST_NAME}"
+
+ _setup_nvmet
+
+ local nvmedev
+
+ _nvmet_target_setup --blkdev file
+
+ _nvme_connect_subsys "${nvme_trtype}" "${def_subsysnqn}"
+
+ nvmedev=$(_find_nvme_dev "${def_subsysnqn}")
+
+ echo "Register"
+ nvme resv-report "/dev/${nvmedev}n1" --cdw11=1
Recent nvme-cli, probably since v2.0 in Apr/2022, replaced this --cdw11 option
of the "nvme resv-report" command with --eds option. To allow this test case
run regardless of the nvme-cli version, I suggest to check "nvme resv-report"
command output at the beginning of the test case and see if it contains the
string "--eds". Depending on the check result, we can change the option for the
"nvme resv-report" commands in this test case.
My nvme-cli is v1.16, I will update it to the newest version to take a look.
+ nvme resv-register "/dev/${nvmedev}n1" --nrkey=4 --rrega=0
+ nvme resv-report "/dev/${nvmedev}n1" --cdw11=1
+
+ echo "Replace"
+ nvme resv-register "/dev/${nvmedev}n1" --crkey=4 --nrkey=5 --rrega=2
+ nvme resv-report "/dev/${nvmedev}n1" --cdw11=1
+
+ echo "Unregister"
+ nvme resv-register "/dev/${nvmedev}n1" --crkey=5 --rrega=1
+ nvme resv-report "/dev/${nvmedev}n1" --cdw11=1
+
+ echo "Acquire"
+ nvme resv-register "/dev/${nvmedev}n1" --nrkey=4 --rrega=0
+ nvme resv-acquire "/dev/${nvmedev}n1" --crkey=4 --rtype=1 --racqa=0
+ nvme resv-report "/dev/${nvmedev}n1" --cdw11=1
+
+ echo "Preempt"
+ nvme resv-acquire "/dev/${nvmedev}n1" --crkey=4 --rtype=2 --racqa=1
+ nvme resv-report "/dev/${nvmedev}n1" --cdw11=1
+
+ echo "Release"
+ nvme resv-release "/dev/${nvmedev}n1" --crkey=4 --rtype=2 --rrela=0
+ nvme resv-report "/dev/${nvmedev}n1" --cdw11=1
+
+ echo "Clear"
+ nvme resv-register "/dev/${nvmedev}n1" --nrkey=4 --rrega=0
+ nvme resv-acquire "/dev/${nvmedev}n1" --crkey=4 --rtype=1 --racqa=0
+ nvme resv-report "/dev/${nvmedev}n1" --cdw11=1
+ nvme resv-release "/dev/${nvmedev}n1" --crkey=4 --rrela=1
+
+ _nvme_disconnect_subsys "${def_subsysnqn}"
+
+ _nvmet_target_cleanup
+
+ echo "Test complete"
+}
diff --git a/tests/nvme/050.out b/tests/nvme/050.out
new file mode 100644
index 0000000..3be417d
--- /dev/null
+++ b/tests/nvme/050.out
@@ -0,0 +1,114 @@
+Running nvme/050
+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 : 1
+ rcsts : 0
+ rkey : 4
+ hostid : f1fb429f7f4856b0b351e6b8de349
When I ran the test case on my system, it failed because of hostid mismatch.
Actually, we are trying to make the nvme test cases independent from hostid. To
not check the hostid lines for this test case, I suggest to add
"| grep -v hostid" to the "nvme resv-report" commands.
OK, I miss that def_hostid is not same for different system.
+
+Replace
+NVME Reservation success
+
+NVME Reservation status:
+
+gen : 2
+rtype : 0
+regctl : 1
+ptpls : 0
+regctlext[0] :
+ cntlid : 1
+ rcsts : 0
+ rkey : 5
+ hostid : f1fb429f7f4856b0b351e6b8de349
+
+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 : 1
+ rcsts : 1
+ rkey : 4
+ hostid : f1fb429f7f4856b0b351e6b8de349
+
+Preempt
+NVME Reservation Acquire success
+
+NVME Reservation status:
+
+gen : 5
+rtype : 2
+regctl : 1
+ptpls : 0
+regctlext[0] :
+ cntlid : 1
+ rcsts : 1
+ rkey : 4
+ hostid : f1fb429f7f4856b0b351e6b8de349
+
+Release
+NVME Reservation Release success
+
+NVME Reservation status:
+
+gen : 5
+rtype : 0
+regctl : 1
+ptpls : 0
+regctlext[0] :
+ cntlid : 1
+ rcsts : 0
+ rkey : 4
+ hostid : f1fb429f7f4856b0b351e6b8de349
+
+Clear
+NVME Reservation success
+NVME Reservation Acquire success
+
+NVME Reservation status:
+
+gen : 6
+rtype : 1
+regctl : 1
+ptpls : 0
+regctlext[0] :
+ cntlid : 1
+ rcsts : 1
+ rkey : 4
+ hostid : f1fb429f7f4856b0b351e6b8de349
+
+NVME Reservation Release success
+disconnected 1 controller(s)
+Test complete
diff --git a/tests/nvme/rc b/tests/nvme/rc
index b0ef1ee..8de59e2 100644
--- a/tests/nvme/rc
+++ b/tests/nvme/rc
@@ -670,6 +670,9 @@ _create_nvmet_ns() {
mkdir "${ns_path}"
printf "%s" "${blkdev}" > "${ns_path}/device_path"
printf "%s" "${uuid}" > "${ns_path}/device_uuid"
+ if [[ -f "${ns_path}/resv_enable" ]]; then
+ printf 1 > "${ns_path}/resv_enable"
+ fi
I think this operation is required only for this test case, so resv_enable
should not be set for other test cases. So, it is the better to add an optional
argument like "--resv_enable" to _nvmet_target_setup() and propagate it to
_create_nvmet_subsystem() and _create_nvmet_ns(). It's the better to make
it a separated patch.
Yeah, sure, it will be changes in v2.
printf 1 > "${ns_path}/enable"
}
--
2.30.1 (Apple Git-130)
