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

[Chaitanya Kulkarni <chaitanyak@nvidia.com>]

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