Re: [PATCH] nvme-multipath: fix possible io hang after ctrl reconnect

[Hannes Reinecke <hare@suse.de>]

On 10/15/19 7:08 PM, Anton Eidelman wrote:
> The following scenario results in an IO hang:
> 1) ctrl completes a request with NVME_SC_ANA_TRANSITION.
>    NVME_NS_ANA_PENDING bit in ns->flags is set and ana_work is triggered.
> 2) ana_work: nvme_read_ana_log() tries to get the ANA log page from the ctrl.
>    This fails because ctrl disconnects.
>    Therefore nvme_update_ns_ana_state() is not called
>    and NVME_NS_ANA_PENDING bit in ns->flags is not cleared.
> 3) ctrl reconnects: nvme_mpath_init(ctrl,...) calls
>    nvme_read_ana_log(ctrl, groups_only=true).
>    However, nvme_update_ana_state() does not update namespaces
>    because nr_nsids = 0 (due to groups_only mode).
> 4) scan_work calls nvme_validate_ns() finds the ns and re-validates OK.
> 
> Result:
> The ctrl is now live but NVME_NS_ANA_PENDING bit in ns->flags is still set.
> Consequently ctrl will never be considered a viable path by __nvme_find_path().
> IO will hang if ctrl is the only or the last path to the namespace.
> 
> More generally, while ctrl is reconnecting, its ANA state may change.
> And because nvme_mpath_init() requests ANA log in groups_only mode,
> these changes are not propagated to the existing ctrl namespaces.
> This may result in a mal-function or an IO hang.
> 
> Solution:
> nvme_mpath_init() will nvme_read_ana_log() with groups_only set to false.
> This will not harm the new ctrl case (no namespaces present),
> and will make sure the ANA state of namespaces gets updated after reconnect.
> Leaving the groups_only capability in nvme_read_ana_log() in place,
> although it is unused for now (NVME_ANA_LOG_RGO is defined in spec).
> 
> Another option would be for nvme_mpath_init() to invoke
> nvme_parse_ana_log(..., nvme_set_ns_ana_state) for each existing
> namespace.
> 
> Signed-off-by: Anton Eidelman <anton@lightbitslabs.com>
> ---
>  drivers/nvme/host/multipath.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index 30de7efef003..d320684d25b2 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -715,7 +715,7 @@ int nvme_mpath_init(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
>  		goto out;
>  	}
>  
> -	error = nvme_read_ana_log(ctrl, true);
> +	error = nvme_read_ana_log(ctrl, false);
>  	if (error)
>  		goto out_free_ana_log_buf;
>  	return 0;
> 
Actually I think we should drop the RGO handling in nvme_read_ana_log(),
seeing the issues it has caused already.
But this can be done in a new patch.

Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      Teamlead Storage & Networking
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer

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