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

[Shinichiro Kawasaki]

On Aug 20, 2024 / 22:25, Nilay Shroff wrote:
> 
> 
> On 8/20/24 15:50, Shin'ichiro Kawasaki wrote:
[...]
> > diff --git a/tests/nvme/052 b/tests/nvme/052
> > index cf6061a..e1ac823 100755
> > --- a/tests/nvme/052
> > +++ b/tests/nvme/052
> > @@ -39,15 +39,32 @@ nvmf_wait_for_ns() {
> >  		ns=$(_find_nvme_ns "${uuid}")
> >  	done
> >  
> > +	echo "$ns"
> >  	return 0
> >  }
> >  
> > +nvmf_wait_for_ns_removal() {
> > +	local ns=$1 i
> > +
> > +	for ((i = 0; i < 10; i++)); do
> > +		if [[ ! -e /dev/$ns ]]; then
> > +			return
> > +		fi
> > +		sleep .1
> > +		echo "wait removal of $ns" >> "$FULL"
> > +	done
> > +
> > +	if [[ -e /dev/$ns ]]; then
> > +		echo "Failed to remove the namespace $ns"
> > +	fi
> > +}
> > +
> Under nvmf_wait_for_ns_removal(), instead of checking the existence of "/dev/$ns", 
> how about checking the existence of file "/sys/block/$ns"? As we know, when this issue 
> manifests, we have a stale entry "/sys/block/$ns/$uuid" lurking around from the 
> previous iteration for sometime causing the observed symptom. So I think, we may reuse the 
> _find_nvme_ns() function to wait until the stale "/sys/block/$ns/$uuid" file 
> exists.

It sounds a good idea to reuse _find_nvme_ns().

> Maybe something like below:
> 
> nvmf_wait_for_ns_removal() {
>         local ns
>         local timeout="5"
>         local uuid="$1"
> 
>         ns=$(_find_nvme_ns "${uuid}")

I tried this, and found that the _find_nvme_ns call spits out the failure
"cat: /sys/block/nvme1n2/uuid: No such file or directory", because the
delayed namespace removal can happen here. To suppress the error message,
this line should be,

         ns=$(_find_nvme_ns "${uuid}" 2> /dev/null)

> 
>         start_time=$(date +%s)
>         while [[ ! -z "$ns" ]]; do
>                 sleep 1
>                 end_time=$(date +%s)
>                 if (( end_time - start_time > timeout )); then
>                         echo "namespace with uuid \"${uuid}\" still " \
>                                 "not deleted within ${timeout} seconds"
>                         return 1
>                 fi
> 		echo "Waiting for $ns removal" >> ${FULL}
>                 ns=$(_find_nvme_ns "${uuid}")

Same comment as above.

> 				
>         done
> 
>         return 0
> }

I found that your nvmf_wait_for_ns_removal() above has certain amount of
duplication with the existing nvmf_wait_for_ns(). To avoid the duplication,
I suggest to reuse nvmf_wait_for_ns() and add a new argument to control wait
target event: namespace 'created' or 'removed'. With this idea, I created the
patch below. I confirmed the patch avoids the failure.

One drawback of this new patch based on your suggestion is that it extends
execution time of the test case from 20+ seconds to 40+ seconds. In most cases,
the while loop condition check in nvmf_wait_for_ns() is true at the first time,
and false at the the second time. So, nvmf_wait_for_ns() takes 1 second for one
time "sleep 1". Before applying this patch, it took 20+ seconds for 20 times
iteration. After applying the patch, it takes 40+ seconds, since one iteration
calls nvmf_wait_for_ns() twice. So how about to reduce the sleep time from 1 to
0.1? I tried it and observed that it reduced the runtime from 40+ seconds to 10+
seconds.

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
+
 		rm "$(_nvme_def_file_path).$i"
 	}
 	done