[PATCH] nvme: Do not detach nshead when a namespace is removed

[PATCH] nvme: Do not detach nshead when a namespace is removed

[Hannes Reinecke]

'struct nshead' and 'struct ns' have different lifetimes; the nshead
can (and occasionally will) stay around even if no namespaces are
connected, as it will only be finally removed once the last
opener is gone.
Once the system enters this state it becomes very hard to recover,
as the only way to remove a namespace is to disconnect the controller,
but the nshead is precisely _not_ related to a controller. This leads
to the very annoying behaviour that a subsequent 'nvme connect' call
will establish a _different_ nshead, _and_ the previous nshead will
stay around:

> nvme list
/dev/nvme0n1     9329c4c4af01db2c     Linux \
    1          17.18  GB /  17.18  GB      4 KiB +  0 B   5.3.18-4
> nvme disconnect -d /dev/nvme0
> nvme list
/dev/nvme0n1                                \
    -1          0.00   B /   0.00   B      1   B +  0 B   n1
> nvme connect
> nvme list
/dev/nvme0n1     9329c4c4af01db2c     Linux \
    -1          0.00   B /   0.00   B      1   B +  0 B   5.3.18-4
/dev/nvme0n2     9329c4c4af01db2c     Linux \
    1          17.18  GB /  17.18  GB      4 KiB +  0 B   5.3.18-4

This patch fixes the situation by only removing the nshead from the
internal lists once it's being finally removed.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/host/core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 206e6f883ca8..20fa62ae297a 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -542,6 +542,10 @@ static void nvme_free_ns_head(struct kref *ref)
 	struct nvme_ns_head *head =
 		container_of(ref, struct nvme_ns_head, ref);
 
+	mutex_lock(&head->subsys->lock);
+	list_del_init(&head->entry);
+	mutex_unlock(&head->subsys->lock);
+
 	nvme_mpath_remove_disk(head);
 	ida_simple_remove(&head->subsys->ns_ida, head->instance);
 	cleanup_srcu_struct(&head->srcu);
@@ -3983,8 +3987,6 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid,
  out_unlink_ns:
 	mutex_lock(&ctrl->subsys->lock);
 	list_del_rcu(&ns->siblings);
-	if (list_empty(&ns->head->list))
-		list_del_init(&ns->head->entry);
 	mutex_unlock(&ctrl->subsys->lock);
 	nvme_put_ns_head(ns->head);
  out_free_queue:
@@ -4005,8 +4007,6 @@ static void nvme_ns_remove(struct nvme_ns *ns)
 
 	mutex_lock(&ns->ctrl->subsys->lock);
 	list_del_rcu(&ns->siblings);
-	if (list_empty(&ns->head->list))
-		list_del_init(&ns->head->entry);
 	mutex_unlock(&ns->ctrl->subsys->lock);
 
 	synchronize_rcu(); /* guarantee not available in head->list */
-- 
2.29.2


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

Re: [PATCH] nvme: Do not detach nshead when a namespace is removed

[Sagi Grimberg]

> 'struct nshead' and 'struct ns' have different lifetimes; the nshead
> can (and occasionally will) stay around even if no namespaces are
> connected, as it will only be finally removed once the last
> opener is gone.
> Once the system enters this state it becomes very hard to recover,
> as the only way to remove a namespace is to disconnect the controller,
> but the nshead is precisely _not_ related to a controller. This leads
> to the very annoying behaviour that a subsequent 'nvme connect' call
> will establish a _different_ nshead, _and_ the previous nshead will
> stay around:
> 
>> nvme list
> /dev/nvme0n1     9329c4c4af01db2c     Linux \
>      1          17.18  GB /  17.18  GB      4 KiB +  0 B   5.3.18-4
>> nvme disconnect -d /dev/nvme0
>> nvme list
> /dev/nvme0n1                                \
>      -1          0.00   B /   0.00   B      1   B +  0 B   n1
>> nvme connect
>> nvme list
> /dev/nvme0n1     9329c4c4af01db2c     Linux \
>      -1          0.00   B /   0.00   B      1   B +  0 B   5.3.18-4
> /dev/nvme0n2     9329c4c4af01db2c     Linux \
>      1          17.18  GB /  17.18  GB      4 KiB +  0 B   5.3.18-4
> 
> This patch fixes the situation by only removing the nshead from the
> internal lists once it's being finally removed.

I'd say that we should remove the nshead exactly when the last
lower ns was removed? Why should it still occupy sysfs and ida
resources if it is going away soon?

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