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

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

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