[PATCH RESEND 0/3] nvme/mpath: fix missed namespaces in ana state update

[PATCH RESEND 0/3] nvme/mpath: fix missed namespaces in ana state update

[Anton Eidelman]

Fixed two issues in nvme_update_ana_state() that caused ana_work
to miss existing namespaces and consequently a failure to update
the namespace ANA state based on the ANA log page.

1) A plain bug: we skipped an nsid in desc->nsids in a certain
   combination of nsids present and nsids reports in the ANA log,
   and failed to match this nsid to an existing namespace.
2) Unhandled situation when scan_work appended new namespaces to
   ctrl->namespaces and did not sort the list yet.
   In such transient state ana_work would fail to match nsids
   to those new namespaces.

Both issues potentially caused some namespaces to get stuck
in an incorrect ANA state, e.g. to never become live.

Anton Eidelman (3):
  nvme/multipath: fix failure to update ns ana state
  nvme/multipath: cosmetic: keep ns nsid locally
  nvme/multipath: fix stale ana state for namespaces just added by scan
    work

 drivers/nvme/host/multipath.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

-- 
2.25.1


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

[PATCH 1/3] nvme/multipath: fix failure to update ns ana state

[Anton Eidelman]

nvme_update_ana_state() has a deficiency that results
in failure to update the ana state for a namespace
in the following case:
nsid's in ctrl->namespaces: 1, 3, 4
nsid's in desc->nsids: 1, 2, 3, 4

Loop iteration 0:
    ns index = 0, n = 0, ns->head->ns_id = 1, nsid = 1, MATCH.
Loop iteration 1:
    ns index = 1, n = 1, ns->head->ns_id = 3, nsid = 2, NO MATCH.
Loop iteration 2:
    ns index = 2, n = 2, ns->head->ns_id = 4, nsid = 4, MATCH.

Result: missed nsid=3 and did not update its ana state.

Solution: when ns->head->ns_id is higher than nsid,
increment n and RETRY with the SAME ns.

Signed-off-by: Anton Eidelman <anton@lightbitslabs.com>
---
 drivers/nvme/host/multipath.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 5d7bc58a27bd..e8ccdd398f78 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -600,14 +600,17 @@ static int nvme_update_ana_state(struct nvme_ctrl *ctrl,
 
 	down_read(&ctrl->namespaces_rwsem);
 	list_for_each_entry(ns, &ctrl->namespaces, list) {
-		unsigned nsid = le32_to_cpu(desc->nsids[n]);
-
+		unsigned nsid;
+again:
+		nsid = le32_to_cpu(desc->nsids[n]);
 		if (ns->head->ns_id < nsid)
 			continue;
 		if (ns->head->ns_id == nsid)
 			nvme_update_ns_ana_state(desc, ns);
 		if (++n == nr_nsids)
 			break;
+		if (ns->head->ns_id > nsid)
+			goto again;
 	}
 	up_read(&ctrl->namespaces_rwsem);
 	return 0;
-- 
2.25.1


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

[PATCH 2/3] nvme/multipath: cosmetic: keep ns nsid locally

[Anton Eidelman]

Keep the nsid of the current namespace in a local variable.
Change the type to unsigned int to make checkpatch happy.

Signed-off-by: Anton Eidelman <anton@lightbitslabs.com>
---
 drivers/nvme/host/multipath.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index e8ccdd398f78..a51561d67b93 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -600,16 +600,18 @@ static int nvme_update_ana_state(struct nvme_ctrl *ctrl,
 
 	down_read(&ctrl->namespaces_rwsem);
 	list_for_each_entry(ns, &ctrl->namespaces, list) {
-		unsigned nsid;
+		unsigned int nsid;
+		unsigned int ns_nsid = ns->head->ns_id;
+
 again:
 		nsid = le32_to_cpu(desc->nsids[n]);
-		if (ns->head->ns_id < nsid)
+		if (ns_nsid < nsid)
 			continue;
-		if (ns->head->ns_id == nsid)
+		if (ns_nsid == nsid)
 			nvme_update_ns_ana_state(desc, ns);
 		if (++n == nr_nsids)
 			break;
-		if (ns->head->ns_id > nsid)
+		if (ns_nsid > nsid)
 			goto again;
 	}
 	up_read(&ctrl->namespaces_rwsem);
-- 
2.25.1


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

[PATCH 3/3] nvme/multipath: fix stale ana state for namespaces just added by scan work

[Anton Eidelman]

Scan work initially adds new namespaces to ctrl->namespaces TAIL.
They make the list unordered temporarily until nvme_scan_work()
finally sorts the list.

In case nvme_update_ana_state() runs while the list is unsorted,
the recently added namespaces are missed and their ana state
may remain not updated forever if timing between scan work and ana work
is unfortunate, e.g.
Initial state: namespaces = {2, 3}
scan_work: adds nsid=1: namespaces = {2, 3, 1}
scan_work: finds nsid=1 is still Inaccessible
ana_work: log page has nsids = {1, 2, 3, 4}, all Optimized.
ana_work: updates nsids {2, 3} but fails to find nsid=1 in namespaces.
scan_work: adds nsid=4: namespaces = {2, 3, 1, 4}
scan_work: finds nsid=4 is Optimized: sets it live.
scan_work: completes an sorts namespaces = {1, 2, 3, 4}
Result: nsid=1 will remain in Inaccessible state.

Solution:
In order to preserve the way ctrl->namespaces is updated and sorted,
make nvme_update_ana_state() deal with the case where ctrl->namespaces
is not fully sorted and has new namespaces appended with potentially
lower nsids.
nvme_update_ana_state() keeps track of the nsid seen in the list,
detects the unsorted case (rare), and restarts scanning of desc->nsids.

Signed-off-by: Anton Eidelman <anton@lightbitslabs.com>
---
 drivers/nvme/host/multipath.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index a51561d67b93..1ad8dc8adb86 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -587,6 +587,7 @@ static int nvme_update_ana_state(struct nvme_ctrl *ctrl,
 	u32 nr_nsids = le32_to_cpu(desc->nnsids), n = 0;
 	unsigned *nr_change_groups = data;
 	struct nvme_ns *ns;
+	unsigned int last_ns_nsid = 0;
 
 	dev_dbg(ctrl->device, "ANA group %d: %s.\n",
 			le32_to_cpu(desc->grpid),
@@ -603,6 +604,11 @@ static int nvme_update_ana_state(struct nvme_ctrl *ctrl,
 		unsigned int nsid;
 		unsigned int ns_nsid = ns->head->ns_id;
 
+		if (ns_nsid < last_ns_nsid) {
+			/* Detected unsorted ctrl->namespaces: re-scan desc->nsids */
+			last_ns_nsid = ns_nsid;
+			n = 0;
+		}
 again:
 		nsid = le32_to_cpu(desc->nsids[n]);
 		if (ns_nsid < nsid)
-- 
2.25.1


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

[PATCH v2 0/3] nvme/mpath: fix missed namespaces in ana state update

[Anton Eidelman]

Fixed two issues in nvme_update_ana_state() that caused ana_work
to miss existing namespaces and consequently a failure to update
the namespace ANA state based on the ANA log page.

1) A plain bug: we skipped an nsid in desc->nsids in a certain
   combination of nsids present and nsids reports in the ANA log,
   and failed to match this nsid to an existing namespace.
2) Unhandled situation when scan_work appended new namespaces to
   ctrl->namespaces and did not sort the list yet.
   In such transient state ana_work would fail to match nsids
   to those new namespaces.

Both issues potentially caused some namespaces to get stuck
in an incorrect ANA state, e.g. to never become live.

Anton Eidelman (3):
  nvme/multipath: fix failure to update ns ana state
  nvme/multipath: cosmetic: keep ns nsid locally
  nvme/multipath: fix stale ana state for namespaces just added by scan
    work

 drivers/nvme/host/multipath.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

-- 
2.25.1


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

[PATCH v2 1/3] nvme/multipath: fix failure to update ns ana state

[Anton Eidelman]

nvme_update_ana_state() has a deficiency that results
in failure to update the ana state for a namespace
in the following case:
nsid's in ctrl->namespaces: 1, 3, 4
nsid's in desc->nsids: 1, 2, 3, 4

Loop iteration 0:
    ns index = 0, n = 0, ns->head->ns_id = 1, nsid = 1, MATCH.
Loop iteration 1:
    ns index = 1, n = 1, ns->head->ns_id = 3, nsid = 2, NO MATCH.
Loop iteration 2:
    ns index = 2, n = 2, ns->head->ns_id = 4, nsid = 4, MATCH.

Result: missed nsid=3 and did not update its ana state.

Solution: when ns->head->ns_id is higher than nsid,
increment n and RETRY with the SAME ns.

Signed-off-by: Anton Eidelman <anton@lightbitslabs.com>
---
 drivers/nvme/host/multipath.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 5d7bc58a27bd..e8ccdd398f78 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -600,14 +600,17 @@ static int nvme_update_ana_state(struct nvme_ctrl *ctrl,
 
 	down_read(&ctrl->namespaces_rwsem);
 	list_for_each_entry(ns, &ctrl->namespaces, list) {
-		unsigned nsid = le32_to_cpu(desc->nsids[n]);
-
+		unsigned nsid;
+again:
+		nsid = le32_to_cpu(desc->nsids[n]);
 		if (ns->head->ns_id < nsid)
 			continue;
 		if (ns->head->ns_id == nsid)
 			nvme_update_ns_ana_state(desc, ns);
 		if (++n == nr_nsids)
 			break;
+		if (ns->head->ns_id > nsid)
+			goto again;
 	}
 	up_read(&ctrl->namespaces_rwsem);
 	return 0;
-- 
2.25.1


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

Re: [PATCH v2 1/3] nvme/multipath: fix failure to update ns ana state

[Christoph Hellwig]

I kinda hate the goto, but couldn't come up with anything better,
so this looks good.

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

Re: [PATCH v2 1/3] nvme/multipath: fix failure to update ns ana state

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi@grimberg.me>

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

Re: [PATCH v2 1/3] nvme/multipath: fix failure to update ns ana state

[Christoph Hellwig]

I've applied this patch for now while we're still pondering the sorting
of the namespace list.

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

[PATCH v2 2/3] nvme/multipath: cosmetic: keep ns nsid locally

[Anton Eidelman]

Keep the nsid of the current namespace in a local variable.
Change the type to unsigned int to make checkpatch happy.

Signed-off-by: Anton Eidelman <anton@lightbitslabs.com>
---
 drivers/nvme/host/multipath.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index e8ccdd398f78..a51561d67b93 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -600,16 +600,18 @@ static int nvme_update_ana_state(struct nvme_ctrl *ctrl,
 
 	down_read(&ctrl->namespaces_rwsem);
 	list_for_each_entry(ns, &ctrl->namespaces, list) {
-		unsigned nsid;
+		unsigned int nsid;
+		unsigned int ns_nsid = ns->head->ns_id;
+
 again:
 		nsid = le32_to_cpu(desc->nsids[n]);
-		if (ns->head->ns_id < nsid)
+		if (ns_nsid < nsid)
 			continue;
-		if (ns->head->ns_id == nsid)
+		if (ns_nsid == nsid)
 			nvme_update_ns_ana_state(desc, ns);
 		if (++n == nr_nsids)
 			break;
-		if (ns->head->ns_id > nsid)
+		if (ns_nsid > nsid)
 			goto again;
 	}
 	up_read(&ctrl->namespaces_rwsem);
-- 
2.25.1


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

Re: [PATCH v2 2/3] nvme/multipath: cosmetic: keep ns nsid locally

[Christoph Hellwig]

On Mon, Sep 13, 2021 at 09:30:22AM -0600, Anton Eidelman wrote:
> Keep the nsid of the current namespace in a local variable.
> Change the type to unsigned int to make checkpatch happy.
> 
> Signed-off-by: Anton Eidelman <anton@lightbitslabs.com>

I can't really see any improvement here.  Also checkpath as so often
these days is fundamentally wrong here.

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

Re: [PATCH v2 2/3] nvme/multipath: cosmetic: keep ns nsid locally

[Anton Eidelman]

On Mon, Sep 13, 2021 at 06:02:20PM +0200, Christoph Hellwig wrote:
> On Mon, Sep 13, 2021 at 09:30:22AM -0600, Anton Eidelman wrote:
> > Keep the nsid of the current namespace in a local variable.
> > Change the type to unsigned int to make checkpatch happy.
> > 
> > Signed-off-by: Anton Eidelman <anton@lightbitslabs.com>
> 
> I can't really see any improvement here.  Also checkpath as so often
> these days is fundamentally wrong here.

Do we you prefer to scrap this commit?

I just wanted to get rid of multiple occurances of "ns->head->ns_id",
and just use a local variable that only changes
once in the for_each iteration.

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

Re: [PATCH v2 2/3] nvme/multipath: cosmetic: keep ns nsid locally

[Keith Busch]

On Mon, Sep 13, 2021 at 02:19:35PM -0600, Anton Eidelman wrote:
> On Mon, Sep 13, 2021 at 06:02:20PM +0200, Christoph Hellwig wrote:
> > On Mon, Sep 13, 2021 at 09:30:22AM -0600, Anton Eidelman wrote:
> > > Keep the nsid of the current namespace in a local variable.
> > > Change the type to unsigned int to make checkpatch happy.
> > > 
> > > Signed-off-by: Anton Eidelman <anton@lightbitslabs.com>
> > 
> > I can't really see any improvement here.  Also checkpath as so often
> > these days is fundamentally wrong here.
> 
> Do we you prefer to scrap this commit?
> 
> I just wanted to get rid of multiple occurances of "ns->head->ns_id",
> and just use a local variable that only changes
> once in the for_each iteration.

I personally think "unsigned" is a well defined type in C and we should
ignore check-patch complaints on its use. Otherwise, I don't have a
problem with the patch.

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

Re: [PATCH v2 2/3] nvme/multipath: cosmetic: keep ns nsid locally

[Christoph Hellwig]

On Mon, Sep 13, 2021 at 01:57:17PM -0700, Keith Busch wrote:
> > > I can't really see any improvement here.  Also checkpath as so often
> > > these days is fundamentally wrong here.
> > 
> > Do we you prefer to scrap this commit?
> > 
> > I just wanted to get rid of multiple occurances of "ns->head->ns_id",
> > and just use a local variable that only changes
> > once in the for_each iteration.
> 
> I personally think "unsigned" is a well defined type in C and we should
> ignore check-patch complaints on its use.

Agreed.  If we want to cleanup nsids (and at this point I don't), we
should move them all over to u32 instead..

> Otherwise, I don't have a
> problem with the patch.

Well, it is obviously correct, but I also don't see the point of it.

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

[PATCH v2 3/3] nvme/multipath: fix stale ana state for namespaces just added by scan work

[Anton Eidelman]

Scan work initially adds new namespaces to ctrl->namespaces TAIL.
They make the list unordered temporarily until nvme_scan_work()
finally sorts the list.

In case nvme_update_ana_state() runs while the list is unsorted,
the recently added namespaces are missed and their ana state
may remain not updated forever if timing between scan work and ana work
is unfortunate, e.g.
Initial state: namespaces = {2, 3}
scan_work: adds nsid=1: namespaces = {2, 3, 1}
scan_work: finds nsid=1 is still Inaccessible
ana_work: log page has nsids = {1, 2, 3, 4}, all Optimized.
ana_work: updates nsids {2, 3} but fails to find nsid=1 in namespaces.
scan_work: adds nsid=4: namespaces = {2, 3, 1, 4}
scan_work: finds nsid=4 is Optimized: sets it live.
scan_work: completes an sorts namespaces = {1, 2, 3, 4}
Result: nsid=1 will remain in Inaccessible state.

Solution:
In order to preserve the way ctrl->namespaces is updated and sorted,
make nvme_update_ana_state() deal with the case where ctrl->namespaces
is not fully sorted and has new namespaces appended with potentially
lower nsids.
nvme_update_ana_state() keeps track of the nsid seen in the list,
detects the unsorted case (rare), and restarts scanning of desc->nsids.

Signed-off-by: Anton Eidelman <anton@lightbitslabs.com>
---
 drivers/nvme/host/multipath.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index a51561d67b93..0a50439a8c41 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -587,6 +587,7 @@ static int nvme_update_ana_state(struct nvme_ctrl *ctrl,
 	u32 nr_nsids = le32_to_cpu(desc->nnsids), n = 0;
 	unsigned *nr_change_groups = data;
 	struct nvme_ns *ns;
+	unsigned int last_ns_nsid = 0;
 
 	dev_dbg(ctrl->device, "ANA group %d: %s.\n",
 			le32_to_cpu(desc->grpid),
@@ -603,6 +604,11 @@ static int nvme_update_ana_state(struct nvme_ctrl *ctrl,
 		unsigned int nsid;
 		unsigned int ns_nsid = ns->head->ns_id;
 
+		if (ns_nsid < last_ns_nsid) {
+			/* Unsorted ctrl->namespaces: re-scan desc->nsids */
+			n = 0;
+		}
+		last_ns_nsid = ns_nsid;
 again:
 		nsid = le32_to_cpu(desc->nsids[n]);
 		if (ns_nsid < nsid)
-- 
2.25.1


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

Re: [PATCH v2 3/3] nvme/multipath: fix stale ana state for namespaces just added by scan work

[Christoph Hellwig]

On Mon, Sep 13, 2021 at 09:30:23AM -0600, Anton Eidelman wrote:
> Scan work initially adds new namespaces to ctrl->namespaces TAIL.
> They make the list unordered temporarily until nvme_scan_work()
> finally sorts the list.
> 
> Solution:
> In order to preserve the way ctrl->namespaces is updated and sorted,
> make nvme_update_ana_state() deal with the case where ctrl->namespaces
> is not fully sorted and has new namespaces appended with potentially
> lower nsids.
> nvme_update_ana_state() keeps track of the nsid seen in the list,
> detects the unsorted case (rare), and restarts scanning of desc->nsids.

Can we dust off the patch from Chaitanya to switch to an always sorted
xarray instead of papering over this delayed sort?

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

Re: [PATCH v2 3/3] nvme/multipath: fix stale ana state for namespaces just added by scan work

[Keith Busch]

On Mon, Sep 13, 2021 at 06:03:10PM +0200, Christoph Hellwig wrote:
> On Mon, Sep 13, 2021 at 09:30:23AM -0600, Anton Eidelman wrote:
> > Scan work initially adds new namespaces to ctrl->namespaces TAIL.
> > They make the list unordered temporarily until nvme_scan_work()
> > finally sorts the list.
> > 
> > Solution:
> > In order to preserve the way ctrl->namespaces is updated and sorted,
> > make nvme_update_ana_state() deal with the case where ctrl->namespaces
> > is not fully sorted and has new namespaces appended with potentially
> > lower nsids.
> > nvme_update_ana_state() keeps track of the nsid seen in the list,
> > detects the unsorted case (rare), and restarts scanning of desc->nsids.
> 
> Can we dust off the patch from Chaitanya to switch to an always sorted
> xarray instead of papering over this delayed sort?

Yeah, that's what I was going to say. :)

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

Re: [PATCH v2 3/3] nvme/multipath: fix stale ana state for namespaces just added by scan work

[Sagi Grimberg]

>>> Scan work initially adds new namespaces to ctrl->namespaces TAIL.
>>> They make the list unordered temporarily until nvme_scan_work()
>>> finally sorts the list.
>>>
>>> Solution:
>>> In order to preserve the way ctrl->namespaces is updated and sorted,
>>> make nvme_update_ana_state() deal with the case where ctrl->namespaces
>>> is not fully sorted and has new namespaces appended with potentially
>>> lower nsids.
>>> nvme_update_ana_state() keeps track of the nsid seen in the list,
>>> detects the unsorted case (rare), and restarts scanning of desc->nsids.
>>
>> Can we dust off the patch from Chaitanya to switch to an always sorted
>> xarray instead of papering over this delayed sort?
> 
> Yeah, that's what I was going to say. :)

I agree it should not happen with xarray as inserts preserve sorting,
but that patch has the non-trivial removal of the namespaces_rwsem which
synchronizes in a lot in a lot of error recovery flows. It's very
possible that xarray will introduce regressions.

Anton, is it possible to rerun tests with the xarray patch applied?
The latest one is:
[PATCH V6] nvme-core: use xarray for ctrl ns tracking

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

Re: [PATCH v2 3/3] nvme/multipath: fix stale ana state for namespaces just added by scan work

[Christoph Hellwig]

On Tue, Sep 14, 2021 at 09:24:30AM +0300, Sagi Grimberg wrote:
> I agree it should not happen with xarray as inserts preserve sorting,
> but that patch has the non-trivial removal of the namespaces_rwsem which
> synchronizes in a lot in a lot of error recovery flows. It's very
> possible that xarray will introduce regressions.

I've actually started looking over the series earlier today, and staring
it I'm pretty sure the namespaces_rwsem is completely broken.  There
is nothing preventing the namespaces from going away in these loops.

But I can't see why we don't just always do a sorted insertation into
the ->namespaces list.  Just trying to figure out the history of the
delayed sort ATM.

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

Re: [PATCH v2 3/3] nvme/multipath: fix stale ana state for namespaces just added by scan work

[Christoph Hellwig]

On Tue, Sep 14, 2021 at 08:30:52AM +0200, Christoph Hellwig wrote:
> On Tue, Sep 14, 2021 at 09:24:30AM +0300, Sagi Grimberg wrote:
> > I agree it should not happen with xarray as inserts preserve sorting,
> > but that patch has the non-trivial removal of the namespaces_rwsem which
> > synchronizes in a lot in a lot of error recovery flows. It's very
> > possible that xarray will introduce regressions.
> 
> I've actually started looking over the series earlier today, and staring
> it I'm pretty sure the namespaces_rwsem is completely broken.  There
> is nothing preventing the namespaces from going away in these loops.
> 
> But I can't see why we don't just always do a sorted insertation into
> the ->namespaces list.  Just trying to figure out the history of the
> delayed sort ATM.

Anton, can you try this instead of your last patch?


diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 97f8211cf92c1..3e3c835777458 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -13,7 +13,6 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/backing-dev.h>
-#include <linux/list_sort.h>
 #include <linux/slab.h>
 #include <linux/types.h>
 #include <linux/pr.h>
@@ -3716,15 +3715,6 @@ static int nvme_init_ns_head(struct nvme_ns *ns, unsigned nsid,
 	return ret;
 }
 
-static int ns_cmp(void *priv, const struct list_head *a,
-		const struct list_head *b)
-{
-	struct nvme_ns *nsa = container_of(a, struct nvme_ns, list);
-	struct nvme_ns *nsb = container_of(b, struct nvme_ns, list);
-
-	return nsa->head->ns_id - nsb->head->ns_id;
-}
-
 struct nvme_ns *nvme_find_get_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 {
 	struct nvme_ns *ns, *ret = NULL;
@@ -3745,6 +3735,23 @@ struct nvme_ns *nvme_find_get_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 }
 EXPORT_SYMBOL_NS_GPL(nvme_find_get_ns, NVME_TARGET_PASSTHRU);
 
+/*
+ * Add the namespace to the controller list while keeping the list ordered.
+ */
+static void nvme_ns_add_to_ctrl_list(struct nvme_ns *ns)
+{
+	struct nvme_ns *tmp;
+
+	list_for_each_entry_reverse(tmp, &ns->ctrl->namespaces, list) {
+		if (tmp->head->ns_id < ns->head->ns_id) {
+			list_add(&tmp->list, &ns->ctrl->namespaces);
+			return;
+		}
+	}
+
+	list_add_tail(&ns->list, &ns->ctrl->namespaces);
+}
+
 static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
 		struct nvme_ns_ids *ids)
 {
@@ -3794,10 +3801,9 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
 	if (nvme_update_ns_info(ns, id))
 		goto out_unlink_ns;
 
-	down_write(&ctrl->namespaces_rwsem);
-	list_add_tail(&ns->list, &ctrl->namespaces);
-	up_write(&ctrl->namespaces_rwsem);
-
+	down_write(&ns->ctrl->namespaces_rwsem);
+	nvme_ns_add_to_ctrl_list(ns);
+	up_write(&ns->ctrl->namespaces_rwsem);
 	nvme_get_ctrl(ctrl);
 
 	if (device_add_disk(ctrl->device, ns->disk, nvme_ns_id_attr_groups))
@@ -4082,10 +4088,6 @@ static void nvme_scan_work(struct work_struct *work)
 	if (nvme_scan_ns_list(ctrl) != 0)
 		nvme_scan_ns_sequential(ctrl);
 	mutex_unlock(&ctrl->scan_lock);
-
-	down_write(&ctrl->namespaces_rwsem);
-	list_sort(NULL, &ctrl->namespaces, ns_cmp);
-	up_write(&ctrl->namespaces_rwsem);
 }
 
 /*

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

Re: [PATCH v2 3/3] nvme/multipath: fix stale ana state for namespaces just added by scan work

[Sagi Grimberg]

>> I agree it should not happen with xarray as inserts preserve sorting,
>> but that patch has the non-trivial removal of the namespaces_rwsem which
>> synchronizes in a lot in a lot of error recovery flows. It's very
>> possible that xarray will introduce regressions.
> 
> I've actually started looking over the series earlier today, and staring
> it I'm pretty sure the namespaces_rwsem is completely broken.  There
> is nothing preventing the namespaces from going away in these loops.

Where? the ns itself? the ns is removed from ctrl->namespaces list under
this rwsem (nvme_ns_remove).

> But I can't see why we don't just always do a sorted insertation into
> the ->namespaces list.  Just trying to figure out the history of the
> delayed sort ATM.

Don't know the history.

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

Re: [PATCH v2 3/3] nvme/multipath: fix stale ana state for namespaces just added by scan work

[Christoph Hellwig]

On Tue, Sep 14, 2021 at 10:09:43AM +0300, Sagi Grimberg wrote:
>
>>> I agree it should not happen with xarray as inserts preserve sorting,
>>> but that patch has the non-trivial removal of the namespaces_rwsem which
>>> synchronizes in a lot in a lot of error recovery flows. It's very
>>> possible that xarray will introduce regressions.
>>
>> I've actually started looking over the series earlier today, and staring
>> it I'm pretty sure the namespaces_rwsem is completely broken.  There
>> is nothing preventing the namespaces from going away in these loops.
>
> Where? the ns itself? the ns is removed from ctrl->namespaces list under
> this rwsem (nvme_ns_remove).

Sorry, with the series I meant the xarray conversion that removes the
rwsem entirely.

>> But I can't see why we don't just always do a sorted insertation into
>> the ->namespaces list.  Just trying to figure out the history of the
>> delayed sort ATM.
>
> Don't know the history.

git-blame teels me it showed up with the initial ID ns list support
from Keith, so not all that much history here.

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

Re: [PATCH v2 3/3] nvme/multipath: fix stale ana state for namespaces just added by scan work

[Anton Eidelman]

On Tue, Sep 14, 2021 at 09:32:36AM +0200, Christoph Hellwig wrote:
> On Tue, Sep 14, 2021 at 10:09:43AM +0300, Sagi Grimberg wrote:
> >
> >>> I agree it should not happen with xarray as inserts preserve sorting,
> >>> but that patch has the non-trivial removal of the namespaces_rwsem which
> >>> synchronizes in a lot in a lot of error recovery flows. It's very
> >>> possible that xarray will introduce regressions.
> >>
> >> I've actually started looking over the series earlier today, and staring
> >> it I'm pretty sure the namespaces_rwsem is completely broken.  There
> >> is nothing preventing the namespaces from going away in these loops.
> >
> > Where? the ns itself? the ns is removed from ctrl->namespaces list under
> > this rwsem (nvme_ns_remove).
> 
> Sorry, with the series I meant the xarray conversion that removes the
> rwsem entirely.
> 
> >> But I can't see why we don't just always do a sorted insertation into
> >> the ->namespaces list.  Just trying to figure out the history of the
> >> delayed sort ATM.
> >
> > Don't know the history.
> 
> git-blame teels me it showed up with the initial ID ns list support
> from Keith, so not all that much history here.

The main reason I suggest applying this as a short term fix
is that it is well isolated and is easy to cherry-pick into the LTS/stable versions.

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

Re: [PATCH v2 3/3] nvme/multipath: fix stale ana state for namespaces just added by scan work

[Sagi Grimberg]

>>>>> I agree it should not happen with xarray as inserts preserve sorting,
>>>>> but that patch has the non-trivial removal of the namespaces_rwsem which
>>>>> synchronizes in a lot in a lot of error recovery flows. It's very
>>>>> possible that xarray will introduce regressions.
>>>>
>>>> I've actually started looking over the series earlier today, and staring
>>>> it I'm pretty sure the namespaces_rwsem is completely broken.  There
>>>> is nothing preventing the namespaces from going away in these loops.
>>>
>>> Where? the ns itself? the ns is removed from ctrl->namespaces list under
>>> this rwsem (nvme_ns_remove).
>>
>> Sorry, with the series I meant the xarray conversion that removes the
>> rwsem entirely.
>>
>>>> But I can't see why we don't just always do a sorted insertation into
>>>> the ->namespaces list.  Just trying to figure out the history of the
>>>> delayed sort ATM.
>>>
>>> Don't know the history.
>>
>> git-blame teels me it showed up with the initial ID ns list support
>> from Keith, so not all that much history here.
> 
> The main reason I suggest applying this as a short term fix
> is that it is well isolated and is easy to cherry-pick into the LTS/stable versions.

I think that Christoph's version of keeping the list sorted is pretty
self-contained and can probably apply on stable kernels.

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

Re: [PATCH v2 3/3] nvme/multipath: fix stale ana state for namespaces just added by scan work

[Keith Busch]

On Tue, Sep 14, 2021 at 08:30:52AM +0200, Christoph Hellwig wrote:
> On Tue, Sep 14, 2021 at 09:24:30AM +0300, Sagi Grimberg wrote:
> > I agree it should not happen with xarray as inserts preserve sorting,
> > but that patch has the non-trivial removal of the namespaces_rwsem which
> > synchronizes in a lot in a lot of error recovery flows. It's very
> > possible that xarray will introduce regressions.
> 
> I've actually started looking over the series earlier today, and staring
> it I'm pretty sure the namespaces_rwsem is completely broken.  There
> is nothing preventing the namespaces from going away in these loops.
> 
> But I can't see why we don't just always do a sorted insertation into
> the ->namespaces list.  Just trying to figure out the history of the
> delayed sort ATM.

There doesn't appear to be any reason for the delayed list sort. It
looks correct to do a ordered list insert from the beginning.

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

Re: [PATCH v2 3/3] nvme/multipath: fix stale ana state for namespaces just added by scan work

[Keith Busch]

On Mon, Sep 13, 2021 at 09:30:23AM -0600, Anton Eidelman wrote:
> Scan work initially adds new namespaces to ctrl->namespaces TAIL.
> They make the list unordered temporarily until nvme_scan_work()
> finally sorts the list.
> 
> In case nvme_update_ana_state() runs while the list is unsorted,
> the recently added namespaces are missed and their ana state
> may remain not updated forever if timing between scan work and ana work
> is unfortunate, e.g.
> Initial state: namespaces = {2, 3}
> scan_work: adds nsid=1: namespaces = {2, 3, 1}
> scan_work: finds nsid=1 is still Inaccessible
> ana_work: log page has nsids = {1, 2, 3, 4}, all Optimized.
> ana_work: updates nsids {2, 3} but fails to find nsid=1 in namespaces.
> scan_work: adds nsid=4: namespaces = {2, 3, 1, 4}
> scan_work: finds nsid=4 is Optimized: sets it live.
> scan_work: completes an sorts namespaces = {1, 2, 3, 4}
> Result: nsid=1 will remain in Inaccessible state.
> 
> Solution:
> In order to preserve the way ctrl->namespaces is updated and sorted,
> make nvme_update_ana_state() deal with the case where ctrl->namespaces
> is not fully sorted and has new namespaces appended with potentially
> lower nsids.
> nvme_update_ana_state() keeps track of the nsid seen in the list,
> detects the unsorted case (rare), and restarts scanning of desc->nsids.
> 
> Signed-off-by: Anton Eidelman <anton@lightbitslabs.com>

I guess this works for the structures we have in place, but I think we
should replace the sorted list and with xarray. Then you can get
something simpler to follow like:

---
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 5d7bc58a27bd..2b4bd624bded 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -595,21 +595,15 @@ static int nvme_update_ana_state(struct nvme_ctrl *ctrl,
 	if (desc->state == NVME_ANA_CHANGE)
 		(*nr_change_groups)++;
 
-	if (!nr_nsids)
-		return 0;
-
-	down_read(&ctrl->namespaces_rwsem);
-	list_for_each_entry(ns, &ctrl->namespaces, list) {
-		unsigned nsid = le32_to_cpu(desc->nsids[n]);
+	while (n < nr_nsids) {
+		unsigned nsid = le32_to_cpu(desc->nsids[n++]);
 
-		if (ns->head->ns_id < nsid)
-			continue;
-		if (ns->head->ns_id == nsid)
+		ns = nvme_find_get_ns(ctrl, nsid);
+		if (ns) {
 			nvme_update_ns_ana_state(desc, ns);
-		if (++n == nr_nsids)
-			break;
+			nvme_put_ns(ns);
+		}
 	}
-	up_read(&ctrl->namespaces_rwsem);
 	return 0;
 }
 
--

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

Re: [PATCH v2 3/3] nvme/multipath: fix stale ana state for namespaces just added by scan work

[Christoph Hellwig]

On Mon, Sep 13, 2021 at 09:07:56AM -0700, Keith Busch wrote:
> I guess this works for the structures we have in place, but I think we
> should replace the sorted list and with xarray. Then you can get
> something simpler to follow like:

I guess that's fine for the short term.  It mean we'll have to walk
the list for every namespace in the decriptor, but I've not actually
seen deployments with crazy numbers of namespaces yet.

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

Re: [PATCH v2 3/3] nvme/multipath: fix stale ana state for namespaces just added by scan work

[Keith Busch]

On Mon, Sep 13, 2021 at 06:11:04PM +0200, Christoph Hellwig wrote:
> On Mon, Sep 13, 2021 at 09:07:56AM -0700, Keith Busch wrote:
> > I guess this works for the structures we have in place, but I think we
> > should replace the sorted list and with xarray. Then you can get
> > something simpler to follow like:
> 
> I guess that's fine for the short term.  It mean we'll have to walk
> the list for every namespace in the decriptor, but I've not actually
> seen deployments with crazy numbers of namespaces yet.

Oh, my suggestion depends on using the xarray implementation since the
current nvme_find_get_ns() also depends on a sorted list. Lookup
time with an xarray doesn't change with the number of namespaces, so
even a controller with lots of them should be fine if someone wants to make
such a device.

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

Re: [PATCH v2 3/3] nvme/multipath: fix stale ana state for namespaces just added by scan work

[Anton Eidelman]

On Mon, Sep 13, 2021 at 06:11:04PM +0200, Christoph Hellwig wrote:
> On Mon, Sep 13, 2021 at 09:07:56AM -0700, Keith Busch wrote:
> > I guess this works for the structures we have in place, but I think we
> > should replace the sorted list and with xarray. Then you can get
> > something simpler to follow like:
> 
> I guess that's fine for the short term.  It mean we'll have to walk
> the list for every namespace in the decriptor, but I've not actually
> seen deployments with crazy numbers of namespaces yet.

This is definitely a short term solution that does not require
us to change the data structures or address sorting at the end of scan_work.

I believe the impact of re-scanning desc->nsids will be minimal because
- It only happens when ana_work catches the namespaces list in a
  transient state with a few new namespaces appended
- The number of desc->nsids (one ANA group) should be much smaller
  than the total number of namespaces.

Thanks,
Anton

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

Re: [PATCH v2 0/3] nvme/mpath: fix missed namespaces in ana state update

[Christoph Hellwig]

On Mon, Sep 13, 2021 at 09:30:20AM -0600, Anton Eidelman wrote:
> Fixed two issues in nvme_update_ana_state() that caused ana_work
> to miss existing namespaces and consequently a failure to update
> the namespace ANA state based on the ANA log page.
> 
> 1) A plain bug: we skipped an nsid in desc->nsids in a certain
>    combination of nsids present and nsids reports in the ANA log,
>    and failed to match this nsid to an existing namespace.
> 2) Unhandled situation when scan_work appended new namespaces to
>    ctrl->namespaces and did not sort the list yet.
>    In such transient state ana_work would fail to match nsids
>    to those new namespaces.
> 
> Both issues potentially caused some namespaces to get stuck
> in an incorrect ANA state, e.g. to never become live.

What are the changes to v1?  That should always go into the cover letter.

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

[PATCH v2 0/3] nvme/mpath: fix missed namespaces in ana state update FIXED

[Anton Eidelman]

Fixed two issues in nvme_update_ana_state() that caused ana_work
to miss existing namespaces and consequently a failure to update
the namespace ANA state based on the ANA log page.

1) A plain bug: we skipped an nsid in desc->nsids in a certain
   combination of nsids present and nsids reports in the ANA log,
   and failed to match this nsid to an existing namespace.
2) Unhandled situation when scan_work appended new namespaces to
   ctrl->namespaces and did not sort the list yet.
   In such transient state ana_work would fail to match nsids
   to those new namespaces.

Both issues potentially caused some namespaces to get stuck
in an incorrect ANA state, e.g. to never become live.

CHANGES FROM V1:
- 3/3: last_ns_nsid was not updated in every iteration, which is wrong

Anton Eidelman (3):
  nvme/multipath: fix failure to update ns ana state
  nvme/multipath: cosmetic: keep ns nsid locally
  nvme/multipath: fix stale ana state for namespaces just added by scan
    work

 drivers/nvme/host/multipath.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

-- 
2.25.1


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

[PATCH v3 0/3] nvme/mpath: fix missed namespaces in ana state update

[Anton Eidelman]

Fixed two issues in nvme_update_ana_state() that caused ana_work
to miss existing namespaces and consequently a failure to update
the namespace ANA state based on the ANA log page.

1) A plain bug: we skipped an nsid in desc->nsids in a certain
   combination of nsids present and nsids reports in the ANA log,
   and failed to match this nsid to an existing namespace.
2) Unhandled situation when scan_work appended new namespaces to
   ctrl->namespaces and did not sort the list yet.
   In such transient state ana_work would fail to match nsids
   to those new namespaces.

Both issues potentially caused some namespaces to get stuck
in an incorrect ANA state, e.g. to never become live.

CHANGES in V2:
- 3/3: last_ns_nsid was not updated in every iteration, which is wrong
CHANGES in V3:
- 2/3: use unsigned instead of unsigned int (ignore checkpatch on this)
- 3/3: use unsigned instead of unsigned int for last_ns_nsid

Anton Eidelman (3):
  nvme/multipath: fix failure to update ns ana state
  nvme/multipath: cosmetic: keep ns nsid locally
  nvme/multipath: fix stale ana state for namespaces just added by scan
    work

 drivers/nvme/host/multipath.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

-- 
2.25.1


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

[PATCH v3 1/3] nvme/multipath: fix failure to update ns ana state

[Anton Eidelman]

nvme_update_ana_state() has a deficiency that results
in failure to update the ana state for a namespace
in the following case:
nsid's in ctrl->namespaces: 1, 3, 4
nsid's in desc->nsids: 1, 2, 3, 4

Loop iteration 0:
    ns index = 0, n = 0, ns->head->ns_id = 1, nsid = 1, MATCH.
Loop iteration 1:
    ns index = 1, n = 1, ns->head->ns_id = 3, nsid = 2, NO MATCH.
Loop iteration 2:
    ns index = 2, n = 2, ns->head->ns_id = 4, nsid = 4, MATCH.

Result: missed nsid=3 and did not update its ana state.

Solution: when ns->head->ns_id is higher than nsid,
increment n and RETRY with the SAME ns.

Signed-off-by: Anton Eidelman <anton@lightbitslabs.com>
---
 drivers/nvme/host/multipath.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 5d7bc58a27bd..e8ccdd398f78 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -600,14 +600,17 @@ static int nvme_update_ana_state(struct nvme_ctrl *ctrl,
 
 	down_read(&ctrl->namespaces_rwsem);
 	list_for_each_entry(ns, &ctrl->namespaces, list) {
-		unsigned nsid = le32_to_cpu(desc->nsids[n]);
-
+		unsigned nsid;
+again:
+		nsid = le32_to_cpu(desc->nsids[n]);
 		if (ns->head->ns_id < nsid)
 			continue;
 		if (ns->head->ns_id == nsid)
 			nvme_update_ns_ana_state(desc, ns);
 		if (++n == nr_nsids)
 			break;
+		if (ns->head->ns_id > nsid)
+			goto again;
 	}
 	up_read(&ctrl->namespaces_rwsem);
 	return 0;
-- 
2.25.1


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

[PATCH v3 2/3] nvme/multipath: cosmetic: keep ns nsid locally

[Anton Eidelman]

Keep the nsid of the current namespace in a local variable,
in order to avoid multiple occurrances of "ns->head->ns_id".

Signed-off-by: Anton Eidelman <anton@lightbitslabs.com>
---
 drivers/nvme/host/multipath.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index e8ccdd398f78..e1ce80c12815 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -601,15 +601,17 @@ static int nvme_update_ana_state(struct nvme_ctrl *ctrl,
 	down_read(&ctrl->namespaces_rwsem);
 	list_for_each_entry(ns, &ctrl->namespaces, list) {
 		unsigned nsid;
+		unsigned ns_nsid = ns->head->ns_id;
+
 again:
 		nsid = le32_to_cpu(desc->nsids[n]);
-		if (ns->head->ns_id < nsid)
+		if (ns_nsid < nsid)
 			continue;
-		if (ns->head->ns_id == nsid)
+		if (ns_nsid == nsid)
 			nvme_update_ns_ana_state(desc, ns);
 		if (++n == nr_nsids)
 			break;
-		if (ns->head->ns_id > nsid)
+		if (ns_nsid > nsid)
 			goto again;
 	}
 	up_read(&ctrl->namespaces_rwsem);
-- 
2.25.1


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

[PATCH v3 3/3] nvme/multipath: fix stale ana state for namespaces just added by scan work

[Anton Eidelman]

Scan work initially adds new namespaces to ctrl->namespaces TAIL.
They make the list unordered temporarily until nvme_scan_work()
finally sorts the list.

In case nvme_update_ana_state() runs while the list is unsorted,
the recently added namespaces are missed and their ana state
may remain not updated forever if timing between scan work and ana work
is unfortunate, e.g.
Initial state: namespaces = {2, 3}
scan_work: adds nsid=1: namespaces = {2, 3, 1}
scan_work: finds nsid=1 is still Inaccessible
ana_work: log page has nsids = {1, 2, 3, 4}, all Optimized.
ana_work: updates nsids {2, 3} but fails to find nsid=1 in namespaces.
scan_work: adds nsid=4: namespaces = {2, 3, 1, 4}
scan_work: finds nsid=4 is Optimized: sets it live.
scan_work: completes an sorts namespaces = {1, 2, 3, 4}
Result: nsid=1 will remain in Inaccessible state.

Solution:
In order to preserve the way ctrl->namespaces is updated and sorted,
make nvme_update_ana_state() deal with the case where ctrl->namespaces
is not fully sorted and has new namespaces appended with potentially
lower nsids.
nvme_update_ana_state() keeps track of the nsid seen in the list,
detects the unsorted case (rare), and restarts scanning of desc->nsids.

Signed-off-by: Anton Eidelman <anton@lightbitslabs.com>
---
 drivers/nvme/host/multipath.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index e1ce80c12815..14504a71711c 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -587,6 +587,7 @@ static int nvme_update_ana_state(struct nvme_ctrl *ctrl,
 	u32 nr_nsids = le32_to_cpu(desc->nnsids), n = 0;
 	unsigned *nr_change_groups = data;
 	struct nvme_ns *ns;
+	unsigned last_ns_nsid = 0;
 
 	dev_dbg(ctrl->device, "ANA group %d: %s.\n",
 			le32_to_cpu(desc->grpid),
@@ -603,6 +604,11 @@ static int nvme_update_ana_state(struct nvme_ctrl *ctrl,
 		unsigned nsid;
 		unsigned ns_nsid = ns->head->ns_id;
 
+		if (ns_nsid < last_ns_nsid) {
+			/* Unsorted ctrl->namespaces: re-scan desc->nsids */
+			n = 0;
+		}
+		last_ns_nsid = ns_nsid;
 again:
 		nsid = le32_to_cpu(desc->nsids[n]);
 		if (ns_nsid < nsid)
-- 
2.25.1


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