[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 at 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;
 }
 
--