[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