Re: [PATCH 1/2] nvme: check for NVME_CTRL_LIVE in nvme_report_ns_ids()

[Keith Busch <kbusch@kernel.org>]

On Thu, Jun 04, 2020 at 01:56:01PM +0200, Hannes Reinecke wrote:
> When a controller reset happens during scanning nvme_identify_ns()
> will be aborted, but the subsequent call to nvme_identify_ns_descs()
> will stall as it will only be completed once the controller reconnect
> is finished.

If nvme_identify_ns() is aborted, shouldn't we not proceed to the next
command? It looks like the code already does that, from nvme_alloc_ns():

	ret = nvme_identify_ns(ctrl, nsid, &id);
	if (ret)
		goto out_free_queue;

And then from nvme_revalidate_disk():

	ret = nvme_identify_ns(ctrl, ns->head->ns_id, &id);
	if (ret)
		goto out;

Those are the only two paths to nvme_identify_ns_descs(), so it looks
like nvme_identify_ns() must be successful in order to get there.

> But as the reconnect waits for scanning to complete the particular
> namespace will deadlock and never reconnected again.

The fix looks a bit fragile. What if the controller is reset immediately
after the check for live? It'd be safer such that reconnect didn't have
to wait for scan_work, no?  The pci transport has no such dependency,
for example.

> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 569671e264b5..b811787505f7 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1792,7 +1792,7 @@ static int nvme_report_ns_ids(struct nvme_ctrl *ctrl, unsigned int nsid,
>  		memcpy(ids->eui64, id->eui64, sizeof(id->eui64));
>  	if (ctrl->vs >= NVME_VS(1, 2, 0))
>  		memcpy(ids->nguid, id->nguid, sizeof(id->nguid));
> -	if (ctrl->vs >= NVME_VS(1, 3, 0))
> +	if (ctrl->vs >= NVME_VS(1, 3, 0) && ctrl->state == NVME_CTRL_LIVE)
>  		return nvme_identify_ns_descs(ctrl, nsid, ids);
>  	return 0;
>  }

_______________________________________________
linux-nvme mailing list
linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme