[v1] nvme: make core.nvme_multipath configurable

[PATCH 2/4] nvme-multipath: add the NVME_MULTIPATH_PARAM config option

Posted by John Meneghini 1 year, 1 month ago

The NVME_MULTIPATH_PARAM option controls the core.nvme_multipath module
parameter. When NVME_MULTIPATH_PARAM=n the multipath parameter is removed
and core nvme multipathing is enabled. When NVME_MULTIPATH_PARAM=y
the multipath parameter is added and multipath support becomes
configurable with the core.nvme_multipath parameter.

By default NVME_MULTIPATH_PARAM=y

Signed-off-by: John Meneghini <jmeneghi@redhat.com>
---
 drivers/nvme/host/Kconfig     | 15 +++++++++++++++
 drivers/nvme/host/multipath.c |  3 ++-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/Kconfig b/drivers/nvme/host/Kconfig
index 91b0346ce65a..c4251504f201 100644
--- a/drivers/nvme/host/Kconfig
+++ b/drivers/nvme/host/Kconfig
@@ -28,6 +28,21 @@ config NVME_MULTIPATH
 
 	  If unsure, say Y.
 
+config NVME_MULTIPATH_PARAM
+	bool "NVMe multipath param"
+	depends on NVME_CORE && NVME_MULTIPATH
+	help
+	  This option enables configurable support for multipath access with
+	  NVMe subsystems. If this option is enabled NVMe multipath support is
+	  configured by the nvme core module parameter named "multipath". If
+	  this option is disabled the nvme core module "multipath" parameter
+	  is removed and support for NVMe multipath access can not be
+	  configured. When this option is disabled a single /dev/nvmeXnY
+	  device entry will be seen for each NVMe namespace, even if the
+	  namespace is accessible through multiple controllers.
+
+	  If unsure, say Y.
+
 config NVME_VERBOSE_ERRORS
 	bool "NVMe verbose error reporting"
 	depends on NVME_CORE
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 2a7635565083..4536ad5fbb82 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -10,10 +10,11 @@
 #include "nvme.h"
 
 bool multipath = true;
+#ifdef NVME_MULTIPATH_PARAM
 module_param(multipath, bool, 0444);
 MODULE_PARM_DESC(multipath,
 	"turn on native support for multiple controllers per subsystem");
-
+#endif
 static const char *nvme_iopolicy_names[] = {
 	[NVME_IOPOLICY_NUMA]	= "numa",
 	[NVME_IOPOLICY_RR]	= "round-robin",
-- 
2.48.1

Re: [PATCH 2/4] nvme-multipath: add the NVME_MULTIPATH_PARAM config option

Posted by Christoph Hellwig 1 year ago

On Thu, Feb 27, 2025 at 10:25:39PM -0500, John Meneghini wrote:
> The NVME_MULTIPATH_PARAM option controls the core.nvme_multipath module
> parameter. When NVME_MULTIPATH_PARAM=n the multipath parameter is removed
> and core nvme multipathing is enabled. When NVME_MULTIPATH_PARAM=y
> the multipath parameter is added and multipath support becomes
> configurable with the core.nvme_multipath parameter.

What's the point of adding yet another confusing option?

Re: [PATCH 2/4] nvme-multipath: add the NVME_MULTIPATH_PARAM config option

Posted by John Meneghini 1 year ago

On 3/5/25 9:33 AM, Christoph Hellwig wrote:
> On Thu, Feb 27, 2025 at 10:25:39PM -0500, John Meneghini wrote:
>> The NVME_MULTIPATH_PARAM option controls the core.nvme_multipath module
>> parameter. When NVME_MULTIPATH_PARAM=n the multipath parameter is removed
>> and core nvme multipathing is enabled. When NVME_MULTIPATH_PARAM=y
>> the multipath parameter is added and multipath support becomes
>> configurable with the core.nvme_multipath parameter.
> 
> What's the point of adding yet another confusing option?

If you'll read the kConfig description, hopefully it's not confusing.

The whole point of this patch series is to remove the core.nvme_mulipath parameter.

This is what the patch at: https://lore.kernel.org/linux-nvme/20250204211158.43126-1-bgurney@redhat.com/
does, and this is what this patch does. Since people didn't want to remove core.nvme_multipath parameter
in  https://lore.kernel.org/linux-nvme/20250204211158.43126-1-bgurney@redhat.com/ I've proposed this
patch as an alternative.

It provides a kConfig option to remove the core.nvme_multipath parameter so those who want it
can keep it, and those who don't and compile it out.

/John

Re: [PATCH 2/4] nvme-multipath: add the NVME_MULTIPATH_PARAM config option

Posted by Christoph Hellwig 1 year ago

On Tue, Mar 11, 2025 at 10:35:40PM -0400, John Meneghini wrote:
>> What's the point of adding yet another confusing option?
>
> If you'll read the kConfig description, hopefully it's not confusing.

It still is.

> The whole point of this patch series is to remove the core.nvme_mulipath parameter.

Why would a compile time option be preferable over a runtime one?

Re: [PATCH 2/4] nvme-multipath: add the NVME_MULTIPATH_PARAM config option

Posted by John Meneghini 1 year ago

On 3/12/25 1:19 AM, Christoph Hellwig wrote:
> On Tue, Mar 11, 2025 at 10:35:40PM -0400, John Meneghini wrote:
>>> What's the point of adding yet another confusing option?
>>
>> If you'll read the kConfig description, hopefully it's not confusing.
> 
> It still is.

OK. I can fix that.

>> The whole point of this patch series is to remove the core.nvme_mulipath parameter.
> 
> Why would a compile time option be preferable over a runtime one?

Because distos like RHEL don't allow customers to recompile their kernel. And including this runtime
switch in the kernel let's users turn nvme-multipath off w/out recompiling the kernel.

Our RHEL customers do this all of the time to enable DMMP with their NVMe devices. This has caused tremendous
confusion and turmoil with our RHEL customers who keep on turning nvme-multipath off even when we ship
RHEL with nvme-multipath on and we tell them turning it off is not supported.

I finally decided that there is no good way to control this situation w/out removing the
core.nvme_multipath parameter while CONFIG_NVME_MULTIPATH is enabled.

This patch enables distros like RHEL simply config out the parameter w/out impacting all of the people out
there who currently want and use the run time switch when CONFIG_NVME_MULTIPATH is enabled.

Please let me know what I need to do to make these patches less confusing and get them accepted upstream.

/John

Re: [PATCH 2/4] nvme-multipath: add the NVME_MULTIPATH_PARAM config option

Posted by Keith Busch 1 year ago

On Thu, Mar 13, 2025 at 05:46:25PM -0400, John Meneghini wrote:
> Our RHEL customers do this all of the time to enable DMMP with their NVMe devices. This has caused tremendous
> confusion and turmoil with our RHEL customers who keep on turning nvme-multipath off even when we ship
> RHEL with nvme-multipath on and we tell them turning it off is not supported.
> 
> I finally decided that there is no good way to control this situation w/out removing the
> core.nvme_multipath parameter while CONFIG_NVME_MULTIPATH is enabled.
> 
> This patch enables distros like RHEL simply config out the parameter w/out impacting all of the people out
> there who currently want and use the run time switch when CONFIG_NVME_MULTIPATH is enabled.
> 
> Please let me know what I need to do to make these patches less confusing and get them accepted upstream.

It'd be one thing if it was just me lamenting the loss of the parameter,
but Hannes has customers with a very real and obvious use case for it,
even if Christoph hasn't encountered many (or any?) devices that fall
into that trap. So, from my perspective, I really think pcie hotplug
handling needs to be sorted out before we can just force these to
subscribe to the native mulitpath virtual device hierarchy.

In the meantime, I know RHEL has done out of tree patches for many
reasons, so it shouldn't be a big deal to change the module_param type
from "bool" to "bool_enable_only" in your kernel releases.

Re: [PATCH 2/4] nvme-multipath: add the NVME_MULTIPATH_PARAM config option

Posted by Nilay Shroff 1 year, 1 month ago


On 2/28/25 8:55 AM, John Meneghini wrote:
> The NVME_MULTIPATH_PARAM option controls the core.nvme_multipath module
> parameter. When NVME_MULTIPATH_PARAM=n the multipath parameter is removed
> and core nvme multipathing is enabled. When NVME_MULTIPATH_PARAM=y
> the multipath parameter is added and multipath support becomes
> configurable with the core.nvme_multipath parameter.
> 
> By default NVME_MULTIPATH_PARAM=y
> 
> Signed-off-by: John Meneghini <jmeneghi@redhat.com>
> ---
>  drivers/nvme/host/Kconfig     | 15 +++++++++++++++
>  drivers/nvme/host/multipath.c |  3 ++-
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/Kconfig b/drivers/nvme/host/Kconfig
> index 91b0346ce65a..c4251504f201 100644
> --- a/drivers/nvme/host/Kconfig
> +++ b/drivers/nvme/host/Kconfig
> @@ -28,6 +28,21 @@ config NVME_MULTIPATH
>  
>  	  If unsure, say Y.
>  
> +config NVME_MULTIPATH_PARAM
> +	bool "NVMe multipath param"
> +	depends on NVME_CORE && NVME_MULTIPATH
> +	help
> +	  This option enables configurable support for multipath access with
> +	  NVMe subsystems. If this option is enabled NVMe multipath support is
> +	  configured by the nvme core module parameter named "multipath". If
> +	  this option is disabled the nvme core module "multipath" parameter
> +	  is removed and support for NVMe multipath access can not be
> +	  configured. When this option is disabled a single /dev/nvmeXnY
> +	  device entry will be seen for each NVMe namespace, even if the
> +	  namespace is accessible through multiple controllers.
> +
> +	  If unsure, say Y.
> +
If we want to make NVME_MULTIPATH_PARAM default on then I think we need to add 
"default y" under config NVME_MULTIPATH_PARAM.

>  config NVME_VERBOSE_ERRORS
>  	bool "NVMe verbose error reporting"
>  	depends on NVME_CORE
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index 2a7635565083..4536ad5fbb82 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -10,10 +10,11 @@
>  #include "nvme.h"
>  
>  bool multipath = true;
> +#ifdef NVME_MULTIPATH_PARAM

Shouldn't it be CONFIG_NVME_MULTIPATH_PARAM instead of NVME_MULTIPATH_PARAM?

Thanks,
--Nilay

Re: [PATCH 2/4] nvme-multipath: add the NVME_MULTIPATH_PARAM config option

Posted by John Meneghini 1 year, 1 month ago

On 2/28/25 1:28 AM, Nilay Shroff wrote: 
> On 2/28/25 8:55 AM, John Meneghini wrote:
>> diff --git a/drivers/nvme/host/Kconfig b/drivers/nvme/host/Kconfig
>> index 91b0346ce65a..c4251504f201 100644
>> --- a/drivers/nvme/host/Kconfig
>> +++ b/drivers/nvme/host/Kconfig
>> @@ -28,6 +28,21 @@ config NVME_MULTIPATH
>>   
>>   	  If unsure, say Y.
>>   
>> +config NVME_MULTIPATH_PARAM
>> +	bool "NVMe multipath param"
>> +	depends on NVME_CORE && NVME_MULTIPATH
>> +	help
>> +	  This option enables configurable support for multipath access with
>> +	  NVMe subsystems. If this option is enabled NVMe multipath support is
>> +	  configured by the nvme core module parameter named "multipath". If
>> +	  this option is disabled the nvme core module "multipath" parameter
>> +	  is removed and support for NVMe multipath access can not be
>> +	  configured. When this option is disabled a single /dev/nvmeXnY
>> +	  device entry will be seen for each NVMe namespace, even if the
>> +	  namespace is accessible through multiple controllers.
>> +
>> +	  If unsure, say Y.
>> +
> If we want to make NVME_MULTIPATH_PARAM default on then I think we need to add
> "default y" under config NVME_MULTIPATH_PARAM.

OK. I've tested all of the config options.

make mod2noconfig
make allyesconfig
make allmodconfig
make oldconfig

And is all seems to be working correctly, but I'll add the "default y"
as you've suggested.

>>   config NVME_VERBOSE_ERRORS
>>   	bool "NVMe verbose error reporting"
>>   	depends on NVME_CORE
>> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
>> index 2a7635565083..4536ad5fbb82 100644
>> --- a/drivers/nvme/host/multipath.c
>> +++ b/drivers/nvme/host/multipath.c
>> @@ -10,10 +10,11 @@
>>   #include "nvme.h"
>>   
>>   bool multipath = true;
>> +#ifdef NVME_MULTIPATH_PARAM
> 
> Shouldn't it be CONFIG_NVME_MULTIPATH_PARAM instead of NVME_MULTIPATH_PARAM?

Oops.  As you can tell, I haven't tested this yet.  I'll fix this up and test these
changes before sending a version 2 patch.

/John
  
> Thanks,
> --Nilay
>