| Message ID | 20180821202221.22732-1-khalid.elmously@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [SRU,Bionic,1/1] nvme/multipath: Fix multipath disabled naming collisions | expand |
Acked-by: Kamal Mostafa <kamal@canonical.com> On Tue, Aug 21, 2018 at 04:22:21PM -0400, Khalid Elmously wrote: > From: Keith Busch <keith.busch@intel.com> > > BugLink: https://bugs.launchpad.net/bugs/1785334 > > 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 this by falling back to the non-multipath naming method > when the parameter disabled using multipath. > > Reported-by: Mike Snitzer <snitzer@redhat.com> > Signed-off-by: Keith Busch <keith.busch@intel.com> > Reviewed-by: Christoph Hellwig <hch@lst.de> > Signed-off-by: Jens Axboe <axboe@kernel.dk> > (cherry-picked from a785dbccd95c37606c720580714f5a7a8b3255f1) > Signed-off-by: Khalid Elmously <khalid.elmously@canonical.com> > --- > drivers/nvme/host/core.c | 26 +------------------------- > drivers/nvme/host/multipath.c | 22 ++++++++++++++++++++++ > drivers/nvme/host/nvme.h | 12 ++++++++++++ > 3 files changed, 35 insertions(+), 25 deletions(-) > > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c > index a7662186c88e..0230b192540c 100644 > --- a/drivers/nvme/host/core.c > +++ b/drivers/nvme/host/core.c > @@ -2902,31 +2902,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid) > if (nvme_init_ns_head(ns, nsid, id, &new)) > 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 cf16905d25e2..3f3143205071 100644 > --- a/drivers/nvme/host/multipath.c > +++ b/drivers/nvme/host/multipath.c > @@ -19,6 +19,28 @@ module_param(multipath, bool, 0644); > 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) { > + sprintf(disk_name, "nvme%dn%d", ctrl->instance, ns->head->instance); > + } 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 f82365cc99fd..a9ec71cce3cc 100644 > --- a/drivers/nvme/host/nvme.h > +++ b/drivers/nvme/host/nvme.h > @@ -405,6 +405,8 @@ extern const struct attribute_group nvme_ns_id_attr_group; > extern const struct block_device_operations nvme_ns_head_ops; > > #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); > void nvme_kick_requeue_lists(struct nvme_ctrl *ctrl); > @@ -430,6 +432,16 @@ static inline void nvme_mpath_check_last_path(struct nvme_ns *ns) > } > > #else > +/* > + * 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_set_disk_name(char *disk_name, struct nvme_ns *ns, > + struct nvme_ctrl *ctrl, int *flags) > +{ > + sprintf(disk_name, "nvme%dn%d", ctrl->instance, ns->head->instance); > +} > + > static inline void nvme_failover_req(struct request *req) > { > } > -- > 2.17.1 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
Acked-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
to bionic/master-next On 2018-08-21 16:22:21 , Khalid Elmously wrote: > From: Keith Busch <keith.busch@intel.com> > > BugLink: https://bugs.launchpad.net/bugs/1785334 > > 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 this by falling back to the non-multipath naming method > when the parameter disabled using multipath. > > Reported-by: Mike Snitzer <snitzer@redhat.com> > Signed-off-by: Keith Busch <keith.busch@intel.com> > Reviewed-by: Christoph Hellwig <hch@lst.de> > Signed-off-by: Jens Axboe <axboe@kernel.dk> > (cherry-picked from a785dbccd95c37606c720580714f5a7a8b3255f1) > Signed-off-by: Khalid Elmously <khalid.elmously@canonical.com> > --- > drivers/nvme/host/core.c | 26 +------------------------- > drivers/nvme/host/multipath.c | 22 ++++++++++++++++++++++ > drivers/nvme/host/nvme.h | 12 ++++++++++++ > 3 files changed, 35 insertions(+), 25 deletions(-) > > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c > index a7662186c88e..0230b192540c 100644 > --- a/drivers/nvme/host/core.c > +++ b/drivers/nvme/host/core.c > @@ -2902,31 +2902,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid) > if (nvme_init_ns_head(ns, nsid, id, &new)) > 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 cf16905d25e2..3f3143205071 100644 > --- a/drivers/nvme/host/multipath.c > +++ b/drivers/nvme/host/multipath.c > @@ -19,6 +19,28 @@ module_param(multipath, bool, 0644); > 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) { > + sprintf(disk_name, "nvme%dn%d", ctrl->instance, ns->head->instance); > + } 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 f82365cc99fd..a9ec71cce3cc 100644 > --- a/drivers/nvme/host/nvme.h > +++ b/drivers/nvme/host/nvme.h > @@ -405,6 +405,8 @@ extern const struct attribute_group nvme_ns_id_attr_group; > extern const struct block_device_operations nvme_ns_head_ops; > > #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); > void nvme_kick_requeue_lists(struct nvme_ctrl *ctrl); > @@ -430,6 +432,16 @@ static inline void nvme_mpath_check_last_path(struct nvme_ns *ns) > } > > #else > +/* > + * 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_set_disk_name(char *disk_name, struct nvme_ns *ns, > + struct nvme_ctrl *ctrl, int *flags) > +{ > + sprintf(disk_name, "nvme%dn%d", ctrl->instance, ns->head->instance); > +} > + > static inline void nvme_failover_req(struct request *req) > { > } > -- > 2.17.1 >
On 08/23/18 07:20, Khaled Elmously wrote: > to bionic/master-next > > On 2018-08-21 16:22:21 , Khalid Elmously wrote: >> From: Keith Busch <keith.busch@intel.com> >> >> BugLink: https://bugs.launchpad.net/bugs/1785334 I have amended this commit using the following public bug report: BugLink: https://bugs.launchpad.net/bugs/1789227 Thanks, Kleber >> >> 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 this by falling back to the non-multipath naming method >> when the parameter disabled using multipath. >> >> Reported-by: Mike Snitzer <snitzer@redhat.com> >> Signed-off-by: Keith Busch <keith.busch@intel.com> >> Reviewed-by: Christoph Hellwig <hch@lst.de> >> Signed-off-by: Jens Axboe <axboe@kernel.dk> >> (cherry-picked from a785dbccd95c37606c720580714f5a7a8b3255f1) >> Signed-off-by: Khalid Elmously <khalid.elmously@canonical.com> >> --- >> drivers/nvme/host/core.c | 26 +------------------------- >> drivers/nvme/host/multipath.c | 22 ++++++++++++++++++++++ >> drivers/nvme/host/nvme.h | 12 ++++++++++++ >> 3 files changed, 35 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c >> index a7662186c88e..0230b192540c 100644 >> --- a/drivers/nvme/host/core.c >> +++ b/drivers/nvme/host/core.c >> @@ -2902,31 +2902,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid) >> if (nvme_init_ns_head(ns, nsid, id, &new)) >> 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 cf16905d25e2..3f3143205071 100644 >> --- a/drivers/nvme/host/multipath.c >> +++ b/drivers/nvme/host/multipath.c >> @@ -19,6 +19,28 @@ module_param(multipath, bool, 0644); >> 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) { >> + sprintf(disk_name, "nvme%dn%d", ctrl->instance, ns->head->instance); >> + } 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 f82365cc99fd..a9ec71cce3cc 100644 >> --- a/drivers/nvme/host/nvme.h >> +++ b/drivers/nvme/host/nvme.h >> @@ -405,6 +405,8 @@ extern const struct attribute_group nvme_ns_id_attr_group; >> extern const struct block_device_operations nvme_ns_head_ops; >> >> #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); >> void nvme_kick_requeue_lists(struct nvme_ctrl *ctrl); >> @@ -430,6 +432,16 @@ static inline void nvme_mpath_check_last_path(struct nvme_ns *ns) >> } >> >> #else >> +/* >> + * 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_set_disk_name(char *disk_name, struct nvme_ns *ns, >> + struct nvme_ctrl *ctrl, int *flags) >> +{ >> + sprintf(disk_name, "nvme%dn%d", ctrl->instance, ns->head->instance); >> +} >> + >> static inline void nvme_failover_req(struct request *req) >> { >> } >> -- >> 2.17.1 >> >
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index a7662186c88e..0230b192540c 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -2902,31 +2902,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid) if (nvme_init_ns_head(ns, nsid, id, &new)) 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 cf16905d25e2..3f3143205071 100644 --- a/drivers/nvme/host/multipath.c +++ b/drivers/nvme/host/multipath.c @@ -19,6 +19,28 @@ module_param(multipath, bool, 0644); 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) { + sprintf(disk_name, "nvme%dn%d", ctrl->instance, ns->head->instance); + } 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 f82365cc99fd..a9ec71cce3cc 100644 --- a/drivers/nvme/host/nvme.h +++ b/drivers/nvme/host/nvme.h @@ -405,6 +405,8 @@ extern const struct attribute_group nvme_ns_id_attr_group; extern const struct block_device_operations nvme_ns_head_ops; #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); void nvme_kick_requeue_lists(struct nvme_ctrl *ctrl); @@ -430,6 +432,16 @@ static inline void nvme_mpath_check_last_path(struct nvme_ns *ns) } #else +/* + * 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_set_disk_name(char *disk_name, struct nvme_ns *ns, + struct nvme_ctrl *ctrl, int *flags) +{ + sprintf(disk_name, "nvme%dn%d", ctrl->instance, ns->head->instance); +} + static inline void nvme_failover_req(struct request *req) { }