| Message ID | 20250321063901.747605-3-nilay@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | improve NVMe multipath handling | expand |
| Context | Check | Description |
|---|---|---|
| shin/vmtest-for-next-PR | success | PR summary |
| shin/vmtest-for-next-VM_Test-0 | success | Logs for build-kernel |
| shin/vmtest-for-next-VM_Test-2 | pending | Logs for run-tests-on-kernel |
| shin/vmtest-for-next-VM_Test-1 | success | Logs for build-kernel |
On 3/21/25 2:37 AM, Nilay Shroff wrote: > Remove the multipath module parameter from nvme-core and make native > NVMe multipath support explicit. Since we now always create a multipath > head disk node, even for single-port NVMe disks, when CONFIG_NVME_ > MULTIPATH is enabled, this module parameter is no longer needed to > toggle the behavior. > > Users who prefer non-native multipath must disable CONFIG_NVME_MULTIPATH > at compile time. This patch is not needed if you use my patch at: https://lore.kernel.org/linux-nvme/20250322232848.225140-1-jmeneghi@redhat.com/T/#m7a6ea63be64731f19df4e17fda7db2b18d8551c7 > Signed-off-by: Nilay Shroff <nilay@linux.ibm.com> > --- > drivers/nvme/host/core.c | 18 +++++++----------- > drivers/nvme/host/multipath.c | 17 ++--------------- > drivers/nvme/host/nvme.h | 1 - > 3 files changed, 9 insertions(+), 27 deletions(-) > > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c > index e798809a8325..50c170425141 100644 > --- a/drivers/nvme/host/core.c > +++ b/drivers/nvme/host/core.c > @@ -3823,14 +3823,13 @@ static int nvme_init_ns_head(struct nvme_ns *ns, struct nvme_ns_info *info) > info->nsid); > goto out_put_ns_head; > } > - > - if (!multipath) { > - dev_warn(ctrl->device, > - "Found shared namespace %d, but multipathing not supported.\n", > - info->nsid); > - dev_warn_once(ctrl->device, > - "Support for shared namespaces without CONFIG_NVME_MULTIPATH is deprecated and will be removed in Linux 6.0.\n"); > - } > +#ifndef CONFIG_NVME_MULTIPATH > + dev_warn(ctrl->device, > + "Found shared namespace %d, but multipathing not supported.\n", > + info->nsid); > + dev_warn_once(ctrl->device, > + "Support for shared namespaces without CONFIG_NVME_MULTIPATH is deprecated and will be removed in Linux 6.0.\n"); I think this message needs to change. See my patch at: https://lore.kernel.org/linux-nvme/20250322232848.225140-1-jmeneghi@redhat.com/T/#md2f1d9706f4cbeb8bd53e562d1036195c0599fe1 > +#endif > } > > list_add_tail_rcu(&ns->siblings, &head->list); > @@ -3929,9 +3928,6 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, struct nvme_ns_info *info) > sprintf(disk->disk_name, "nvme%dc%dn%d", ctrl->subsys->instance, > ctrl->instance, ns->head->instance); > disk->flags |= GENHD_FL_HIDDEN; > - } else if (multipath) { > - sprintf(disk->disk_name, "nvme%dn%d", ctrl->subsys->instance, > - ns->head->instance); I don't think we should remove the 'multipath' variable here we just need to control the users ability to change it. > } else { > sprintf(disk->disk_name, "nvme%dn%d", ctrl->instance, > ns->head->instance); > diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c > index 0f54889bd483..84211f64d178 100644 > --- a/drivers/nvme/host/multipath.c > +++ b/drivers/nvme/host/multipath.c > @@ -9,11 +9,6 @@ > #include <trace/events/block.h> > #include "nvme.h" > > -bool multipath = true; > -module_param(multipath, bool, 0444); > -MODULE_PARM_DESC(multipath, > - "turn on native support for multiple controllers per subsystem"); > - > static const char *nvme_iopolicy_names[] = { > [NVME_IOPOLICY_NUMA] = "numa", > [NVME_IOPOLICY_RR] = "round-robin", > @@ -671,14 +666,6 @@ int nvme_mpath_alloc_disk(struct nvme_ctrl *ctrl, struct nvme_ns_head *head) > INIT_DELAYED_WORK(&head->remove_work, nvme_remove_head_work); > head->delayed_shutdown_sec = 0; > > - /* > - * A head disk node is always created for all types of NVMe disks > - * (single-ported and multi-ported), unless the multipath module > - * parameter is explicitly set to false. > - */ > - if (!multipath) > - return 0; > - > blk_set_stacking_limits(&lim); > lim.dma_alignment = 3; > lim.features |= BLK_FEAT_IO_STAT | BLK_FEAT_NOWAIT | BLK_FEAT_POLL; > @@ -1262,8 +1249,8 @@ int nvme_mpath_init_identify(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id) > size_t ana_log_size; > int error = 0; > > - /* check if multipath is enabled and we have the capability */ > - if (!multipath || !ctrl->subsys || > + /* check if controller has ANA capability */ > + if (!ctrl->subsys || > !(ctrl->subsys->cmic & NVME_CTRL_CMIC_ANA)) > return 0; > > diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h > index 4375357b8cd7..fba686b91976 100644 > --- a/drivers/nvme/host/nvme.h > +++ b/drivers/nvme/host/nvme.h > @@ -997,7 +997,6 @@ static inline bool nvme_disk_is_ns_head(struct gendisk *disk) > return disk->fops == &nvme_ns_head_ops; > } > #else > -#define multipath false > static inline bool nvme_ctrl_use_ana(struct nvme_ctrl *ctrl) > { > return false;
On Fri, Mar 21, 2025 at 12:07:23PM +0530, Nilay Shroff wrote: > Remove the multipath module parameter from nvme-core and make native > NVMe multipath support explicit. Since we now always create a multipath > head disk node, even for single-port NVMe disks, when CONFIG_NVME_ > MULTIPATH is enabled, this module parameter is no longer needed to > toggle the behavior. > > Users who prefer non-native multipath must disable CONFIG_NVME_MULTIPATH > at compile time. Hmm, I actually missed that in the last patch. I fear that is a huge change people don't expect. I suspect we need to make the creation of the head node and the delayed removal an opt-in and not the default to keep existing behavior.
On 4/7/25 8:15 PM, Christoph Hellwig wrote: > On Fri, Mar 21, 2025 at 12:07:23PM +0530, Nilay Shroff wrote: >> Remove the multipath module parameter from nvme-core and make native >> NVMe multipath support explicit. Since we now always create a multipath >> head disk node, even for single-port NVMe disks, when CONFIG_NVME_ >> MULTIPATH is enabled, this module parameter is no longer needed to >> toggle the behavior. >> >> Users who prefer non-native multipath must disable CONFIG_NVME_MULTIPATH >> at compile time. > > Hmm, I actually missed that in the last patch. I fear that is a huge > change people don't expect. I suspect we need to make the creation > of the head node and the delayed removal an opt-in and not the default > to keep existing behavior. Okay, we can add an option to avoid making this behavior "the default". So do you recommend adding a module option for opting in this behavior change or something else? BTW, we're not removing nvme_core.multipath module option as we discussed during LSFMM. Keith and others were against removing this option. So I'd drop this second patch in the series. Thanks, --Nilay
On Tue, Apr 08, 2025 at 08:05:20PM +0530, Nilay Shroff wrote: > Okay, we can add an option to avoid making this behavior "the default". > So do you recommend adding a module option for opting in this behavior > change or something else? I guess a module option as default makes sense. I'd still love to figure out a way to have per-controller options of some kind as e.g. this option make very little sense for thunderbolt-attached external devices. But unfortunately I'm a bit lost what a good interface for that would be.
On 4/9/25 4:15 PM, Christoph Hellwig wrote: > On Tue, Apr 08, 2025 at 08:05:20PM +0530, Nilay Shroff wrote: >> Okay, we can add an option to avoid making this behavior "the default". >> So do you recommend adding a module option for opting in this behavior >> change or something else? > > I guess a module option as default makes sense. I'd still love to figure > out a way to have per-controller options of some kind as e.g. this > option make very little sense for thunderbolt-attached external devices. > > But unfortunately I'm a bit lost what a good interface for that would be. > > I don't know how to make this option per-controller as you know the head node, typically, refers to namespace paths and each path then refers to different controller. So if we were to make this option per controller then how could we handle it in case one controller has this option set but then the another controller doesn't set this option. It could be confusing. How about module option "nvme_core.multipath_head_always"? The default is set to false. So now it becomes two step process: 1. modprobe nvme_core multipath_head_always=Y && modprobe nvme 2. echo "<val>" > /sys/block/nvme0XnY/delayed_shutdown_sec Thanks, --Nilay
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index e798809a8325..50c170425141 100644 --- a/drivers/nvme/host/core.c +++ b/drivers/nvme/host/core.c @@ -3823,14 +3823,13 @@ static int nvme_init_ns_head(struct nvme_ns *ns, struct nvme_ns_info *info) info->nsid); goto out_put_ns_head; } - - if (!multipath) { - dev_warn(ctrl->device, - "Found shared namespace %d, but multipathing not supported.\n", - info->nsid); - dev_warn_once(ctrl->device, - "Support for shared namespaces without CONFIG_NVME_MULTIPATH is deprecated and will be removed in Linux 6.0.\n"); - } +#ifndef CONFIG_NVME_MULTIPATH + dev_warn(ctrl->device, + "Found shared namespace %d, but multipathing not supported.\n", + info->nsid); + dev_warn_once(ctrl->device, + "Support for shared namespaces without CONFIG_NVME_MULTIPATH is deprecated and will be removed in Linux 6.0.\n"); +#endif } list_add_tail_rcu(&ns->siblings, &head->list); @@ -3929,9 +3928,6 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, struct nvme_ns_info *info) sprintf(disk->disk_name, "nvme%dc%dn%d", ctrl->subsys->instance, ctrl->instance, ns->head->instance); disk->flags |= GENHD_FL_HIDDEN; - } else if (multipath) { - sprintf(disk->disk_name, "nvme%dn%d", ctrl->subsys->instance, - ns->head->instance); } else { sprintf(disk->disk_name, "nvme%dn%d", ctrl->instance, ns->head->instance); diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c index 0f54889bd483..84211f64d178 100644 --- a/drivers/nvme/host/multipath.c +++ b/drivers/nvme/host/multipath.c @@ -9,11 +9,6 @@ #include <trace/events/block.h> #include "nvme.h" -bool multipath = true; -module_param(multipath, bool, 0444); -MODULE_PARM_DESC(multipath, - "turn on native support for multiple controllers per subsystem"); - static const char *nvme_iopolicy_names[] = { [NVME_IOPOLICY_NUMA] = "numa", [NVME_IOPOLICY_RR] = "round-robin", @@ -671,14 +666,6 @@ int nvme_mpath_alloc_disk(struct nvme_ctrl *ctrl, struct nvme_ns_head *head) INIT_DELAYED_WORK(&head->remove_work, nvme_remove_head_work); head->delayed_shutdown_sec = 0; - /* - * A head disk node is always created for all types of NVMe disks - * (single-ported and multi-ported), unless the multipath module - * parameter is explicitly set to false. - */ - if (!multipath) - return 0; - blk_set_stacking_limits(&lim); lim.dma_alignment = 3; lim.features |= BLK_FEAT_IO_STAT | BLK_FEAT_NOWAIT | BLK_FEAT_POLL; @@ -1262,8 +1249,8 @@ int nvme_mpath_init_identify(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id) size_t ana_log_size; int error = 0; - /* check if multipath is enabled and we have the capability */ - if (!multipath || !ctrl->subsys || + /* check if controller has ANA capability */ + if (!ctrl->subsys || !(ctrl->subsys->cmic & NVME_CTRL_CMIC_ANA)) return 0; diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h index 4375357b8cd7..fba686b91976 100644 --- a/drivers/nvme/host/nvme.h +++ b/drivers/nvme/host/nvme.h @@ -997,7 +997,6 @@ static inline bool nvme_disk_is_ns_head(struct gendisk *disk) return disk->fops == &nvme_ns_head_ops; } #else -#define multipath false static inline bool nvme_ctrl_use_ana(struct nvme_ctrl *ctrl) { return false;
Remove the multipath module parameter from nvme-core and make native NVMe multipath support explicit. Since we now always create a multipath head disk node, even for single-port NVMe disks, when CONFIG_NVME_ MULTIPATH is enabled, this module parameter is no longer needed to toggle the behavior. Users who prefer non-native multipath must disable CONFIG_NVME_MULTIPATH at compile time. Signed-off-by: Nilay Shroff <nilay@linux.ibm.com> --- drivers/nvme/host/core.c | 18 +++++++----------- drivers/nvme/host/multipath.c | 17 ++--------------- drivers/nvme/host/nvme.h | 1 - 3 files changed, 9 insertions(+), 27 deletions(-)