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.
