[PATCH blktests] nvme/058: detach loop device after test finish

[PATCH blktests] nvme/058: detach loop device after test finish

[Nilay Shroff]

The nvme/058 creates three (temp file backed) namespaces and
attach each namespace to a loop device while starting the test.
However it never detach those namespaces from the loop device
once test finishes. Ideally, we should detach loop device from
namespace so that the associated loop device is later destroyed
and its resources are released. This patch helps detach each
namespace from its associated loop device after test finishes.

Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
---
 tests/nvme/058 | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/tests/nvme/058 b/tests/nvme/058
index d230a21..99e7e81 100755
--- a/tests/nvme/058
+++ b/tests/nvme/058
@@ -99,6 +99,17 @@ test() {
 	done
 
 	_nvme_disconnect_subsys
+
+	for ((d = 1; d <= num_namespaces; d++)); do
+		local file_path
+		local blkdev
+
+		file_path="${TMPDIR}/img${d}"
+		blkdev="$(losetup -l | awk -v path="${file_path}" '$6 == path { print $1 }')"
+
+		losetup -d "${blkdev}"
+	done
+
 	_nvmet_target_cleanup
 
 	echo "Test complete"
-- 
2.47.1

Re: [PATCH blktests] nvme/058: detach loop device after test finish

[Shinichiro Kawasaki]

On Jan 16, 2025 / 18:52, Nilay Shroff wrote:
> The nvme/058 creates three (temp file backed) namespaces and
> attach each namespace to a loop device while starting the test.
> However it never detach those namespaces from the loop device
> once test finishes. Ideally, we should detach loop device from
> namespace so that the associated loop device is later destroyed
> and its resources are released. This patch helps detach each
> namespace from its associated loop device after test finishes.

Thank you for catching this issue. This needs a fix.

> 
> Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
> ---
>  tests/nvme/058 | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/tests/nvme/058 b/tests/nvme/058
> index d230a21..99e7e81 100755
> --- a/tests/nvme/058
> +++ b/tests/nvme/058
> @@ -99,6 +99,17 @@ test() {
>  	done
>  
>  	_nvme_disconnect_subsys
> +
> +	for ((d = 1; d <= num_namespaces; d++)); do
> +		local file_path
> +		local blkdev
> +
> +		file_path="${TMPDIR}/img${d}"
> +		blkdev="$(losetup -l | awk -v path="${file_path}" '$6 == path { print $1 }')"
> +
> +		losetup -d "${blkdev}"
> +	done

This for loop looks similar and redundant with the other for loop to set up the
namespaces. How about to keep the blkdevs in an array, and refer to them to
detach? The pseudo code below shows this approach:

 local blkdev
 local -a blkdevs
  ...
 (in the 1st loop)
    ...
    blkdevs+=$("$blkdev")
    ...

  ...
  (after _nvme_disconnect_subsys)

  for blkdev in "${blkdevs[@]}"; do
      losetup --detach "$blkdev"
  done
  ...


Also I suggest to use long option names instead of short names for readability
and code stability: e.g., instead of "losetup -d", let's use "losetup --detach".
(No need to replace short option names in existing code. I'm suggesting this for
the new lines to add.)

> +
>  	_nvmet_target_cleanup
>  
>  	echo "Test complete"
> -- 
> 2.47.1
> 

Re: [PATCH blktests] nvme/058: detach loop device after test finish

[Nilay Shroff]

On 1/23/25 10:23 AM, Shinichiro Kawasaki wrote:
> On Jan 16, 2025 / 18:52, Nilay Shroff wrote:
>> The nvme/058 creates three (temp file backed) namespaces and
>> attach each namespace to a loop device while starting the test.
>> However it never detach those namespaces from the loop device
>> once test finishes. Ideally, we should detach loop device from
>> namespace so that the associated loop device is later destroyed
>> and its resources are released. This patch helps detach each
>> namespace from its associated loop device after test finishes.
> 
> Thank you for catching this issue. This needs a fix.
> 
Thank you for your review and comments!
>>
>> Signed-off-by: Nilay Shroff <nilay@linux.ibm.com>
>> ---
>>  tests/nvme/058 | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/tests/nvme/058 b/tests/nvme/058
>> index d230a21..99e7e81 100755
>> --- a/tests/nvme/058
>> +++ b/tests/nvme/058
>> @@ -99,6 +99,17 @@ test() {
>>  	done
>>  
>>  	_nvme_disconnect_subsys
>> +
>> +	for ((d = 1; d <= num_namespaces; d++)); do
>> +		local file_path
>> +		local blkdev
>> +
>> +		file_path="${TMPDIR}/img${d}"
>> +		blkdev="$(losetup -l | awk -v path="${file_path}" '$6 == path { print $1 }')"
>> +
>> +		losetup -d "${blkdev}"
>> +	done
> 
> This for loop looks similar and redundant with the other for loop to set up the
> namespaces. How about to keep the blkdevs in an array, and refer to them to
> detach? The pseudo code below shows this approach:
> 
>  local blkdev
>  local -a blkdevs
>   ...
>  (in the 1st loop)
>     ...
>     blkdevs+=$("$blkdev")
>     ...
> 
>   ...
>   (after _nvme_disconnect_subsys)
> 
>   for blkdev in "${blkdevs[@]}"; do
>       losetup --detach "$blkdev"
>   done
>   ...
> 
Yes this looks good! I will incorporate this in the next patch.
> 
> Also I suggest to use long option names instead of short names for readability
> and code stability: e.g., instead of "losetup -d", let's use "losetup --detach".
> (No need to replace short option names in existing code. I'm suggesting this for
> the new lines to add.)
> 
Sure, will do the needful.

Thanks,
--Nilay