[v2] nvme: make core.nvme_multipath configurable

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

Posted by John Meneghini 10 months, 3 weeks ago

The new CONFIG_NVME_MULTIPATH_PARAM option controls the core_nvme.multipath
parameter. When CONFIG_NVME_MULTIPATH_PARAM=n the multipath parameter is
removed from the kernel and nvme multipathing is permanently enabled.
When NVME_MULTIPATH_PARAM=y the nvme multipath parameter is added to the
kernel and nvme multipath support is controlled by the
core_nvme.multipath parameter.

By default CONFIG_NVME_MULTIPATH_PARAM=y

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

diff --git a/drivers/nvme/host/Kconfig b/drivers/nvme/host/Kconfig
index d47dfa80fb95..c8458b10c83f 100644
--- a/drivers/nvme/host/Kconfig
+++ b/drivers/nvme/host/Kconfig
@@ -28,6 +28,19 @@ config NVME_MULTIPATH
 
 	  If unsure, say Y.
 
+config NVME_MULTIPATH_PARAM
+	bool "NVMe multipath param"
+	depends on NVME_CORE && NVME_MULTIPATH
+	default y
+	help
+	  This option controls the inclusion of the NVMe core module
+	  "multipath" parameter. If this option is disabled the
+	  nvme_core.multipath parameter is excluded from the kernel.
+	  If this option is enabled the nvme_core.multipath parameter
+	  is included in the kernel.
+
+	  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 6b12ca80aa27..cad76de2830a 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -10,10 +10,11 @@
 #include "nvme.h"
 
 bool multipath = true;
+#ifdef CONFIG_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 v2 2/3] nvme-multipath: add the NVME_MULTIPATH_PARAM config option

Posted by Keith Busch 10 months ago

On Sat, Mar 22, 2025 at 07:28:47PM -0400, John Meneghini wrote:
> +config NVME_MULTIPATH_PARAM
> +	bool "NVMe multipath param"
> +	depends on NVME_CORE && NVME_MULTIPATH
> +	default y
> +	help
> +	  This option controls the inclusion of the NVMe core module
> +	  "multipath" parameter. If this option is disabled the
> +	  nvme_core.multipath parameter is excluded from the kernel.
> +	  If this option is enabled the nvme_core.multipath parameter
> +	  is included in the kernel.
> +
> +	  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 6b12ca80aa27..cad76de2830a 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -10,10 +10,11 @@
>  #include "nvme.h"
>  
>  bool multipath = true;
> +#ifdef CONFIG_NVME_MULTIPATH_PARAM
>  module_param(multipath, bool, 0444);
>  MODULE_PARM_DESC(multipath,
>  	"turn on native support for multiple controllers per subsystem");
> -
> +#endif

John,

Is this the logic and wording you want this option to be? You, or at
least somebody, suggested to rename it DISABLE_MULTIPATH", default "n".
The end result is just a matter of setting it accordingly; I just don't
want to stall getting your feature in over a miscommunication. Thanks!

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

Posted by Christoph Hellwig 10 months, 1 week ago

On Sat, Mar 22, 2025 at 07:28:47PM -0400, John Meneghini wrote:
> The new CONFIG_NVME_MULTIPATH_PARAM option controls the core_nvme.multipath
> parameter. When CONFIG_NVME_MULTIPATH_PARAM=n the multipath parameter is
> removed from the kernel and nvme multipathing is permanently enabled.
> When NVME_MULTIPATH_PARAM=y the nvme multipath parameter is added to the
> kernel and nvme multipath support is controlled by the
> core_nvme.multipath parameter.

So as stated before I hate these options with passion.  On the other
hand both RH and SuSE stated they'd prefer to ship with the option
disabled, so I'd rather accomodate them rather than having to ship
patches for this which will then confuse folks why they are different
from upstream.

But:

> +config NVME_MULTIPATH_PARAM
> +	bool "NVMe multipath param"

This isn't really a good config options description.

> +	depends on NVME_CORE && NVME_MULTIPATH
> +	default y
> +	help
> +	  This option controls the inclusion of the NVMe core module
> +	  "multipath" parameter. If this option is disabled the
> +	  nvme_core.multipath parameter is excluded from the kernel.
> +	  If this option is enabled the nvme_core.multipath parameter
> +	  is included in the kernel.

So maybe invert the option to 

config NVME_MULTIPATH_DISABLE
	bool "Allow overriding the default nvme-multipath parameter"

	help
	  This option controls the inclusion of the NVMe core module
	  "multipath" parameter. If this option is enabled the
	  nvme_core.multipath parameter is excluded from the kernel.
	  If this option is enabled the nvme_core.multipath parameter

	  See the nvme_core.multipath documentation why disabling
	  multipathing is generally harmful but there might be
	  exception reasons to do so anyway.

(assuming we already have the documentation mentioned, if not we
need to add it)

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

Posted by John Meneghini 10 months, 1 week ago

On 4/3/25 12:35 AM, Christoph Hellwig wrote:
> On Sat, Mar 22, 2025 at 07:28:47PM -0400, John Meneghini wrote:

>> +config NVME_MULTIPATH_PARAM
>> +	bool "NVMe multipath param"
> 
> This isn't really a good config options description.
> 
>> +	depends on NVME_CORE && NVME_MULTIPATH
>> +	default y
>> +	help
>> +	  This option controls the inclusion of the NVMe core module
>> +	  "multipath" parameter. If this option is disabled the
>> +	  nvme_core.multipath parameter is excluded from the kernel.
>> +	  If this option is enabled the nvme_core.multipath parameter
>> +	  is included in the kernel.
> 
> So maybe invert the option to
> 
> config NVME_MULTIPATH_DISABLE
> 	bool "Allow overriding the default nvme-multipath parameter"

So the question is: do you want the core_nvme.multipath parameter
to be excluded by default, or included by default?

Keith and I agreed to call this CONFIG_NVME_DISBALE_MULTIPATH_PARAM.
However during testing I realized that many of the default make 'config'
rules would end up with CONFIG_NVME_DISBALE_MULTIPATH_PARAM=y,
even if I set the config rule to "default n".

For example:

  make localmodconfig
  make allmodconfig

would end up with compiling out the core_nvme.multipath parameter and I
don't think this is what we want.

If we want this new config option to provide no net change in the
default behavior, we need this to be the logic to be positive.

> 	help
> 	  This option controls the inclusion of the NVMe core module
> 	  "multipath" parameter. If this option is enabled the
> 	  nvme_core.multipath parameter is excluded from the kernel.
> 	  If this option is enabled the nvme_core.multipath parameter

How about something simple like this:

+config NVME_ENABLE_MULTIPATH_PARAM
+       bool "NVMe enable core_nvme.multipath param"
+       depends on NVME_CORE && NVME_MULTIPATH
+       default y
+       help
+         If this option is N the core_nvme.multipath parameter
+         is excluded from the kernel. If this option is Y the
+         core_nvme.multipath parameter is included in the kernel.
+
+         If unsure, say Y.
+

> 	  See the nvme_core.multipath documentation why disabling
> 	  multipathing is generally harmful but there might be
> 	  exception reasons to do so anyway.
> 
> (assuming we already have the documentation mentioned, if not we
> need to add it)

Yes, I will add some documentation about this.  It looks like we currently
don't have any.

/John

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

Posted by Christoph Hellwig 10 months, 1 week ago

On Fri, Apr 04, 2025 at 06:28:10PM -0400, John Meneghini wrote:
>> So maybe invert the option to
>>
>> config NVME_MULTIPATH_DISABLE
>> 	bool "Allow overriding the default nvme-multipath parameter"
>
> So the question is: do you want the core_nvme.multipath parameter
> to be excluded by default, or included by default?

I can live with it either way.

> Keith and I agreed to call this CONFIG_NVME_DISBALE_MULTIPATH_PARAM.
> However during testing I realized that many of the default make 'config'
> rules would end up with CONFIG_NVME_DISBALE_MULTIPATH_PARAM=y,
> even if I set the config rule to "default n".
>
> For example:
>
>  make localmodconfig
>  make allmodconfig
>
> would end up with compiling out the core_nvme.multipath parameter and I
> don't think this is what we want.

Weird, how does that override the explicit default statement?

> How about something simple like this:
>
> +config NVME_ENABLE_MULTIPATH_PARAM
> +       bool "NVMe enable core_nvme.multipath param"
> +       depends on NVME_CORE && NVME_MULTIPATH
> +       default y

"default y" is the default, so it can be skipped.