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

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

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