* [PATCHv5] nvme: allow to re-attach namespaces after all paths are down
@ 2021-06-07 13:34 Hannes Reinecke
2021-06-08 16:42 ` Christoph Hellwig
0 siblings, 1 reply; 3+ messages in thread
From: Hannes Reinecke @ 2021-06-07 13:34 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Sagi Grimberg, Keith Busch, linux-nvme, Hannes Reinecke
We should only remove the ns head from the list of heads per
subsystem if the reference count drops to zero. Removing it
at the start of nvme_ns_remove() will prevent us from reattaching
the namespace to the correct ns head once a path becomes available
again.
Changes to v4:
- Call del_gendisk() in nvme_mpath_check_last_path() to avoid deadlock
Changes to v3:
- Simplify if() clause to detect duplicate namespaces
Changes to v2:
- Drop memcpy() statement
Changes to v1:
- Always check NSIDs after reattach
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
drivers/nvme/host/core.c | 13 +++++++++----
drivers/nvme/host/multipath.c | 12 ++++++++++--
drivers/nvme/host/nvme.h | 11 ++---------
3 files changed, 21 insertions(+), 15 deletions(-)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 177cae44b612..065edf20bef4 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -566,6 +566,9 @@ 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);
@@ -3669,7 +3672,11 @@ static int nvme_init_ns_head(struct nvme_ns *ns, unsigned nsid,
head->shared = is_shared;
} else {
ret = -EINVAL;
- if (!is_shared || !head->shared) {
+ /*
+ * If multipath is enabled we might hit an ns head with no
+ * paths, but that doesn't indicate it's a shared namespace.
+ */
+ if (!list_empty(&head->list) && (!is_shared || !head->shared)) {
dev_err(ctrl->device,
"Duplicate unshared namespace %d\n", nsid);
goto out_put_ns_head;
@@ -3828,8 +3835,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 */
@@ -3849,7 +3854,7 @@ static void nvme_ns_remove(struct nvme_ns *ns)
list_del_init(&ns->list);
up_write(&ns->ctrl->namespaces_rwsem);
- nvme_mpath_check_last_path(ns);
+ nvme_mpath_check_last_path(ns->head);
nvme_put_ns(ns);
}
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 127a17b4c13d..de74650cdb2d 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -776,14 +776,22 @@ void nvme_mpath_add_disk(struct nvme_ns *ns, struct nvme_id_ns *id)
#endif
}
-void nvme_mpath_remove_disk(struct nvme_ns_head *head)
+void nvme_mpath_check_last_path(struct nvme_ns_head *head)
{
if (!head->disk)
return;
- if (head->disk->flags & GENHD_FL_UP) {
+
+ if (list_empty(&head->list) && (head->disk->flags & GENHD_FL_UP)) {
+ kblockd_schedule_work(&head->requeue_work);
nvme_cdev_del(&head->cdev, &head->cdev_device);
del_gendisk(head->disk);
}
+}
+
+void nvme_mpath_remove_disk(struct nvme_ns_head *head)
+{
+ if (!head->disk)
+ return;
blk_set_queue_dying(head->disk->queue);
/* make sure all pending bios are cleaned up */
kblockd_schedule_work(&head->requeue_work);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 1f397ecba16c..812fc1d273e3 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -716,14 +716,7 @@ void nvme_mpath_uninit(struct nvme_ctrl *ctrl);
void nvme_mpath_stop(struct nvme_ctrl *ctrl);
bool nvme_mpath_clear_current_path(struct nvme_ns *ns);
void nvme_mpath_clear_ctrl_paths(struct nvme_ctrl *ctrl);
-
-static inline void nvme_mpath_check_last_path(struct nvme_ns *ns)
-{
- struct nvme_ns_head *head = ns->head;
-
- if (head->disk && list_empty(&head->list))
- kblockd_schedule_work(&head->requeue_work);
-}
+void nvme_mpath_check_last_path(struct nvme_ns_head *head);
static inline void nvme_trace_bio_complete(struct request *req)
{
@@ -772,7 +765,7 @@ static inline bool nvme_mpath_clear_current_path(struct nvme_ns *ns)
static inline void nvme_mpath_clear_ctrl_paths(struct nvme_ctrl *ctrl)
{
}
-static inline void nvme_mpath_check_last_path(struct nvme_ns *ns)
+static inline void nvme_mpath_check_last_path(struct nvme_ns_head *head)
{
}
static inline void nvme_trace_bio_complete(struct request *req)
--
2.29.2
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCHv5] nvme: allow to re-attach namespaces after all paths are down
2021-06-07 13:34 [PATCHv5] nvme: allow to re-attach namespaces after all paths are down Hannes Reinecke
@ 2021-06-08 16:42 ` Christoph Hellwig
2021-06-08 17:44 ` Hannes Reinecke
0 siblings, 1 reply; 3+ messages in thread
From: Christoph Hellwig @ 2021-06-08 16:42 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: Christoph Hellwig, Sagi Grimberg, Keith Busch, linux-nvme
While I do like the look, I don't see how this could actually work:
> } else {
> ret = -EINVAL;
> - if (!is_shared || !head->shared) {
> + /*
> + * If multipath is enabled we might hit an ns head with no
> + * paths, but that doesn't indicate it's a shared namespace.
> + */
> + if (!list_empty(&head->list) && (!is_shared || !head->shared)) {
> dev_err(ctrl->device,
> "Duplicate unshared namespace %d\n", nsid);
> goto out_put_ns_head;
When we get here with an existing ns_head with no namespaces attached
to it, we can assume it once had namespaces attached to it. Because
of that NVME_NSHEAD_DISK_LIVE will be set, and we'll never add the
disk for it again. Even if we did add the disk again what prevents
adding a disk and removing one from racing with each other?
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCHv5] nvme: allow to re-attach namespaces after all paths are down
2021-06-08 16:42 ` Christoph Hellwig
@ 2021-06-08 17:44 ` Hannes Reinecke
0 siblings, 0 replies; 3+ messages in thread
From: Hannes Reinecke @ 2021-06-08 17:44 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Sagi Grimberg, Keith Busch, linux-nvme
On 6/8/21 6:42 PM, Christoph Hellwig wrote:
> While I do like the look, I don't see how this could actually work:
>
>> } else {
>> ret = -EINVAL;
>> - if (!is_shared || !head->shared) {
>> + /*
>> + * If multipath is enabled we might hit an ns head with no
>> + * paths, but that doesn't indicate it's a shared namespace.
>> + */
>> + if (!list_empty(&head->list) && (!is_shared || !head->shared)) {
>> dev_err(ctrl->device,
>> "Duplicate unshared namespace %d\n", nsid);
>> goto out_put_ns_head;
>
> When we get here with an existing ns_head with no namespaces attached
> to it, we can assume it once had namespaces attached to it. Because
> of that NVME_NSHEAD_DISK_LIVE will be set, and we'll never add the
> disk for it again. Even if we did add the disk again what prevents
> adding a disk and removing one from racing with each other?
>
Hmm. I thought it was resolved by refcount magic.
Will have a closer look to check what needs to be done.
Thanks for the review.
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-06-08 17:44 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-07 13:34 [PATCHv5] nvme: allow to re-attach namespaces after all paths are down Hannes Reinecke
2021-06-08 16:42 ` Christoph Hellwig
2021-06-08 17:44 ` Hannes Reinecke
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.