[PATCH 0/2] nvme: multipath: round-robin fixes

[PATCH 0/2] nvme: multipath: round-robin fixes

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Hello Christoph, Sagi, Hannes,

The version of the patch set "nvme-multipath: fixes for non-optimized paths"
that was eventually merged fails for the case where just a single,
non-optimized path is available.
This mini-series fixes it, and simplifies the code somewhat.

Regards,
Martin

Martin Wilck (2):
  nvme: multipath: round-robin: fix single non-optimized path case
  nvme: multipath: round-robin: eliminate "fallback" variable

 drivers/nvme/host/multipath.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

-- 
2.28.0


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

[PATCH 1/2] nvme: multipath: round-robin: fix single non-optimized path case

[mwilck]

From: Martin Wilck <mwilck@suse.com>

If there's only one usable, non-optimized path, nvme_round_robin_path()
returns NULL, which is wrong. Fix it by falling back to "old", like in
the single optimized path case. Also, if the active path isn't changed,
there's no need to re-assign the pointer.

Fixes: 3f6e3246db0e ("nvme-multipath: fix logic for non-optimized paths")
Signed-off-by: Martin Wilck <mwilck@suse.com>
Reported-by: Martin George <marting@netapp.com>
---
 drivers/nvme/host/multipath.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 3ded54d2c9c6..a64dfff0d0ce 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -255,12 +255,17 @@ static struct nvme_ns *nvme_round_robin_path(struct nvme_ns_head *head,
 			fallback = ns;
 	}
 
-	/* No optimized path found, re-check the current path */
+	/*
+	 * The loop above skips the current path for round-robin semantics.
+	 * Fall back to the current path if either:
+	 *  - no other optimized path found and current is optimized,
+	 *  - no other usable path found and current is usable.
+	 */
 	if (!nvme_path_is_disabled(old) &&
-	    old->ana_state == NVME_ANA_OPTIMIZED) {
-		found = old;
-		goto out;
-	}
+	    (old->ana_state == NVME_ANA_OPTIMIZED ||
+	     (!fallback && old->ana_state == NVME_ANA_NONOPTIMIZED)))
+		return old;
+
 	if (!fallback)
 		return NULL;
 	found = fallback;
-- 
2.28.0


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

[PATCH 2/2] nvme: multipath: round-robin: eliminate "fallback" variable

[mwilck]

From: Martin Wilck <mwilck@suse.com>

If we find an optimized path, we quit the loop immediately. Thus we can use
just one variable for the next path, slighly simplifying the code.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 drivers/nvme/host/multipath.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index a64dfff0d0ce..760f625cefc8 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -233,7 +233,7 @@ 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))
@@ -252,7 +252,7 @@ static struct nvme_ns *nvme_round_robin_path(struct nvme_ns_head *head,
 			goto out;
 		}
 		if (ns->ana_state == NVME_ANA_NONOPTIMIZED)
-			fallback = ns;
+			found = ns;
 	}
 
 	/*
@@ -263,12 +263,11 @@ static struct nvme_ns *nvme_round_robin_path(struct nvme_ns_head *head,
 	 */
 	if (!nvme_path_is_disabled(old) &&
 	    (old->ana_state == NVME_ANA_OPTIMIZED ||
-	     (!fallback && old->ana_state == NVME_ANA_NONOPTIMIZED)))
+	     (!found && old->ana_state == NVME_ANA_NONOPTIMIZED)))
 		return old;
 
-	if (!fallback)
+	if (!found)
 		return NULL;
-	found = fallback;
 out:
 	rcu_assign_pointer(head->current_path[node], found);
 	return found;
-- 
2.28.0


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

Re: [PATCH 0/2] nvme: multipath: round-robin fixes

[Sagi Grimberg]

These look fine to me.

Can I get another set of eyes on this please?

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

Re: [PATCH 1/2] nvme: multipath: round-robin: fix single non-optimized path case

[Martin Wilck]

On Thu, 2020-08-06 at 15:19 +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> If there's only one usable, non-optimized path,
> nvme_round_robin_path()
> returns NULL, which is wrong. Fix it by falling back to "old", like
> in
> the single optimized path case. Also, if the active path isn't
> changed,
> there's no need to re-assign the pointer.
> 
> Fixes: 3f6e3246db0e ("nvme-multipath: fix logic for non-optimized
> paths")
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> Reported-by: Martin George <marting@netapp.com>

I made a mistake here. It should be

Signed-off-by: Martin George <marting@netapp.com>

because Martin G. provided the initial idea for the fix.



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

Re: [PATCH 0/2] nvme: multipath: round-robin fixes

[Keith Busch]

On Thu, Aug 06, 2020 at 12:16:28PM -0700, Sagi Grimberg wrote:
> These look fine to me.
> 
> Can I get another set of eyes on this please?

Looks correct to me.

Reviewed-by: Keith Busch <kbusch@kernel.org>

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

Re: [PATCH 1/2] nvme: multipath: round-robin: fix single non-optimized path case

[George, Martin]

On Thu, 2020-08-06 at 21:40 +0200, Martin Wilck wrote:
> On Thu, 2020-08-06 at 15:19 +0200, mwilck@suse.com wrote:
> > From: Martin Wilck <mwilck@suse.com>
> > 
> > If there's only one usable, non-optimized path,
> > nvme_round_robin_path()
> > returns NULL, which is wrong. Fix it by falling back to "old", like
> > in
> > the single optimized path case. Also, if the active path isn't
> > changed,
> > there's no need to re-assign the pointer.
> > 
> > Fixes: 3f6e3246db0e ("nvme-multipath: fix logic for non-optimized
> > paths")
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > Reported-by: Martin George <marting@netapp.com>
> 
> I made a mistake here. It should be
> 
> Signed-off-by: Martin George <marting@netapp.com>
> 
> because Martin G. provided the initial idea for the fix.
> 

Thanks MartinW. 

I've also tested this patch and verified that it works fine for all
scenarios including the single ANA non-optimized path scenario. So in
addition to MartinW's Signed-off-by tag, please add

Signed-off-by: Martin George <marting@netapp.com>
Tested-by: Martin George <marting@netapp.com>

as well for this patch. Thank you.

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