Re: [PATCH 1/2] nvme: multipath: round-robin: fix logic for non-optimized paths

[Hannes Reinecke <hare@suse.de>]

On 7/16/20 9:59 PM, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Handle the special case where we have exactly one optimized path,
> which we should keep using in this case. Also, use the next
> non-optimized path, not the last one.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>   drivers/nvme/host/multipath.c | 21 +++++++++++----------
>   1 file changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index 74bad4e3d377..2c575b783d3e 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -224,13 +224,8 @@ static struct nvme_ns *nvme_next_ns(struct nvme_ns_head *head,
>   static struct nvme_ns *nvme_round_robin_path(struct nvme_ns_head *head,
>   		int node, struct nvme_ns *old)
>   {
> -	struct nvme_ns *ns, *found, *fallback = NULL;
> +	struct nvme_ns *ns, *found = NULL;
>   
> -	if (list_is_singular(&head->list)) {
> -		if (nvme_path_is_disabled(old))
> -			return NULL;
> -		return old;
> -	}
>   
>   	for (ns = nvme_next_ns(head, old);
>   	     ns != old;

Why do you remove this?
This is an optimisation for single paths, and should stay.

> @@ -242,13 +237,19 @@ static struct nvme_ns *nvme_round_robin_path(struct nvme_ns_head *head,
>   			found = ns;
>   			goto out;
>   		}
> -		if (ns->ana_state == NVME_ANA_NONOPTIMIZED)
> -			fallback = ns;
> +		if (!found && ns->ana_state == NVME_ANA_NONOPTIMIZED)
> +			found = ns;
>   	}
>   
> -	if (!fallback)
> +	/* Fall back to old if it's better than the others */
> +	if (!nvme_path_is_disabled(old) &&
> +	    (old->ana_state == NVME_ANA_OPTIMIZED ||
> +	     (!found && old->ana_state == NVME_ANA_NONOPTIMIZED)))
> +		found = old;
> +
> +	if (!found)
>   		return NULL;
> -	found = fallback;
> +
>   out:
>   	rcu_assign_pointer(head->current_path[node], found);
>   	return found;
> 
The problem is that we should have tested all paths from (old + 1)
up to and including (old); currently we're only testing paths from
(old + 1) up to, but excluding, (old).

I would rather use this explanation instead of referring to 'better' 
paths; at the very least please name it 'optimal'.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare@suse.de                               +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

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