[PATCH blktests] test/nvme/050: test the reservation feature

Guixin Liu kanie at linux.alibaba.com
Mon Jan 15 17:57:22 PST 2024


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@linux.alibaba.com/
>
> 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 at linux.alibaba.com>
>> ---
>>   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)
>>



More information about the Linux-nvme mailing list