[PATCHv2 blktests] nvme/{033-037}: timeout while waiting for nvme passthru namespace device

[PATCHv2 blktests] nvme/{033-037}: timeout while waiting for nvme passthru namespace device

[Nilay Shroff]

Avoid waiting indefinitely for nvme passthru namespace block device
to appear. Wait for up to 5 seconds and during this time if namespace
device doesn't appear then bail out and FAIL the test.

Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
---
    Changes from v1:
        - Add a meaningful error message if test fails (Shnichiro
          Kawasaki)
        - Use sleep "1" instead of ".1" while waiting for nsdev to be
          created as we don't see much gain in test runtime with short 
          duration of sleep. This would also help further optimize
          the sleep logic (Shnichiro Kawasaki)
        - Few other trivial cleanups (Shnichiro Kawasaki)
---
 tests/nvme/033 | 7 +++++--
 tests/nvme/034 | 8 +++++---
 tests/nvme/035 | 6 +++---
 tests/nvme/036 | 1 +
 tests/nvme/037 | 7 ++++++-
 tests/nvme/rc  | 9 ++++++++-
 6 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/tests/nvme/033 b/tests/nvme/033
index 5e05175..d0581c2 100755
--- a/tests/nvme/033
+++ b/tests/nvme/033
@@ -62,8 +62,11 @@ test_device() {
 	_nvmet_passthru_target_setup
 
 	nsdev=$(_nvmet_passthru_target_connect)
-
-	compare_dev_info "${nsdev}"
+	if [[ -z "$nsdev" ]]; then
+		echo "FAIL: Failed to find passthru target namespace"
+	else
+		compare_dev_info "${nsdev}"
+	fi
 
 	_nvme_disconnect_subsys
 	_nvmet_passthru_target_cleanup
diff --git a/tests/nvme/034 b/tests/nvme/034
index 154fc91..a4c5e97 100755
--- a/tests/nvme/034
+++ b/tests/nvme/034
@@ -27,13 +27,15 @@ test_device() {
 
 	_setup_nvmet
 
-	local ctrldev
 	local nsdev
 
 	_nvmet_passthru_target_setup
 	nsdev=$(_nvmet_passthru_target_connect)
-
-	_run_fio_verify_io --size="${NVME_IMG_SIZE}" --filename="${nsdev}"
+	if [[ -z "$nsdev" ]]; then
+		echo "FAIL: Failed to find passthru target namespace"
+	else
+		_run_fio_verify_io --size="${NVME_IMG_SIZE}" --filename="${nsdev}"
+	fi
 
 	_nvme_disconnect_subsys
 	_nvmet_passthru_target_cleanup
diff --git a/tests/nvme/035 b/tests/nvme/035
index ff217d6..9f84ced 100755
--- a/tests/nvme/035
+++ b/tests/nvme/035
@@ -30,13 +30,13 @@ test_device() {
 
 	_setup_nvmet
 
-	local ctrldev
 	local nsdev
 
 	_nvmet_passthru_target_setup
 	nsdev=$(_nvmet_passthru_target_connect)
-
-	if ! _xfs_run_fio_verify_io "${nsdev}" "${NVME_IMG_SIZE}"; then
+	if [[ -z "$nsdev" ]]; then
+		echo "FAIL: Failed to find passthru target namespace"
+	elif ! _xfs_run_fio_verify_io "${nsdev}" "${NVME_IMG_SIZE}"; then
 		echo "FAIL: fio verify failed"
 	fi
 
diff --git a/tests/nvme/036 b/tests/nvme/036
index 442ffe7..a114a7c 100755
--- a/tests/nvme/036
+++ b/tests/nvme/036
@@ -27,6 +27,7 @@ test_device() {
 	_setup_nvmet
 
 	local ctrldev
+	local nsdev
 
 	_nvmet_passthru_target_setup
 	nsdev=$(_nvmet_passthru_target_connect)
diff --git a/tests/nvme/037 b/tests/nvme/037
index f7ddc2d..33a6857 100755
--- a/tests/nvme/037
+++ b/tests/nvme/037
@@ -27,7 +27,7 @@ test_device() {
 
 	local subsys="blktests-subsystem-"
 	local iterations=10
-	local ctrldev
+	local nsdev
 
 	for ((i = 0; i < iterations; i++)); do
 		_nvmet_passthru_target_setup --subsysnqn "${subsys}${i}"
@@ -37,6 +37,11 @@ test_device() {
 		_nvme_disconnect_subsys \
 			--subsysnqn "${subsys}${i}" >>"${FULL}" 2>&1
 		_nvmet_passthru_target_cleanup --subsysnqn "${subsys}${i}"
+
+		if [[ -z "$nsdev" ]]; then
+			echo "FAIL: Failed to find passthru target namespace"
+			break
+		fi
 	done
 
 	echo "Test complete"
diff --git a/tests/nvme/rc b/tests/nvme/rc
index a877de3..671012e 100644
--- a/tests/nvme/rc
+++ b/tests/nvme/rc
@@ -394,6 +394,8 @@ _nvmet_passthru_target_setup() {
 
 _nvmet_passthru_target_connect() {
 	local subsysnqn="$def_subsysnqn"
+	local timeout="5"
+	local count="0"
 
 	while [[ $# -gt 0 ]]; do
 		case $1 in
@@ -414,7 +416,12 @@ _nvmet_passthru_target_connect() {
 	# The following tests can race with the creation
 	# of the device so ensure the block device exists
 	# before continuing
-	while [ ! -b "${nsdev}" ]; do sleep 1; done
+	while [ ! -b "${nsdev}" ]; do
+		sleep 1
+		if ((++count >= timeout)); then
+			return 1
+		fi
+	done
 
 	echo "${nsdev}"
 }
-- 
2.45.2

Re: [PATCHv2 blktests] nvme/{033-037}: timeout while waiting for nvme passthru namespace device

[Shinichiro Kawasaki]

On Sep 25, 2024 / 13:01, Nilay Shroff wrote:
> Avoid waiting indefinitely for nvme passthru namespace block device
> to appear. Wait for up to 5 seconds and during this time if namespace
> device doesn't appear then bail out and FAIL the test.
> 
> Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
> ---
>     Changes from v1:
>         - Add a meaningful error message if test fails (Shnichiro
>           Kawasaki)
>         - Use sleep "1" instead of ".1" while waiting for nsdev to be
>           created as we don't see much gain in test runtime with short 
>           duration of sleep. This would also help further optimize
>           the sleep logic (Shnichiro Kawasaki)
>         - Few other trivial cleanups (Shnichiro Kawasaki)

Thanks for this v2 patch.

[...]

> diff --git a/tests/nvme/036 b/tests/nvme/036
> index 442ffe7..a114a7c 100755
> --- a/tests/nvme/036
> +++ b/tests/nvme/036
> @@ -27,6 +27,7 @@ test_device() {
>  	_setup_nvmet
>  
>  	local ctrldev
> +	local nsdev
>  
>  	_nvmet_passthru_target_setup
>  	nsdev=$(_nvmet_passthru_target_connect)

I commented for v1 that "unnecsseary change" was made for nmve/036. With that
comment, I meant that one empty line removal was unnecessary. However, you
removed the nsdev check in nvme/036 for v2. I guess you might have misunderstood
my comment then removed the check. I suggest to put back the nsdev check in
nvme/036. If you agree, could you send out v3? Or I can do it when I apply this
patch.

Other than that, this patch looks good to me. Let's wait and see if anyone has
other comments.

Re: [PATCHv2 blktests] nvme/{033-037}: timeout while waiting for nvme passthru namespace device

[Nilay Shroff]

On 9/27/24 07:41, Shinichiro Kawasaki wrote:
> On Sep 25, 2024 / 13:01, Nilay Shroff wrote:
>> Avoid waiting indefinitely for nvme passthru namespace block device
>> to appear. Wait for up to 5 seconds and during this time if namespace
>> device doesn't appear then bail out and FAIL the test.
>>
>> Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
>> ---
>>     Changes from v1:
>>         - Add a meaningful error message if test fails (Shnichiro
>>           Kawasaki)
>>         - Use sleep "1" instead of ".1" while waiting for nsdev to be
>>           created as we don't see much gain in test runtime with short 
>>           duration of sleep. This would also help further optimize
>>           the sleep logic (Shnichiro Kawasaki)
>>         - Few other trivial cleanups (Shnichiro Kawasaki)
> 
> Thanks for this v2 patch.
> 
> [...]
> 
>> diff --git a/tests/nvme/036 b/tests/nvme/036
>> index 442ffe7..a114a7c 100755
>> --- a/tests/nvme/036
>> +++ b/tests/nvme/036
>> @@ -27,6 +27,7 @@ test_device() {
>>  	_setup_nvmet
>>  
>>  	local ctrldev
>> +	local nsdev
>>  
>>  	_nvmet_passthru_target_setup
>>  	nsdev=$(_nvmet_passthru_target_connect)
> 
> I commented for v1 that "unnecsseary change" was made for nmve/036. With that
> comment, I meant that one empty line removal was unnecessary. However, you
> removed the nsdev check in nvme/036 for v2. I guess you might have misunderstood
> my comment then removed the check. I suggest to put back the nsdev check in
> nvme/036. If you agree, could you send out v3? Or I can do it when I apply this
> patch.
> 
Ah, yes I think that was misunderstanding. When I reviewed your last comment about
nvme/036, I thought you wanted to remove nsdev check just because the intention of 
the nvme/036 test was to test "reset controller" command. So in that sense, the nsdev 
check is not important. Having said that, yes keeping nsdev check may also not 
harm. So I would add that nsdev check back in nvme/036 and send out patch v3.  


> Other than that, this patch looks good to me. Let's wait and see if anyone has
> other comments.

Thanks,
--Nilay