[PATCHv3] nvme: fix namespace removal list

[PATCHv3] nvme: fix namespace removal list

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

This function wants to move a subset of a list from one element to the
tail into another list. It also needs to use the srcu synchronize
instead of the regular rcu version. Do this one element at a time
because that's the only to do it.

Fixes: be647e2c76b27f4 ("nvme: use srcu for iterating namespace list")
Reported-by: Venkat Rao Bagalkote <venkat88@linux.vnet.ibm.com>
Tested-by: Venkat Rao Bagalkote <venkat88@linux.vnet.ibm.com>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
It would be neat to have a more elegant helper that we've been messing
with, and be able to use a single srcu synchronize, but in the interest
of time, this patch will fix the regression.

I was hoping to not have per-ns srcu sync's, but this path pretty
much only gets called on the last namespace anyway, so it should
functionally work out to be a single sync without to optimizations.

 drivers/nvme/host/core.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index c40930d10bd34..782090ce0bc10 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3960,12 +3960,13 @@ static void nvme_remove_invalid_namespaces(struct nvme_ctrl *ctrl,
 
 	mutex_lock(&ctrl->namespaces_lock);
 	list_for_each_entry_safe(ns, next, &ctrl->namespaces, list) {
-		if (ns->head->ns_id > nsid)
-			list_splice_init_rcu(&ns->list, &rm_list,
-					     synchronize_rcu);
+		if (ns->head->ns_id > nsid) {
+			list_del_rcu(&ns->list);
+			synchronize_srcu(&ctrl->srcu);
+			list_add_tail_rcu(&ns->list, &rm_list);
+		}
 	}
 	mutex_unlock(&ctrl->namespaces_lock);
-	synchronize_srcu(&ctrl->srcu);
 
 	list_for_each_entry_safe(ns, next, &rm_list, list)
 		nvme_ns_remove(ns);
-- 
2.43.0

Re: [PATCHv3] nvme: fix namespace removal list

[Christoph Hellwig]

On Thu, Jun 13, 2024 at 09:42:46AM -0700, Keith Busch wrote:
> It would be neat to have a more elegant helper that we've been messing
> with, and be able to use a single srcu synchronize, but in the interest
> of time, this patch will fix the regression.
> 
> I was hoping to not have per-ns srcu sync's, but this path pretty
> much only gets called on the last namespace anyway, so it should
> functionally work out to be a single sync without to optimizations.

Yeah, compared to the 2-3 synchronize_srcu calls in nvme_ns_remove
one more probably doesn't matter right now..

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCHv3] nvme: fix namespace removal list

[Sagi Grimberg]

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