[PATCH blktests] nvme/052: wait for namespace removal before recreating namespace

[Nilay Shroff]

On 8/22/24 14:03, Shinichiro Kawasaki wrote:
> On Aug 21, 2024 / 14:58, Nilay Shroff wrote:
>>
>>
>> On 8/21/24 12:51, Shinichiro Kawasaki wrote:
> [...]
>>> diff --git a/tests/nvme/052 b/tests/nvme/052
>>> index cf6061a..22e0bf5 100755
>>> --- a/tests/nvme/052
>>> +++ b/tests/nvme/052
>>> @@ -20,23 +20,35 @@ set_conditions() {
>>>  	_set_nvme_trtype "$@"
>>>  }
>>>  
>>> +find_nvme_ns() {
>>> +	if [[ "$2" == removed ]]; then
>>> +		_find_nvme_ns "$1" 2> /dev/null
>>> +	else
>>> +		_find_nvme_ns "$1"
>>> +	fi
>>> +}
>>> +
>>> +# Wait for the namespace with specified uuid to fulfill the specified condtion,
>>> +# "created" or "removed".
>>>  nvmf_wait_for_ns() {
>>>  	local ns
>>>  	local timeout="5"
>>>  	local uuid="$1"
>>> +	local condition="$2"
>>>  
>>> -	ns=$(_find_nvme_ns "${uuid}")
>>> +	ns=$(find_nvme_ns "${uuid}" "${condition}")
>>>  
>>>  	start_time=$(date +%s)
>>> -	while [[ -z "$ns" ]]; do
>>> +	while [[ -z "$ns" && "$condition" == created  ]] ||
>>> +		      [[ -n "$ns" && "$condition" == removed ]]; do
>>>  		sleep 1
>>>  		end_time=$(date +%s)
>>>  		if (( end_time - start_time > timeout )); then
>>>  			echo "namespace with uuid \"${uuid}\" not " \
>>> -				"found within ${timeout} seconds"
>>> +				"${condition} within ${timeout} seconds"
>>>  			return 1
>>>  		fi
>>> -		ns=$(_find_nvme_ns "${uuid}")
>>> +		ns=$(find_nvme_ns "${uuid}" "${condition}")
>>>  	done
>>>  
>>>  	return 0
>>> @@ -63,7 +75,7 @@ test() {
>>>  		_create_nvmet_ns "${def_subsysnqn}" "${i}" "$(_nvme_def_file_path).$i" "${uuid}"
>>>  
>>>  		# wait until async request is processed and ns is created
>>> -		nvmf_wait_for_ns "${uuid}"
>>> +		nvmf_wait_for_ns "${uuid}" created
>>>  		if [ $? -eq 1 ]; then
>>>  			echo "FAIL"
>>>  			rm "$(_nvme_def_file_path).$i"
>>> @@ -71,6 +83,10 @@ test() {
>>>  		fi
>>>  
>>>  		_remove_nvmet_ns "${def_subsysnqn}" "${i}"
>>> +
>>> +		# wait until async request is processed and ns is removed
>>> +		nvmf_wait_for_ns "${uuid}" removed
>>> +
>> As nvme_wait_for_ns() returns either 0 (success) or 1 (failure), I think we 
>> should check the return status of nvme_wait_for_ns() here and bail out in case 
>> it returns failure same as what we do above while creating namespace.
> 
> Agreed, I will add the check to v2.
> 
>>
>> Another point: I think, we may always suppress error from _find_nvme_ns() irrespective
>> of it's being called while "creating" or "removing" the namespace assuming we always 
>> check the return status of nvme_wait_for_ns() in the main loop. So ideally we shall 
>> invoke _find_nvme_ns() from nvme_wait_for_ns() as below:
>>
>> ns=$(_find_nvme_ns "${uuid}" 2>/dev/null)
> 
> It doesn't sound correct to suppress the _find_nvme_ns errors always. We found
> the issue since _find_nvme_ns reported the error at after _create_nvmet_ns()
> call. If we suppress it, I can not be confident that this fix avoids the error.
>
Agreed, however my thought was that if we always(while creating as well as deleting namespace) 
validate the return value from nvme_wait_for_ns() in the main loop then we should be OK and 
we may not need to worry about any error generated from the _find_nvme_ns(). The return 
status from nvme_wait_for_ns() is good enough for main loop to proceed further or bail out.
Having said that, it's debatable as what you pointed out is also valid. The only thing which 
looks odd to me is that we have to suppress the error from _find_nvme_ns() for one case but 
not for the other.  
>>
>> On a cosmetic note: Maybe we can use readonly constants to make "created" and "removed"
>> parameters looks more elegant/readable. 
>>
>> # Define constants
>> readonly NS_ADD="added"
>> readonly NS_DEL="deleted" 
>>  
>> Now we may reuse above constants instead of "created" and "removed". You may rename 
>> constant name if you don't like the name I used above :)
> 
> This sounds too much for me. "created" and "removed" are constant strings, so
> I'm not sure about the merit of introducing the constant variables. Number of
> characters will not change much either.
> 
Alright! Maybe this is more of a personal test.. so I am fine either way.

Thanks,
--Nilay