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

[Daniel Wagner]

On Wed, Aug 16, 2023 at 12:04:24PM +0000, Shinichiro Kawasaki wrote:
> > 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.

I am worried about the case that in _nvmet_passthru_target_connect we
call first _nvme_connet_subsys and then we do also the above loop.

> 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.

And this is something we should not do on purpose, IMO.

> > 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.

I'd rather not go down this route. Ideally, the infrastructure code does
the right thing and we don't have to deal in each test case with this
problem.

> 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.

As we already have an argument parser in _nvme_connect_subsys, we could
also introduce a new option which allows to select the wait type. With
this _nvmet_passtrhu_target_connect could be something like

_nvmet_passthru_target_connect() {
        [...]

        _nvme_connect_subsys "${trtype}" "${subsys_name}" \
                --wait-for=device || return

        [...]
}

and for the rest of the test cases we just set the default for
--wait-for to ns.