nvme: Fix disk names when not using nvme multipath
PATCH 1/1 nvme: Fix disk names when not using nvme multipath
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.
---
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(-)
drivers/nvme/host/core.c +7 -1
@@ -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:
drivers/nvme/host/nvme.h +1 -0
@@ -283,6 +283,7 @@ struct nvme_ns {struct kref kref;struct nvme_ns_head *head; +int instance;int lba_shift;u16 ms;u16 sgs;
Signature · 1 line
2.14.3
Christoph Hellwig
maintainerPATCH 1/1 nvme: Fix disk names when not using nvme multipath
Quoted 3 lines · Keith Busch
When
CONFIG_NVME_MULTIPATHis 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):
drivers/nvme/host/core.c +1 -4
@@ -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
PATCH 1/1 nvme: Fix disk names when not using nvme multipath
Quoted 16 lines · Christoph Hellwig
Quoted 3 lines · Keith Busch
When
CONFIG_NVME_MULTIPATHis 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):
drivers/nvme/host/core.c +0
-0@@ -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,
drivers/nvme/host/core.c static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid) · line 3007
Quoted 10 lines
drivers/nvme/host/core.c +1
-4ctrl->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.
Christoph Hellwig
maintainerPATCH 1/1 nvme: Fix disk names when not using nvme multipath
Quoted 4 lines · Keith Busch
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:
drivers/nvme/host/core.c +14 -9
@@ -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)) {
drivers/nvme/host/multipath.c +0 -5
@@ -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;
drivers/nvme/host/nvme.h +2 -0
@@ -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;
PATCH 1/1 nvme: Fix disk names when not using nvme multipath
Quoted 10 lines · Christoph Hellwig
Quoted 4 lines · Keith Busch
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. :)
drivers/nvme/host/core.c static bool streams; · line 72
Quoted 10 lines
drivers/nvme/host/core.c +2
-0@@ -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.
Quoted 2 lines
drivers/nvme/host/core.c +2
-0+MODULE_PARM_DESC (multipath, +"turn on native support for multiple controllers per subsystem" );
Christoph Hellwig
maintainerPATCH 1/1 nvme: Fix disk names when not using nvme multipath
Quoted 2 lines · Keith Busch
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.
PATCH 1/1 nvme: Fix disk names when not using nvme multipath
Quoted 2 lines · Christoph Hellwig
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:
---
drivers/nvme/host/core.c +1 -25
@@ -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)) {
drivers/nvme/host/multipath.c +23 -1
@@ -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;
drivers/nvme/host/nvme.h +18 -0
@@ -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) { }
--
Christoph Hellwig
maintainerPATCH 1/1 nvme: Fix disk names when not using nvme multipath
Quoted 5 lines · Keith Busch
Quoted 2 lines · Christoph Hellwig
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 :)
drivers/nvme/host/multipath.c line 19
Quoted 4 lines
static bool multipath = true;
drivers/nvme/host/multipath.c +1
-1-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.