[PATCHv2 blktests 0/2] add nvme test for creating sleep while atomic kernel BUG

[PATCHv2 blktests 0/2] add nvme test for creating sleep while atomic kernel BUG

[Nilay Shroff]

Hi,

There're two patches in this series. The first patch is a preparation patch
for reusing a common function nvmf_wait_for_ns from multiple nvme test scripts.
The second patch adds a new nvme regression[1] test for commit 505363957fad
("nvmet: fix nvme status code when namespace is disabled").

For fixing regression, as suggested by Christoph[2], I would send a separate
patch upstream.

[1] https://lore.kernel.org/linux-nvme/tqcy3sveity7p56v7ywp7ssyviwcb3w4623cnxj3knoobfcanq@yxgt2mjkbkam/
[2] https://lore.kernel.org/linux-nvme/20241022070252.GA11389@lst.de/

Changes from v1:
    - Update the second patch in the series so that we can trigger the
      regression 100% of time when we run the test (Chaitanya Kulkarni)

Nilay Shroff (2):
  nvme/052: move nvmf_wait_for_ns() to common/nvme
  nvme: add test for writing to file-ns just after disabling it

 common/nvme        |  26 ++++++++++
 tests/nvme/052     |  30 +-----------
 tests/nvme/055     | 117 +++++++++++++++++++++++++++++++++++++++++++++
 tests/nvme/055.out |   2 +
 4 files changed, 147 insertions(+), 28 deletions(-)
 create mode 100755 tests/nvme/055
 create mode 100644 tests/nvme/055.out

-- 
2.45.2

[PATCHv2 blktests 1/2] nvme/052: move nvmf_wait_for_ns() to common/nvme

[Nilay Shroff]

In a preparation for the use of nvmf_wait_for_ns function from
other nvme test script, move nvmf_wait_for_ns from nvme/052 to
common/nvme file so that we can reuse the nvmf_wait_for_ns from
multiple test scripts.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
---
 common/nvme    | 26 ++++++++++++++++++++++++++
 tests/nvme/052 | 30 ++----------------------------
 2 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/common/nvme b/common/nvme
index fd472fe..6b8a57a 100644
--- a/common/nvme
+++ b/common/nvme
@@ -483,6 +483,32 @@ _remove_nvmet_port() {
 	rmdir "${NVMET_CFS}/ports/${port}"
 }
 
+# Wait for the namespace with specified uuid to fulfill the specified condtion,
+# "created" or "removed".
+_nvmf_wait_for_ns() {
+    local ns
+    local timeout="5"
+    local uuid="$1"
+    local condition="$2"
+
+    ns=$(_find_nvme_ns "${uuid}" 2>> "$FULL")
+
+    start_time=$(date +%s)
+    while [[ -z "$ns" && "$condition" == created ]] ||
+              [[ -n "$ns" && "$condition" == removed ]]; do
+        sleep .1
+        end_time=$(date +%s)
+        if (( end_time - start_time > timeout )); then
+            echo "namespace with uuid \"${uuid}\" not " \
+                "${condition} within ${timeout} seconds"
+            return 1
+        fi
+        ns=$(_find_nvme_ns "${uuid}" 2>> "$FULL")
+    done
+
+    return 0
+}
+
 _create_nvmet_ns() {
 	local subsysnqn="${def_subsysnqn}"
 	local nsid="${def_nsid}"
diff --git a/tests/nvme/052 b/tests/nvme/052
index 8443c90..22d8a9e 100755
--- a/tests/nvme/052
+++ b/tests/nvme/052
@@ -20,32 +20,6 @@ set_conditions() {
 	_set_nvme_trtype "$@"
 }
 
-# Wait for the namespace with specified uuid to fulfill the specified condtion,
-# "created" or "removed".
-nvmf_wait_for_ns() {
-	local ns
-	local timeout="5"
-	local uuid="$1"
-	local condition="$2"
-
-	ns=$(_find_nvme_ns "${uuid}" 2>> "$FULL")
-
-	start_time=$(date +%s)
-	while [[ -z "$ns" && "$condition" == created ]] ||
-		      [[ -n "$ns" && "$condition" == removed ]]; do
-		sleep .1
-		end_time=$(date +%s)
-		if (( end_time - start_time > timeout )); then
-			echo "namespace with uuid \"${uuid}\" not " \
-				"${condition} within ${timeout} seconds"
-			return 1
-		fi
-		ns=$(_find_nvme_ns "${uuid}" 2>> "$FULL")
-	done
-
-	return 0
-}
-
 test() {
 	echo "Running ${TEST_NAME}"
 
@@ -65,7 +39,7 @@ test() {
 		uuid=$(_create_nvmet_ns --blkdev "$filepath" --nsid "${nsid}")
 
 		# wait until async request is processed and ns is created
-		if ! nvmf_wait_for_ns "${uuid}" created; then
+		if ! _nvmf_wait_for_ns "${uuid}" created; then
 			echo "FAIL"
 			rm "$filepath"
 			break
@@ -74,7 +48,7 @@ test() {
 		_remove_nvmet_ns "${def_subsysnqn}" "${nsid}"
 
 		# wait until async request is processed and ns is removed
-		if ! nvmf_wait_for_ns "${uuid}" removed; then
+		if ! _nvmf_wait_for_ns "${uuid}" removed; then
 			echo "FAIL"
 			rm "$filepath"
 			break
-- 
2.45.2

[PATCHv2 blktests 2/2] nvme: add test for writing to file-ns just after disabling it

[Nilay Shroff]

This is a regression test for commit 505363957fad ("nvmet: fix nvme
status code when namespace is disabled")[1].

This test creates a regular file backed loop target namespace, disables
the asynchronous event notification for ns-changed events and then write
to the namespace just after disabling it.

[1] https://lore.kernel.org/linux-nvme/tqcy3sveity7p56v7ywp7ssyviwcb3w4623cnxj3knoobfcanq@yxgt2mjkbkam/

Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
---
 tests/nvme/055     | 117 +++++++++++++++++++++++++++++++++++++++++++++
 tests/nvme/055.out |   2 +
 2 files changed, 119 insertions(+)
 create mode 100755 tests/nvme/055
 create mode 100644 tests/nvme/055.out

diff --git a/tests/nvme/055 b/tests/nvme/055
new file mode 100755
index 0000000..f1f19fe
--- /dev/null
+++ b/tests/nvme/055
@@ -0,0 +1,117 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-3.0+
+# Copyright (C) 2024 Nilay Shroff
+#
+# Regression test for commit 505363957fad ("nvmet: fix nvme
+# status code when namespace is disabled").
+
+. tests/nvme/rc
+
+DESCRIPTION="Test nvme write to a loop target ns just after ns is disabled"
+
+QUICK=1
+
+requires() {
+	_nvme_requires
+	_have_loop
+	_require_nvme_trtype_is_loop
+	_have_kernel_option DEBUG_ATOMIC_SLEEP
+}
+
+set_conditions() {
+	_set_nvme_trtype "$@"
+}
+
+nvmf_disable_ns_change_aen() {
+
+	local disk="$1"
+	local timeout="5"
+
+	# get async event configuration value
+	aen_conf=$(nvme get-feature "$disk" --feature-id=0xB | cut -d':' -f3)
+
+	# disable async ns change event notfication
+	disable_ns_change=$(( aen_conf & 0xFEFF ))
+
+	# stop receiving aen event for ns change from target
+	nvme set-feature "$disk" --feature-id=0xB \
+			--value="$disable_ns_change" 2>>"$FULL" 1>&2
+
+	start_time=$(date +%s)
+
+	while true; do
+		aen_conf=$(nvme get-feature "$disk" \
+			--feature-id=0xB | cut -d':' -f3)
+
+		# Validate whether ns-changed notification is disabled or not;
+		# If it's already disabled then break and return success
+		if ! (( aen_conf & 0x100 )); then
+			break
+		fi
+
+		sleep 1
+
+		end_time=$(date +%s)
+
+		if (( end_time - start_time > timeout )); then
+			echo "can't disable ns-changed async event "\
+				"notification within $timeout seconds"
+			return 1
+		fi
+	done
+
+	return 0
+}
+
+test() {
+	echo "Running ${TEST_NAME}"
+
+	local iteration=1 i=0
+
+	_setup_nvmet
+
+	_nvmet_target_setup
+
+	_nvme_connect_subsys
+
+	img="$(mktemp /tmp/blk_img_XXXXXX)"
+	dd if=/dev/urandom of="$img" bs=512 count=1 status=none
+
+	subsys_path="${NVMET_CFS}/subsystems/${def_subsysnqn}"
+	ns_path="${subsys_path}/namespaces/${def_nsid}"
+
+	while (( i < iteration )); do
+		# Wait until async request is processed and default ns
+		# is created
+		if ! _nvmf_wait_for_ns "${def_subsys_uuid}" created; then
+			echo "FAIL"
+			break
+		fi
+
+		disk="/dev/$(_find_nvme_ns "${def_subsys_uuid}")"
+
+		# Disable ns change async event notification from target. It
+		# would ensure that when we disable the target ns, the host
+		# would not receive ns removal notification from target and
+		# so from host we can attempt writing to a disabled ns.
+		if ! nvmf_disable_ns_change_aen "${disk}"; then
+			echo "FAIL"
+			break
+		fi
+
+		# disable target namespace and write to it
+		echo 0 > ${ns_path}/enable
+		nvme write --start-block=1 --block-count=0 \
+			--data-size=512 --data="$img" "$disk" 2>>"$FULL"
+
+		((i++))
+	done
+
+	rm "$img"
+
+	_nvme_disconnect_subsys >> "$FULL" 2>&1
+
+	_nvmet_target_cleanup
+
+	echo "Test complete"
+}
diff --git a/tests/nvme/055.out b/tests/nvme/055.out
new file mode 100644
index 0000000..bbefc28
--- /dev/null
+++ b/tests/nvme/055.out
@@ -0,0 +1,2 @@
+Running nvme/055
+Test complete
-- 
2.45.2

Re: [PATCHv2 blktests 2/2] nvme: add test for writing to file-ns just after disabling it

[Chaitanya Kulkarni]

> +test() {
> +	echo "Running ${TEST_NAME}"
> +
> +	local iteration=1 i=0
> +
> +	_setup_nvmet
> +
> +	_nvmet_target_setup
> +
> +	_nvme_connect_subsys
> +
> +	img="$(mktemp /tmp/blk_img_XXXXXX)"
> +	dd if=/dev/urandom of="$img" bs=512 count=1 status=none

why can't we just use the standard block device backed ns ?

> +
> +	subsys_path="${NVMET_CFS}/subsystems/${def_subsysnqn}"
> +	ns_path="${subsys_path}/namespaces/${def_nsid}"
> +
> +	while (( i < iteration )); do
> +		# Wait until async request is processed and default ns
> +		# is created
> +		if ! _nvmf_wait_for_ns "${def_subsys_uuid}" created; then
> +			echo "FAIL"
> +			break
> +		fi
> +
> +		disk="/dev/$(_find_nvme_ns "${def_subsys_uuid}")"
> +
> +		# Disable ns change async event notification from target. It
> +		# would ensure that when we disable the target ns, the host
> +		# would not receive ns removal notification from target and
> +		# so from host we can attempt writing to a disabled ns.
> +		if ! nvmf_disable_ns_change_aen "${disk}"; then
> +			echo "FAIL"
> +			break
> +		fi
> +

I'm really not sure disabling a functionality for testing
purpose gives us full coverage, for a tests I believe it is important
to have full functionality running and available so it will reflect the
real setup, e.g. for this scenario we cannot dependent on disabling AEN
since users are not disabling the AEN's every-time they disable the
namespace and ends up writing to the ns on host side triggering the BUG.

Can this be an exception for some reason ?

Shinichiro, Daniel any thoughts ?

why not something like following ? (totally untested and rough)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 1f4e9989663b..eede3a7c5594 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -5,6 +5,7 @@
   */
  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
  #include <linux/module.h>
+#include <linux/delay.h>
  #include <linux/random.h>
  #include <linux/rculist.h>
  #include <linux/pci-p2pdma.h>
@@ -18,6 +19,11 @@
  #include "nvmet.h"
  #include "debugfs.h"

+unsigned int ns_disable_error_inject;
+module_param(ns_disable_error_inject, int, 0644);
+MODULE_PARM_DESC(ns_disable_error_inject,
+                "delay xa_erase() in ns-disable path in seconds 
(default 0)");
+
  struct kmem_cache *nvmet_bvec_cache;
  struct workqueue_struct *buffered_io_wq;
  struct workqueue_struct *zbd_wq;
@@ -649,6 +655,8 @@ void nvmet_ns_disable(struct nvmet_ns *ns)
                 goto out_unlock;

         ns->enabled = false;
+       if (ns_disable_error_inject)
+               msleep(ns_disable_error_inject * 1000);
         xa_erase(&ns->subsys->namespaces, ns->nsid);
         if (ns->nsid == subsys->max_nsid)
                 subsys->max_nsid = nvmet_max_nsid(subsys);


and add load the module with modprobe ns_disable_error_inject=10 ?
or we can even mode this module as a part of ns configfs attr.

with above change following statement will block for 10 seconds.
> +		# disable target namespace and write to it
> +		echo 0 > ${ns_path}/enable
> +		nvme write --start-block=1 --block-count=0 \
> +			--data-size=512 --data="$img" "$disk" 2>>"$FULL"

-ck

Re: [PATCHv2 blktests 2/2] nvme: add test for writing to file-ns just after disabling it

[Nilay Shroff]

On 12/5/24 13:02, Chaitanya Kulkarni wrote:
> 
>> +test() {
>> +	echo "Running ${TEST_NAME}"
>> +
>> +	local iteration=1 i=0
>> +
>> +	_setup_nvmet
>> +
>> +	_nvmet_target_setup
>> +
>> +	_nvme_connect_subsys
>> +
>> +	img="$(mktemp /tmp/blk_img_XXXXXX)"
>> +	dd if=/dev/urandom of="$img" bs=512 count=1 status=none
> 
> why can't we just use the standard block device backed ns ?
Using standard block device would also work however using the file backed block 
device would be handy in case user doesn't have a spare standard block device 
available which could be used for writing a random junk to it. 
> 
>> +
>> +	subsys_path="${NVMET_CFS}/subsystems/${def_subsysnqn}"
>> +	ns_path="${subsys_path}/namespaces/${def_nsid}"
>> +
>> +	while (( i < iteration )); do
>> +		# Wait until async request is processed and default ns
>> +		# is created
>> +		if ! _nvmf_wait_for_ns "${def_subsys_uuid}" created; then
>> +			echo "FAIL"
>> +			break
>> +		fi
>> +
>> +		disk="/dev/$(_find_nvme_ns "${def_subsys_uuid}")"
>> +
>> +		# Disable ns change async event notification from target. It
>> +		# would ensure that when we disable the target ns, the host
>> +		# would not receive ns removal notification from target and
>> +		# so from host we can attempt writing to a disabled ns.
>> +		if ! nvmf_disable_ns_change_aen "${disk}"; then
>> +			echo "FAIL"
>> +			break
>> +		fi
>> +
> 
> I'm really not sure disabling a functionality for testing
> purpose gives us full coverage, for a tests I believe it is important
> to have full functionality running and available so it will reflect the
> real setup, e.g. for this scenario we cannot dependent on disabling AEN
> since users are not disabling the AEN's every-time they disable the
> namespace and ends up writing to the ns on host side triggering the BUG.
> 
OK I think maybe there's a confusion, let me clarify one thing here: We're
*not* disabling AEN functionality but only masking the ns-changed async even 
notification from target and I believe that's perfectly legal per spec. I 
shall update the comment added in the test case to avoid this confusion. 

> Can this be an exception for some reason ?
> 
> Shinichiro, Daniel any thoughts ?
> 
> why not something like following ? (totally untested and rough)
> 
> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
> index 1f4e9989663b..eede3a7c5594 100644
> --- a/drivers/nvme/target/core.c
> +++ b/drivers/nvme/target/core.c
> @@ -5,6 +5,7 @@
>    */
>   #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>   #include <linux/module.h>
> +#include <linux/delay.h>
>   #include <linux/random.h>
>   #include <linux/rculist.h>
>   #include <linux/pci-p2pdma.h>
> @@ -18,6 +19,11 @@
>   #include "nvmet.h"
>   #include "debugfs.h"
> 
> +unsigned int ns_disable_error_inject;
> +module_param(ns_disable_error_inject, int, 0644);
> +MODULE_PARM_DESC(ns_disable_error_inject,
> +                "delay xa_erase() in ns-disable path in seconds 
> (default 0)");
> +
>   struct kmem_cache *nvmet_bvec_cache;
>   struct workqueue_struct *buffered_io_wq;
>   struct workqueue_struct *zbd_wq;
> @@ -649,6 +655,8 @@ void nvmet_ns_disable(struct nvmet_ns *ns)
>                  goto out_unlock;
> 
>          ns->enabled = false;
> +       if (ns_disable_error_inject)
> +               msleep(ns_disable_error_inject * 1000);
>          xa_erase(&ns->subsys->namespaces, ns->nsid);
>          if (ns->nsid == subsys->max_nsid)
>                  subsys->max_nsid = nvmet_max_nsid(subsys);
> 
> 
> and add load the module with modprobe ns_disable_error_inject=10 ?
> or we can even mode this module as a part of ns configfs attr.
IMO, adding sleep won't help always because it's not guaranteed that 
write to ns would happen *always* at the correct time. Yes, in theory,
above change would work most of the time but may not always?
> 
> with above change following statement will block for 10 seconds.
>> +		# disable target namespace and write to it
>> +		echo 0 > ${ns_path}/enable
>> +		nvme write --start-block=1 --block-count=0 \
>> +			--data-size=512 --data="$img" "$disk" 2>>"$FULL"
> 
> -ck
> 
> 
So, as you suggested, I think lets wait for Daniel, Shinichiro and others to
comment on this.

Thanks,
--Nilay

Re: [PATCHv2 blktests 2/2] nvme: add test for writing to file-ns just after disabling it

[Daniel Wagner]

On Thu, Dec 05, 2024 at 03:10:23PM +0530, Nilay Shroff wrote:
> >> +	img="$(mktemp /tmp/blk_img_XXXXXX)"
> >> +	dd if=/dev/urandom of="$img" bs=512 count=1 status=none
> > 
> > why can't we just use the standard block device backed ns ?
> Using standard block device would also work however using the file backed block 
> device would be handy in case user doesn't have a spare standard block device 
> available which could be used for writing a random junk to it.

If I understood Chaitanya correctly, he would prefer to use the existing
helpers to setup the backend, either file or block. If so, I am also on
board with this idea, not to hard code backend into the test. We have
moved this into the helpers so that it's a test parameter.

> OK I think maybe there's a confusion, let me clarify one thing here: We're
> *not* disabling AEN functionality but only masking the ns-changed async even 
> notification from target and I believe that's perfectly legal per spec. I 
> shall update the comment added in the test case to avoid this confusion. 
> 
> > Can this be an exception for some reason ?
> > 
> > Shinichiro, Daniel any thoughts ?

I haven't really checked what the spec says on this topic. Nilay, could
you give a pointer, so I don't spend too much time trying to find the
right spot in the spec. Thanks!

> > +unsigned int ns_disable_error_inject;
> > +module_param(ns_disable_error_inject, int, 0644);
> > +MODULE_PARM_DESC(ns_disable_error_inject,
> > +                "delay xa_erase() in ns-disable path in seconds 
> > (default 0)");
> > +

Please no. This type of debugging/testing interface is hard to use IMO.
It should be in one of the psydo FS, sysfs, configfs or debugfs if
really needed.

Daniel

Re: [PATCHv2 blktests 2/2] nvme: add test for writing to file-ns just after disabling it

[Nilay Shroff]

On 12/5/24 16:17, Daniel Wagner wrote:
> On Thu, Dec 05, 2024 at 03:10:23PM +0530, Nilay Shroff wrote:
>>>> +	img="$(mktemp /tmp/blk_img_XXXXXX)"
>>>> +	dd if=/dev/urandom of="$img" bs=512 count=1 status=none
>>>
>>> why can't we just use the standard block device backed ns ?
>> Using standard block device would also work however using the file backed block 
>> device would be handy in case user doesn't have a spare standard block device 
>> available which could be used for writing a random junk to it.
> 
> If I understood Chaitanya correctly, he would prefer to use the existing
> helpers to setup the backend, either file or block. If so, I am also on
> board with this idea, not to hard code backend into the test. We have
> moved this into the helpers so that it's a test parameter.
> 
>> OK I think maybe there's a confusion, let me clarify one thing here: We're
>> *not* disabling AEN functionality but only masking the ns-changed async even 
>> notification from target and I believe that's perfectly legal per spec. I 
>> shall update the comment added in the test case to avoid this confusion. 
>>
>>> Can this be an exception for some reason ?
>>>
>>> Shinichiro, Daniel any thoughts ?
> 
> I haven't really checked what the spec says on this topic. Nilay, could
> you give a pointer, so I don't spend too much time trying to find the
> right spot in the spec. Thanks!
> 
Sure, you may refer "5.27.1.8 Asynchronous Event Configuration (Feature Identifier 0Bh)"
in NVMe base spec 2.0. And then refer Figure 236, Namespace Attribute Notices: 

>>> +unsigned int ns_disable_error_inject;
>>> +module_param(ns_disable_error_inject, int, 0644);
>>> +MODULE_PARM_DESC(ns_disable_error_inject,
>>> +                "delay xa_erase() in ns-disable path in seconds 
>>> (default 0)");
>>> +
> 
> Please no. This type of debugging/testing interface is hard to use IMO.
> It should be in one of the psydo FS, sysfs, configfs or debugfs if
> really needed.
> 
> Daniel
> 
Thanks,
--Nilay

Re: [PATCHv2 blktests 2/2] nvme: add test for writing to file-ns just after disabling it

[Daniel Wagner]

On Thu, Dec 05, 2024 at 04:36:26PM +0530, Nilay Shroff wrote:
> Sure, you may refer "5.27.1.8 Asynchronous Event Configuration (Feature Identifier 0Bh)"
> in NVMe base spec 2.0. And then refer Figure 236, Namespace Attribute
> Notices:

  Attached Namespace Attribute Notices (NAN): This bit determines
  whether an asynchronous event notification is sent to the host for an
  Attached Namespace Attribute Changed asynchronous event (refer to
  Figure 151). If this bit is set to ‘1’, then the Attached Namespace
  Attribute Changed asynchronous event is sent to the host when this
  condition occurs. If this bit is cleared to ‘0’, then the controller
  shall not send the Attached Namespace Attribute Changed asynchronous
  event to the host.

I agree, doesn't say that the controller always has to send a NAN AEN.

Re: [PATCHv2 blktests 2/2] nvme: add test for writing to file-ns just after disabling it

[Shinichiro Kawasaki]

On Dec 05, 2024 / 12:20, Daniel Wagner wrote:
> On Thu, Dec 05, 2024 at 04:36:26PM +0530, Nilay Shroff wrote:
> > Sure, you may refer "5.27.1.8 Asynchronous Event Configuration (Feature Identifier 0Bh)"
> > in NVMe base spec 2.0. And then refer Figure 236, Namespace Attribute
> > Notices:
> 
>   Attached Namespace Attribute Notices (NAN): This bit determines
>   whether an asynchronous event notification is sent to the host for an
>   Attached Namespace Attribute Changed asynchronous event (refer to
>   Figure 151). If this bit is set to ‘1’, then the Attached Namespace
>   Attribute Changed asynchronous event is sent to the host when this
>   condition occurs. If this bit is cleared to ‘0’, then the controller
>   shall not send the Attached Namespace Attribute Changed asynchronous
>   event to the host.
> 
> I agree, doesn't say that the controller always has to send a NAN AEN.

Thanks for the discussion. IIUC, the users may have the NVME control which
does not send a NAN AEN. In that case, the environment has same set up as
this test case prepares. So this test does not look so weird for me.

Re: [PATCHv2 blktests 2/2] nvme: add test for writing to file-ns just after disabling it

[Shinichiro Kawasaki]

Thanks for this v2 patch. I took a quick look. Please find my comments in line.
Will do another round of review when v3 comes out.

On Dec 04, 2024 / 17:17, Nilay Shroff wrote:
> This is a regression test for commit 505363957fad ("nvmet: fix nvme
> status code when namespace is disabled")[1].
> 
> This test creates a regular file backed loop target namespace, disables
> the asynchronous event notification for ns-changed events and then write
> to the namespace just after disabling it.
> 
> [1] https://lore.kernel.org/linux-nvme/tqcy3sveity7p56v7ywp7ssyviwcb3w4623cnxj3knoobfcanq@yxgt2mjkbkam/
> 
> Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
> ---
>  tests/nvme/055     | 117 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/nvme/055.out |   2 +
>  2 files changed, 119 insertions(+)
>  create mode 100755 tests/nvme/055
>  create mode 100644 tests/nvme/055.out
> 
> diff --git a/tests/nvme/055 b/tests/nvme/055
> new file mode 100755
> index 0000000..f1f19fe
> --- /dev/null
> +++ b/tests/nvme/055
> @@ -0,0 +1,117 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright (C) 2024 Nilay Shroff
> +#
> +# Regression test for commit 505363957fad ("nvmet: fix nvme
> +# status code when namespace is disabled").

As I commented for v1 series after this v2 came out, I suggest to describe that
this test intents to be independent from udev rules.

> +
> +. tests/nvme/rc
> +
> +DESCRIPTION="Test nvme write to a loop target ns just after ns is disabled"
> +
> +QUICK=1
> +

[...]

> +
> +test() {
> +	echo "Running ${TEST_NAME}"
> +
> +	local iteration=1 i=0
> +
> +	_setup_nvmet
> +
> +	_nvmet_target_setup
> +
> +	_nvme_connect_subsys
> +
> +	img="$(mktemp /tmp/blk_img_XXXXXX)"
> +	dd if=/dev/urandom of="$img" bs=512 count=1 status=none

mktemp is not recommended, since blktests provides $TMPDIR as described
in the "new" file.

> +
> +	subsys_path="${NVMET_CFS}/subsystems/${def_subsysnqn}"
> +	ns_path="${subsys_path}/namespaces/${def_nsid}"
> +
> +	while (( i < iteration )); do
> +		# Wait until async request is processed and default ns
> +		# is created

[...]

> +
> +		# disable target namespace and write to it
> +		echo 0 > ${ns_path}/enable
> +		nvme write --start-block=1 --block-count=0 \
> +			--data-size=512 --data="$img" "$disk" 2>>"$FULL"

Instead of --data="$img", can we use"--data=/dev/urandam"? If it works,
we can avoid the img file.

> +
> +		((i++))
> +	done
> +
> +	rm "$img"

If we need img file, let's use $TMPDIR to skip this rm command.

> +
> +	_nvme_disconnect_subsys >> "$FULL" 2>&1
> +
> +	_nvmet_target_cleanup
> +
> +	echo "Test complete"
> +}

[...]

Re: [PATCHv2 blktests 2/2] nvme: add test for writing to file-ns just after disabling it

[Nilay Shroff]

On 12/6/24 07:23, Shinichiro Kawasaki wrote:
> Thanks for this v2 patch. I took a quick look. Please find my comments in line.
> Will do another round of review when v3 comes out.
> 
> On Dec 04, 2024 / 17:17, Nilay Shroff wrote:
>> This is a regression test for commit 505363957fad ("nvmet: fix nvme
>> status code when namespace is disabled")[1].
>>
>> This test creates a regular file backed loop target namespace, disables
>> the asynchronous event notification for ns-changed events and then write
>> to the namespace just after disabling it.
>>
>> [1] https://lore.kernel.org/linux-nvme/tqcy3sveity7p56v7ywp7ssyviwcb3w4623cnxj3knoobfcanq@yxgt2mjkbkam/
>>
>> Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
>> ---
>>  tests/nvme/055     | 117 +++++++++++++++++++++++++++++++++++++++++++++
>>  tests/nvme/055.out |   2 +
>>  2 files changed, 119 insertions(+)
>>  create mode 100755 tests/nvme/055
>>  create mode 100644 tests/nvme/055.out
>>
>> diff --git a/tests/nvme/055 b/tests/nvme/055
>> new file mode 100755
>> index 0000000..f1f19fe
>> --- /dev/null
>> +++ b/tests/nvme/055
>> @@ -0,0 +1,117 @@
>> +#!/bin/bash
>> +# SPDX-License-Identifier: GPL-3.0+
>> +# Copyright (C) 2024 Nilay Shroff
>> +#
>> +# Regression test for commit 505363957fad ("nvmet: fix nvme
>> +# status code when namespace is disabled").
> 
> As I commented for v1 series after this v2 came out, I suggest to describe that
> this test intents to be independent from udev rules.
> 
Sure, in the next patch I will update the comment.
>> +
>> +. tests/nvme/rc
>> +
>> +DESCRIPTION="Test nvme write to a loop target ns just after ns is disabled"
>> +
>> +QUICK=1
>> +
> 
> [...]
> 
>> +
>> +test() {
>> +	echo "Running ${TEST_NAME}"
>> +
>> +	local iteration=1 i=0
>> +
>> +	_setup_nvmet
>> +
>> +	_nvmet_target_setup
>> +
>> +	_nvme_connect_subsys
>> +
>> +	img="$(mktemp /tmp/blk_img_XXXXXX)"
>> +	dd if=/dev/urandom of="$img" bs=512 count=1 status=none
> 
> mktemp is not recommended, since blktests provides $TMPDIR as described
> in the "new" file.
> 
>> +
>> +	subsys_path="${NVMET_CFS}/subsystems/${def_subsysnqn}"
>> +	ns_path="${subsys_path}/namespaces/${def_nsid}"
>> +
>> +	while (( i < iteration )); do
>> +		# Wait until async request is processed and default ns
>> +		# is created
> 
> [...]
> 
>> +
>> +		# disable target namespace and write to it
>> +		echo 0 > ${ns_path}/enable
>> +		nvme write --start-block=1 --block-count=0 \
>> +			--data-size=512 --data="$img" "$disk" 2>>"$FULL"
> 
> Instead of --data="$img", can we use"--data=/dev/urandam"? If it works,
> we can avoid the img file.
> 
Yeah I just tested with "/dev/urandom" and that seems to work. So 
we don't need img file. I will update this in the next patch.
>> +
>> +		((i++))
>> +	done
>> +
>> +	rm "$img"
> 
> If we need img file, let's use $TMPDIR to skip this rm command.
As /dev/urandom works, I would remove the use of img file. 
> 
>> +
>> +	_nvme_disconnect_subsys >> "$FULL" 2>&1
>> +
>> +	_nvmet_target_cleanup
>> +
>> +	echo "Test complete"
>> +}
> 
> [...]

Thanks,
--Nilay