* [PATCH] nvme: Fix disk names when not using nvme multipath @ 2018-04-17 22:03 Keith Busch 2018-04-18 6:44 ` Christoph Hellwig 0 siblings, 1 reply; 8+ messages in thread From: Keith Busch @ 2018-04-17 22:03 UTC (permalink / raw) When CONFIG_NVME_MULTIPATH is set, but we're not using nvme to multipath, namespaces with multiple paths were not creating unique names due to reusing the same instance number from the namespace's head. This patch fixes that by allocating a unique instance for each namespace. Reported-by: Mike Snitzer <snitzer at redhat.com> Signed-off-by: Keith Busch <keith.busch at intel.com> --- This is slightly different (and simpler) than the test-version from last week, and will work no matter how many "heads" a subsystem has. drivers/nvme/host/core.c | 8 +++++++- drivers/nvme/host/nvme.h | 1 + 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 127a9cbf3314..3dcb0ebe9575 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -366,6 +366,7 @@ static void nvme_free_ns(struct kref *kref) nvme_nvm_unregister(ns); put_disk(ns->disk); + ida_simple_remove(&ns->head->subsys->ns_ida, ns->instance); nvme_put_ns_head(ns->head); nvme_put_ctrl(ns->ctrl); kfree(ns); @@ -2905,6 +2906,10 @@ static int nvme_init_ns_head(struct nvme_ns *ns, unsigned nsid, list_add_tail(&ns->siblings, &head->list); ns->head = head; + ns->instance = ida_simple_get(&head->subsys->ns_ida, 1, 0, GFP_KERNEL); + if (ns->instance < 0) + ret = ns->instance; + out_unlock: mutex_unlock(&ctrl->subsys->lock); return ret; @@ -3013,7 +3018,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid) flags = GENHD_FL_HIDDEN; } else { sprintf(disk_name, "nvme%dn%d", ctrl->subsys->instance, - ns->head->instance); + ns->instance); } #else /* @@ -3068,6 +3073,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid) mutex_lock(&ctrl->subsys->lock); list_del_rcu(&ns->siblings); mutex_unlock(&ctrl->subsys->lock); + ida_simple_remove(&ns->head->subsys->ns_ida, ns->instance); out_free_id: kfree(id); out_free_queue: diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h index 061fecfd44f5..334dd5311955 100644 --- a/drivers/nvme/host/nvme.h +++ b/drivers/nvme/host/nvme.h @@ -283,6 +283,7 @@ struct nvme_ns { struct kref kref; struct nvme_ns_head *head; + int instance; int lba_shift; u16 ms; u16 sgs; -- 2.14.3 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH] nvme: Fix disk names when not using nvme multipath 2018-04-17 22:03 [PATCH] nvme: Fix disk names when not using nvme multipath Keith Busch @ 2018-04-18 6:44 ` Christoph Hellwig 2018-04-18 13:26 ` Keith Busch 0 siblings, 1 reply; 8+ messages in thread From: Christoph Hellwig @ 2018-04-18 6:44 UTC (permalink / raw) On Tue, Apr 17, 2018@04:03:44PM -0600, Keith Busch wrote: > When CONFIG_NVME_MULTIPATH is set, but we're not using nvme to multipath, > namespaces with multiple paths were not creating unique names due to > reusing the same instance number from the namespace's head. Hmm. They should still differ in the controller instance, though. Shouldn't we need something like this instead (untested): diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 197a6ba9700f..bfd67bc71455 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -3001,10 +3001,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid) sprintf(disk_name, "nvme%dc%dn%d", ctrl->subsys->instance, ctrl->cntlid, ns->head->instance); flags = GENHD_FL_HIDDEN; - } else { - sprintf(disk_name, "nvme%dn%d", ctrl->subsys->instance, - ns->head->instance); - } + } else #else /* * But without the multipath code enabled, multiple controller per ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH] nvme: Fix disk names when not using nvme multipath 2018-04-18 6:44 ` Christoph Hellwig @ 2018-04-18 13:26 ` Keith Busch 2018-04-18 15:04 ` Christoph Hellwig 0 siblings, 1 reply; 8+ messages in thread From: Keith Busch @ 2018-04-18 13:26 UTC (permalink / raw) On Wed, Apr 18, 2018@08:44:12AM +0200, Christoph Hellwig wrote: > On Tue, Apr 17, 2018@04:03:44PM -0600, Keith Busch wrote: > > When CONFIG_NVME_MULTIPATH is set, but we're not using nvme to multipath, > > namespaces with multiple paths were not creating unique names due to > > reusing the same instance number from the namespace's head. > > Hmm. They should still differ in the controller instance, though. > > Shouldn't we need something like this instead (untested): > > > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c > index 197a6ba9700f..bfd67bc71455 100644 > --- a/drivers/nvme/host/core.c > +++ b/drivers/nvme/host/core.c > @@ -3001,10 +3001,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid) > sprintf(disk_name, "nvme%dc%dn%d", ctrl->subsys->instance, > ctrl->cntlid, ns->head->instance); > flags = GENHD_FL_HIDDEN; > - } else { > - sprintf(disk_name, "nvme%dn%d", ctrl->subsys->instance, > - ns->head->instance); > - } > + } else > #else > /* > * But without the multipath code enabled, multiple controller per No-can-do. If we did that with nvme-multipath, we could find NMIC namespaces using the susbystem instance for the disk name, and non-NMIC using the controller instance: we'll have a different set of name conflicts to deal with. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] nvme: Fix disk names when not using nvme multipath 2018-04-18 13:26 ` Keith Busch @ 2018-04-18 15:04 ` Christoph Hellwig 2018-04-18 15:34 ` Keith Busch 0 siblings, 1 reply; 8+ messages in thread From: Christoph Hellwig @ 2018-04-18 15:04 UTC (permalink / raw) On Wed, Apr 18, 2018@07:26:58AM -0600, Keith Busch wrote: > No-can-do. If we did that with nvme-multipath, we could find NMIC > namespaces using the susbystem instance for the disk name, and non-NMIC > using the controller instance: we'll have a different set of name > conflicts to deal with. True. ? think I was also trying to solve the wrong problem to start with, that's what happens if you get up a 4am :) Another just as completely untested attempt below: diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 197a6ba9700f..ffca28dd2836 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -68,6 +68,11 @@ static bool streams; module_param(streams, bool, 0644); MODULE_PARM_DESC(streams, "turn on support for Streams write directives"); +bool multipath = true; +module_param(multipath, bool, 0644); +MODULE_PARM_DESC(multipath, + "turn on native support for multiple controllers per subsystem"); + /* * nvme_wq - hosts nvme related works that are not reset or delete * nvme_reset_wq - hosts nvme reset works @@ -2989,6 +2994,13 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid) goto out_free_id; nvme_setup_streams_ns(ctrl, ns); + /* + * Without the multipath code enabled, multiple controller per subsystem + * are visible as devices and thus we cannot use the subsystem instance. + */ + if (!IS_ENABLED(CONFIG_NVME_MULTIPATH) || !multipath) { + sprintf(disk_name, "nvme%dn%d", ctrl->instance, + ns->head->instance); #ifdef CONFIG_NVME_MULTIPATH /* * If multipathing is enabled we need to always use the subsystem @@ -2997,22 +3009,15 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid) * the multipath-aware subsystem node and those that have a single * controller and use the controller node directly. */ - if (ns->head->disk) { + } else if (ns->head->disk) { sprintf(disk_name, "nvme%dc%dn%d", ctrl->subsys->instance, ctrl->cntlid, ns->head->instance); flags = GENHD_FL_HIDDEN; } else { sprintf(disk_name, "nvme%dn%d", ctrl->subsys->instance, ns->head->instance); - } -#else - /* - * But without the multipath code enabled, multiple controller per - * subsystems are visible as devices and thus we cannot use the - * subsystem instance. - */ - sprintf(disk_name, "nvme%dn%d", ctrl->instance, ns->head->instance); #endif + } if ((ctrl->quirks & NVME_QUIRK_LIGHTNVM) && id->vs[0] == 0x1) { if (nvme_nvm_register(ns, disk_name, node)) { diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c index 956e0b8e9c4d..3b0643d6cfc4 100644 --- a/drivers/nvme/host/multipath.c +++ b/drivers/nvme/host/multipath.c @@ -14,11 +14,6 @@ #include <linux/moduleparam.h> #include "nvme.h" -static bool multipath = true; -module_param(multipath, bool, 0644); -MODULE_PARM_DESC(multipath, - "turn on native support for multiple controllers per subsystem"); - void nvme_failover_req(struct request *req) { struct nvme_ns *ns = req->q->queuedata; diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h index cf93690b3ffc..b35b0f9f94c1 100644 --- a/drivers/nvme/host/nvme.h +++ b/drivers/nvme/host/nvme.h @@ -32,6 +32,8 @@ extern unsigned int admin_timeout; #define NVME_DEFAULT_KATO 5 #define NVME_KATO_GRACE 10 +extern bool multipath; + extern struct workqueue_struct *nvme_wq; extern struct workqueue_struct *nvme_reset_wq; extern struct workqueue_struct *nvme_delete_wq; ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH] nvme: Fix disk names when not using nvme multipath 2018-04-18 15:04 ` Christoph Hellwig @ 2018-04-18 15:34 ` Keith Busch 2018-04-18 15:34 ` Christoph Hellwig 0 siblings, 1 reply; 8+ messages in thread From: Keith Busch @ 2018-04-18 15:34 UTC (permalink / raw) On Wed, Apr 18, 2018@05:04:29PM +0200, Christoph Hellwig wrote: > On Wed, Apr 18, 2018@07:26:58AM -0600, Keith Busch wrote: > > No-can-do. If we did that with nvme-multipath, we could find NMIC > > namespaces using the susbystem instance for the disk name, and non-NMIC > > using the controller instance: we'll have a different set of name > > conflicts to deal with. > > True. ? think I was also trying to solve the wrong problem to start > with, that's what happens if you get up a 4am :) > > Another just as completely untested attempt below: It's a bit of an eyesore, but looks like it will work for the most part. :) > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c > index 197a6ba9700f..ffca28dd2836 100644 > --- a/drivers/nvme/host/core.c > +++ b/drivers/nvme/host/core.c > @@ -68,6 +68,11 @@ static bool streams; > module_param(streams, bool, 0644); > MODULE_PARM_DESC(streams, "turn on support for Streams write directives"); > > +bool multipath = true; > +module_param(multipath, bool, 0644); I think the permissions just have to be 0444 so that the user can't change the naming method after some disks were named with the alternate method. > +MODULE_PARM_DESC(multipath, > + "turn on native support for multiple controllers per subsystem"); ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] nvme: Fix disk names when not using nvme multipath 2018-04-18 15:34 ` Keith Busch @ 2018-04-18 15:34 ` Christoph Hellwig 2018-04-18 16:09 ` Keith Busch 0 siblings, 1 reply; 8+ messages in thread From: Christoph Hellwig @ 2018-04-18 15:34 UTC (permalink / raw) On Wed, Apr 18, 2018@09:34:11AM -0600, Keith Busch wrote: > I think the permissions just have to be 0444 so that the user can't change the > naming method after some disks were named with the alternate method. True. We should do that anyway. I'll turn this into a real series with a few more final touches tomorrow. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] nvme: Fix disk names when not using nvme multipath 2018-04-18 15:34 ` Christoph Hellwig @ 2018-04-18 16:09 ` Keith Busch 2018-04-19 8:50 ` Christoph Hellwig 0 siblings, 1 reply; 8+ messages in thread From: Keith Busch @ 2018-04-18 16:09 UTC (permalink / raw) On Wed, Apr 18, 2018@05:34:45PM +0200, Christoph Hellwig wrote: > True. We should do that anyway. I'll turn this into a real series > with a few more final touches tomorrow. I started this before I saw your message: --- diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index 127a9cbf3314..a3771c5729f5 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -2998,31 +2998,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid) if (nvme_init_ns_head(ns, nsid, id)) goto out_free_id; nvme_setup_streams_ns(ctrl, ns); - -#ifdef CONFIG_NVME_MULTIPATH - /* - * If multipathing is enabled we need to always use the subsystem - * instance number for numbering our devices to avoid conflicts - * between subsystems that have multiple controllers and thus use - * the multipath-aware subsystem node and those that have a single - * controller and use the controller node directly. - */ - if (ns->head->disk) { - sprintf(disk_name, "nvme%dc%dn%d", ctrl->subsys->instance, - ctrl->cntlid, ns->head->instance); - flags = GENHD_FL_HIDDEN; - } else { - sprintf(disk_name, "nvme%dn%d", ctrl->subsys->instance, - ns->head->instance); - } -#else - /* - * But without the multipath code enabled, multiple controller per - * subsystems are visible as devices and thus we cannot use the - * subsystem instance. - */ - sprintf(disk_name, "nvme%dn%d", ctrl->instance, ns->head->instance); -#endif + nvme_set_disk_name(disk_name, ns, ctrl, &flags); if ((ctrl->quirks & NVME_QUIRK_LIGHTNVM) && id->vs[0] == 0x1) { if (nvme_nvm_register(ns, disk_name, node)) { diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c index 956e0b8e9c4d..977539d0bb53 100644 --- a/drivers/nvme/host/multipath.c +++ b/drivers/nvme/host/multipath.c @@ -15,10 +15,32 @@ #include "nvme.h" static bool multipath = true; -module_param(multipath, bool, 0644); +module_param(multipath, bool, 0444); MODULE_PARM_DESC(multipath, "turn on native support for multiple controllers per subsystem"); +/* + * If multipathing is enabled we need to always use the subsystem instance + * number for numbering our devices to avoid conflicts between subsystems that + * have multiple controllers and thus use the multipath-aware subsystem node + * and those that have a single controller and use the controller node + * directly. + */ +void nvme_set_disk_name(char *disk_name, struct nvme_ns *ns, + struct nvme_ctrl *ctrl, int *flags) +{ + if (!multipath) { + nvme_simple_disk_name(disk_name, ns, ctrl); + } else if (ns->head->disk) { + sprintf(disk_name, "nvme%dc%dn%d", ctrl->subsys->instance, + ctrl->cntlid, ns->head->instance); + *flags = GENHD_FL_HIDDEN; + } else { + sprintf(disk_name, "nvme%dn%d", ctrl->subsys->instance, + ns->head->instance); + } +} + void nvme_failover_req(struct request *req) { struct nvme_ns *ns = req->q->queuedata; diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h index 061fecfd44f5..2d4a139b00f1 100644 --- a/drivers/nvme/host/nvme.h +++ b/drivers/nvme/host/nvme.h @@ -435,7 +435,19 @@ int nvme_get_log_ext(struct nvme_ctrl *ctrl, struct nvme_ns *ns, extern const struct attribute_group nvme_ns_id_attr_group; extern const struct block_device_operations nvme_ns_head_ops; +/* + * Without the multipath code enabled, multiple controller per subsystems are + * visible as devices and thus we cannot use the subsystem instance. + */ +static inline void nvme_simple_disk_name(char *disk_name, struct nvme_ns *ns, + struct nvme_ctrl *ctrl) +{ + sprintf(disk_name, "nvme%dn%d", ctrl->instance, ns->head->instance); +} + #ifdef CONFIG_NVME_MULTIPATH +void nvme_set_disk_name(char *disk_name, struct nvme_ns *ns, + struct nvme_ctrl *ctrl, int *flags); void nvme_failover_req(struct request *req); bool nvme_req_needs_failover(struct request *req, blk_status_t error); void nvme_kick_requeue_lists(struct nvme_ctrl *ctrl); @@ -461,6 +473,12 @@ static inline void nvme_mpath_check_last_path(struct nvme_ns *ns) } #else +static inline void nvme_set_disk_name(char *disk_name, struct nvme_ns *ns, + struct nvme_ctrl *ctrl, int *flags) +{ + nvme_simple_disk_name(disk_name, ns, ctrl); +} + static inline void nvme_failover_req(struct request *req) { } -- ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH] nvme: Fix disk names when not using nvme multipath 2018-04-18 16:09 ` Keith Busch @ 2018-04-19 8:50 ` Christoph Hellwig 0 siblings, 0 replies; 8+ messages in thread From: Christoph Hellwig @ 2018-04-19 8:50 UTC (permalink / raw) On Wed, Apr 18, 2018@10:09:42AM -0600, Keith Busch wrote: > On Wed, Apr 18, 2018@05:34:45PM +0200, Christoph Hellwig wrote: > > True. We should do that anyway. I'll turn this into a real series > > with a few more final touches tomorrow. > > I started this before I saw your message: Even better :) > static bool multipath = true; > -module_param(multipath, bool, 0644); > +module_param(multipath, bool, 0444); > MODULE_PARM_DESC(multipath, I think this part should be split into a separate patch as it's already unsafe. Otherwise this looks ok to me, but I wonder if the helper is really worth the effort. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-04-19 8:50 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-04-17 22:03 [PATCH] nvme: Fix disk names when not using nvme multipath Keith Busch 2018-04-18 6:44 ` Christoph Hellwig 2018-04-18 13:26 ` Keith Busch 2018-04-18 15:04 ` Christoph Hellwig 2018-04-18 15:34 ` Keith Busch 2018-04-18 15:34 ` Christoph Hellwig 2018-04-18 16:09 ` Keith Busch 2018-04-19 8:50 ` Christoph Hellwig
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).