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

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

[Nilay Shroff]

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)
---
 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)
+#
+# Regression test for commit be647e2c76b2(nvme: use srcu for iterating
+# namespace list)
+
+. 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}"
+	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 {
+		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
-- 
2.45.1

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

[Chaitanya Kulkarni]

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 ?

> ---
>   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)
> +
> +. 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 ...

> +	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 ...

-ck

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

[Shinichiro Kawasaki]

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.

> 
> > ---
> >   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?

> 
> > +	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?

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

[Nilay Shroff]

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

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

[Nilay Shroff]

On 6/18/24 07:05, 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
Ok will do.

> 
>> 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/ ?
yes will correct the spelling.
> 
> 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 ?
> 
yes I will add more information in the commit log.
>> ---
>>   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 ...
> 
I saw in some test cases email address was added and so I also 
included mine. But anyways, I will remove the email address as 
you suggested.
>> +#
>> +# Regression test for commit be647e2c76b2(nvme: use srcu for iterating
>> +# namespace list)
>> +
>> +. 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 ...
Alright, accepted. 
> 
>> +	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 ...
Yes I will add the relevant comment.
> 
>> +		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 ...
> 
Thank you for your review and comments! Much appreciated...
I will send v2 with above comments incorporated.

Thanks,
--Nilay

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

[Daniel Wagner]

On Mon, Jun 17, 2024 at 02:47:22PM GMT, Nilay Shroff wrote:
> +test() {
> +	echo "Running ${TEST_NAME}"
> +
> +	_setup_nvmet
> +
> +	local subsys="blktests-subsystem-1"

Use the default variables instead of duplicating them.

> +	local iterations="${NVME_NUM_ITER}"
> +	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}"

As far I can tell this could just be:

	_nvmet_target_setup

	_nvme_connect_subsys

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

[Nilay Shroff]

On 6/19/24 13:20, Daniel Wagner wrote:
> On Mon, Jun 17, 2024 at 02:47:22PM GMT, Nilay Shroff wrote:
>> +test() {
>> +	echo "Running ${TEST_NAME}"
>> +
>> +	_setup_nvmet
>> +
>> +	local subsys="blktests-subsystem-1"
> 
> Use the default variables instead of duplicating them.
Yes that makes sense.
> 
>> +	local iterations="${NVME_NUM_ITER}"
>> +	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}"
> 
> As far I can tell this could just be:
> 
> 	_nvmet_target_setup
> 
> 	_nvme_connect_subsys
> 
I updated test case using default subsysnqn and blkdev that works as expected. 
So yes I can use defaults for setting up and then connecting to nvme target.

I will update this in next patch.

Appreciate your review and suggestion... 

Thanks,
--Nilay