Re: [PATCH 2/2] nvme: multipath: round-robin: don't fall back to numa

[Hannes Reinecke <hare@suse.de>]

On 7/16/20 9:59 PM, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Currently, if the RR path selector returns a non-optimized path,
> we fall back to __nvme_find_path(), which uses the logic of the
> numa path selector. For a given numa node, this always chooses
> the same path, thus preventing round-robin logic on non-optimized
> paths.
> 
> By handling the situation where the current ns is NULL in
> nvme_round_robin_path(), we can avoid falling back from round-robin
> to NUMA, fixing the issue. The iopolicy case distinction in
> __nvme_find_path() can be skipped now.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>   drivers/nvme/host/multipath.c | 23 +++++++++++++++--------
>   1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index 2c575b783d3e..ff93bab0d549 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -181,10 +181,7 @@ static struct nvme_ns *__nvme_find_path(struct nvme_ns_head *head, int node)
>   		if (nvme_path_is_disabled(ns))
>   			continue;
>   
> -		if (READ_ONCE(head->subsys->iopolicy) == NVME_IOPOLICY_NUMA)
> -			distance = node_distance(node, ns->ctrl->numa_node);
> -		else
> -			distance = LOCAL_DISTANCE;
> +		distance = node_distance(node, ns->ctrl->numa_node);
>   
>   		switch (ns->ana_state) {
>   		case NVME_ANA_OPTIMIZED:
> @@ -225,7 +222,13 @@ static struct nvme_ns *nvme_round_robin_path(struct nvme_ns_head *head,
>   		int node, struct nvme_ns *old)
>   {
>   	struct nvme_ns *ns, *found = NULL;
> +	bool was_null = (old == NULL);
>   
> +	if (unlikely(was_null))
> +		old = list_first_or_null_rcu(&head->list,
> +					     struct nvme_ns, siblings);
> +	if (unlikely(!old))
> +		return NULL;
>   
>   	for (ns = nvme_next_ns(head, old);
>   	     ns != old;
> @@ -244,9 +247,12 @@ static struct nvme_ns *nvme_round_robin_path(struct nvme_ns_head *head,
>   	/* 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->ana_state == NVME_ANA_NONOPTIMIZED))) {
>   		found = old;
> -
> +		if (!was_null)
> +			/* No need to switch */
> +			return found;
> +	}
>   	if (!found)
>   		return NULL;
>   
> @@ -267,8 +273,9 @@ inline struct nvme_ns *nvme_find_path(struct nvme_ns_head *head)
>   	struct nvme_ns *ns;
>   
>   	ns = srcu_dereference(head->current_path[node], &head->srcu);
> -	if (READ_ONCE(head->subsys->iopolicy) == NVME_IOPOLICY_RR && ns)
> -		ns = nvme_round_robin_path(head, node, ns);
> +	if (READ_ONCE(head->subsys->iopolicy) == NVME_IOPOLICY_RR)
> +		return nvme_round_robin_path(head, node, ns);
> +
>   	if (unlikely(!ns || !nvme_path_is_optimized(ns)))
>   		ns = __nvme_find_path(head, node);
>   	return ns;
> 
Why not modify the last if clause to just

if (unlikely(!ns))

?

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