[PATCH blktests] nvme/rc: fix nvme device readiness check after _nvme_connect_subsys

[Shinichiro Kawasaki]

On Aug 11, 2023 / 08:37, Daniel Wagner wrote:
> On Fri, Aug 11, 2023 at 10:23:34AM +0900, Shin'ichiro Kawasaki wrote:
> > --- a/tests/nvme/rc
> > +++ b/tests/nvme/rc
> > @@ -425,6 +425,7 @@ _nvme_connect_subsys() {
> >  	local keep_alive_tmo=""
> >  	local reconnect_delay=""
> >  	local ctrl_loss_tmo=""
> > +	local dev i
> >  
> >  	while [[ $# -gt 0 ]]; do
> >  		case $1 in
> > @@ -529,6 +530,16 @@ _nvme_connect_subsys() {
> >  	fi
> >  
> >  	nvme connect "${ARGS[@]}" 2> /dev/null
> > +
> > +	dev=$(_find_nvme_dev "$subsysnqn")
> > +	for ((i = 0; i < 10; i++)); do
> > +		if [[ -b /dev/${dev}n1 &&
> > +			      -e /sys/block/${dev}n1/uuid &&
> > +			      -e /sys/block/${dev}n1/wwid ]]; then
> > +			return
> > +		fi
> > +		sleep .1
> > +	done
> >  }
> 
> Not sure if this going to work for the passthru case as intended.

IMO, the check above is not for the passthru case. The function
_nvmet_passthru_target_connect() has the code below:

	while [ ! -b "${nsdev}" ]; do sleep 1; done

This checks readiness of the device for the passthru case.

> If you
> look at the _find_nvme_passthru_loop_dev() function, there is a logic to
> figure out which namespace to use. _nvmet_passthru_target_connect()
> is also using _nvme_connect_subsys() so it is possible that the
> test device for the passthru case uses not namespace 1.
> 
> If namespace 1 doesn't exist we just loop for 1 second. So in this
> particular case nothing changes. Still not nice.

For that case, the check in _nvmet_passthru_target_connect() ensures
readiness of the passthru device. The drawback is that the check for
namespace 1 consumes 1 second for nothing.

> 
> Thinking about it, shouldn't we log that we couldn't find the
> device/uuid/wwid at the end of the loop?

Not sure. For the non-passthru case, it will be helpful. But for passthru case,
check result log for namespace 1 can be confusing.

I can think of two other fix approaches below, but they did not look better than
this patch for me. What do you think?

1) Go back to the fix approach to add another _find_nvme_dev() in nvme/047.
   I worried this will leave the chance that we will fall into the same issue
   when we will add a new test case with multiple _nvme_connect_subsys calls.

2) Rework _find_nvme_dev into two new functions _find_nvme_ctrl_dev and
   _find_nvme_ns_dev, and do the readiness check in _find_nvme_ns_dev.
   IMO, this confusion comes from the fact that _find_nvme_dev returns control
   device, but some test cases use it to operate namespaces by adding "n1" to
   the control device name. If a test case uses namespace device, it's the
   better to call _find_nvme_ns_dev. But I worry this approach may be too much.