Re: [PATCH blktests] loop: add test for creating/deleting file-ns

[Nilay Shroff <nilay@linux.ibm.com>]

On 6/18/24 14:58, Shinichiro Kawasaki wrote:
> On Jun 18, 2024 / 01:35, Chaitanya Kulkarni wrote:
>> On 6/17/24 02:17, Nilay Shroff wrote:
>>
>> I think subject line should start with nvme ?
>>
>> nvme: add test for creating/deleting file-ns
>>
>>> This is regression test for commit be647e2c76b2
>>> (nvme: use srcu for iterating namespace list)
>>>
>>> This test uses a regulare file backed loop device
>>> for creating and then deleting an NVMe namespace
>>> in a loop.
>>
>>
>> s/regulare/regular/ ?
>>
>> nit:- commit log looks a bit short :-
>>
>> This is regression test for commit be647e2c76b2
>> (nvme: use srcu for iterating namespace list)
>>
>> This test uses a regulare file backed loop device for creating and
>> then deleting an NVMe namespace in a loop.
>>
>>> Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
>>> ---
>>> This regression was first reported[1], and now it's
>>> fixed in 6.10-rc4[2]
>>>
>>> [1] https://lore.kernel.org/all/2312e6c3-a069-4388-a863-df7e261b9d70@linux.vnet.ibm.com/
>>> [2] commit ff0ffe5b7c3c (nvme: fix namespace removal list)
>>
>> it will be helpful in long run to add above information
>> into the commit log, Shinichiro any thoughts ?
> 
> Agreed. It is helpful to record the kernel side fix commit and the link to the
> discussion threads in the blktests side commit log.
Yeah I am going to updated the commit message with relevant information as suggested
in the next patch.
> 
>>
>>> ---
>>>   tests/nvme/051     | 65 ++++++++++++++++++++++++++++++++++++++++++++++
>>>   tests/nvme/051.out |  2 ++
>>>   2 files changed, 67 insertions(+)
>>>   create mode 100755 tests/nvme/051
>>>   create mode 100644 tests/nvme/051.out
>>>
>>> diff --git a/tests/nvme/051 b/tests/nvme/051
>>> new file mode 100755
>>> index 0000000..0de5c56
>>> --- /dev/null
>>> +++ b/tests/nvme/051
>>> @@ -0,0 +1,65 @@
>>> +#!/bin/bash
>>> +# SPDX-License-Identifier: GPL-3.0+
>>> +# Copyright (C) 2024 Nilay Shroff(nilay@linux.ibm.com)
>>
>> not sure we need to have email address here as it's a part of
>> commit log anyways ...
>>
>>> +#
>>> +# Regression test for commit be647e2c76b2(nvme: use srcu for iterating
>>> +# namespace list)
> 
> It is also good to enrich this header comment section. Especially, the kernel
> side fix commit will be helpful.
> 
>>> +
>>> +. tests/nvme/rc
>>> +
>>> +DESCRIPTION="Test file-ns creation/deletion under one subsystem"
>>> +
>>> +requires() {
>>> +	_nvme_requires
>>> +	_have_loop
>>> +	_require_nvme_trtype_is_loop
>>> +}
>>> +
>>> +set_conditions() {
>>> +	_set_nvme_trtype "$@"
>>> +}
>>> +
>>> +test() {
>>> +	echo "Running ${TEST_NAME}"
>>> +
>>> +	_setup_nvmet
>>> +
>>> +	local subsys="blktests-subsystem-1"
>>> +	local iterations="${NVME_NUM_ITER}"
>>
>> no need for above var, I think direct use of NVME_NUM_ITER is
>> clear here ...
> 
> I ran this test case on my baremetal test node and QEMU test node using the
> kernel without the fix. On the baremetal test node, the kernel Oops was
> created soon. Good. On the QEMU test node, the Oops was not recreated, and
> the test case passed. For this pass case, it took more than 15 minutes to
> complete the test case. I think the default NVME_NUM_ITER=1000 is too much.
> Can we reduce it to 10 or 20?
> 
I also tested this case on my baremetal machine and I could recreate this crash 
on the first iteration. However I didn't test it on QEMU. But agrees the default 
iteration value of 1000 is too big and I think it's reasonable to make it 20. I 
will change it in the next patch.
>>
>>> +	local loop_dev
>>> +	local port
>>> +
>>> +	truncate -s "${NVME_IMG_SIZE}" "$(_nvme_def_file_path)"
>>> +
>>> +	loop_dev="$(losetup -f --show "$(_nvme_def_file_path)")"
>>> +
>>> +	port="$(_create_nvmet_port "${nvme_trtype}")"
>>> +
>>> +	_nvmet_target_setup --subsysnqn "${subsys}" --blkdev "${loop_dev}"
>>> +
>>> +	_nvme_connect_subsys --subsysnqn "${subsys}"
>>> +
>>> +	for ((i = 2; i <= iterations; i++)); do {
>>
>> small comment would be useful to explain why are starting at 2 ...
>>
>>> +		truncate -s "${NVME_IMG_SIZE}" "$(_nvme_def_file_path).$i"
>>> +		_create_nvmet_ns "${subsys}" "${i}" "$(_nvme_def_file_path).$i"
>>> +
>>> +		# allow async request to be processed
>>> +		sleep 1
>>> +
>>> +		_remove_nvmet_ns "${subsys}" "${i}"
>>> +		rm "$(_nvme_def_file_path).$i"
>>> +	}
>>> +	done
>>> +
>>> +	_nvme_disconnect_subsys --subsysnqn "${subsys}" >> "${FULL}" 2>&1
>>> +
>>> +	_nvmet_target_cleanup --subsysnqn "${subsys}" --blkdev "${loop_dev}"
>>> +
>>> +	_remove_nvmet_port "${port}"
>>> +
>>> +	losetup -d "$loop_dev"
>>> +
>>> +	rm "$(_nvme_def_file_path)"
>>> +
>>> +	echo "Test complete"
>>> +}
>>> diff --git a/tests/nvme/051.out b/tests/nvme/051.out
>>> new file mode 100644
>>> index 0000000..156f068
>>> --- /dev/null
>>> +++ b/tests/nvme/051.out
>>> @@ -0,0 +1,2 @@
>>> +Running nvme/051
>>> +Test complete
>>
>> thanks for the test, I think this is much needed test especially with
>> recent reported issues ...
> 
> I also appreciate this patch. Thanks!
> 
> One more request, recent commit added a test case to the nvme group and it has
> the number nvme/051. Could you renumber this test case to nvme/052?

Yes I will rebase my tree to the latest and update the test case to nvme/052.

And thank you for testing my test case on your machine and your review comments!
Much appreciated.... 

I will soon post the patch v2.

Thanks,
--Nilay