tweak multipath kconfig and options

tweak multipath kconfig and options

[Christoph Hellwig]

Based on all the flames lately I think we need to make the
Kconfig description of CONFIG_NVME_MULTIPATH a little more clear,
and also warn people if they use unsupported and dangerous setups.

[PATCH 1/3] nvme: enable multipathing by default

[Christoph Hellwig]

And make it a little more clear that the only reason to disable it is
to optimize for size in constrained environment.

Signed-off-by: Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/Kconfig | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/nvme/host/Kconfig b/drivers/nvme/host/Kconfig
index 88a8b5916624..d2ae6954ce7e 100644
--- a/drivers/nvme/host/Kconfig
+++ b/drivers/nvme/host/Kconfig
@@ -16,12 +16,20 @@ config BLK_DEV_NVME
 config NVME_MULTIPATH
 	bool "NVMe multipath support"
 	depends on NVME_CORE
+	default y
 	---help---
 	   This option enables support for multipath access to NVMe
 	   subsystems.  If this option is enabled only a single
 	   /dev/nvmeXnY device will show up for each NVMe namespaces,
 	   even if it is accessible through multiple controllers.
 
+	   Say Y here unless you need to optimize for size in embedded
+	   systems that are never going to see multi-port NVMe devices.
+
+	   If you say N multiple ports of the same subsystem will show
+	   up as separate devices, and none of the normal block device
+	   exclusion mechanisms will work.
+
 config NVME_FABRICS
 	tristate
 
-- 
2.19.1

[PATCH 2/3] nvme: warn when finding multi-port subsystems without multipathing enabled

[Christoph Hellwig]

Without CONFIG_NVME_MULTIPATH enabled a multi-port subsystem might
show up as invididual devices and cause problems, warn about it.

Signed-off-by: Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/nvme.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 27663ce3044e..f2594d468f29 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -534,6 +534,9 @@ static inline void nvme_mpath_check_last_path(struct nvme_ns *ns)
 static inline int nvme_mpath_init(struct nvme_ctrl *ctrl,
 		struct nvme_id_ctrl *id)
 {
+	if (ctrl->subsys->cmic & (1 << 3))
+		dev_warn(ctrl->device,
+"Please enable CONFIG_NVME_MULTIPATH for full support of multi-port devices.\n");
 	return 0;
 }
 static inline void nvme_mpath_uninit(struct nvme_ctrl *ctrl)
-- 
2.19.1

[PATCH 3/3] nvme: deprecate the multipath=0 module option

[Christoph Hellwig]

This was intended as a bridge for pre-existing setups if there were
any, and is not intended to build long-term solutions on top.
Explicitly it and print a warning for each controller found when it
is set.

Signed-off-by: Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/multipath.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index eec44e5ec294..3851c09695b5 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -273,9 +273,15 @@ int nvme_mpath_alloc_disk(struct nvme_ctrl *ctrl, struct nvme_ns_head *head)
 	 * We also do this for private namespaces as the namespace sharing data could
 	 * change after a rescan.
 	 */
-	if (!(ctrl->subsys->cmic & (1 << 1)) || !multipath)
+	if (!(ctrl->subsys->cmic & (1 << 1)))
 		return 0;
 
+	if (!multipath) {
+		dev_warn(ctrl->device,
+"multipath=0 module option is deprecated and will be removed in 2020.\n");
+		return 0;
+	}
+
 	q = blk_alloc_queue_node(GFP_KERNEL, ctrl->numa_node);
 	if (!q)
 		goto out;
-- 
2.19.1

[PATCH 1/3] nvme: enable multipathing by default

[Keith Busch]

On Tue, Nov 20, 2018@05:27:32PM +0100, Christoph Hellwig wrote:
> And make it a little more clear that the only reason to disable it is
> to optimize for size in constrained environment.
> 
> Signed-off-by: Christoph Hellwig <hch at lst.de>
> ---
>  drivers/nvme/host/Kconfig | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/nvme/host/Kconfig b/drivers/nvme/host/Kconfig
> index 88a8b5916624..d2ae6954ce7e 100644
> --- a/drivers/nvme/host/Kconfig
> +++ b/drivers/nvme/host/Kconfig
> @@ -16,12 +16,20 @@ config BLK_DEV_NVME
>  config NVME_MULTIPATH
>  	bool "NVMe multipath support"
>  	depends on NVME_CORE
> +	default y
>  	---help---
>  	   This option enables support for multipath access to NVMe
>  	   subsystems.  If this option is enabled only a single
>  	   /dev/nvmeXnY device will show up for each NVMe namespaces,
>  	   even if it is accessible through multiple controllers.
>  
> +	   Say Y here unless you need to optimize for size in embedded
> +	   systems that are never going to see multi-port NVMe devices.
> +
> +	   If you say N multiple ports of the same subsystem will show
> +	   up as separate devices, and none of the normal block device
> +	   exclusion mechanisms will work.
> +
>  config NVME_FABRICS
>  	tristate

This is okay with me, but I have one little follow-on I'd like to RFC here:

The /dev/nvme naming convention with multiplath has broken people's
minds, and making it default will invite a lot of repeat questions on
how to match up block to admin handles.

Could we promote /dev/nvme0 to be a subsystem handle instead of a
controller handle, and add new per-controller handles, like /dev/nvme0c1,
nvme0c2, etc...? The subsystem handle's management interface can just use
the first active controller to forward admin commands, and per-controller
specific administration can be taught to use the new handles as needed.

Tooling shouldn't care about the names, but I am not sure if this could
possibly break productino scripts. The naming disconnect has broken
some things, though, so I hope this doesn't end up being a pick your
poision suggestion...

[PATCH 1/3] nvme: enable multipathing by default

[Christoph Hellwig]

On Tue, Nov 20, 2018@11:47:49AM -0700, Keith Busch wrote:
> This is okay with me, but I have one little follow-on I'd like to RFC here:
> 
> The /dev/nvme naming convention with multiplath has broken people's
> minds, and making it default will invite a lot of repeat questions on
> how to match up block to admin handles.
> 
> Could we promote /dev/nvme0 to be a subsystem handle instead of a
> controller handle, and add new per-controller handles, like /dev/nvme0c1,
> nvme0c2, etc...? The subsystem handle's management interface can just use
> the first active controller to forward admin commands, and per-controller
> specific administration can be taught to use the new handles as needed.
> 
> Tooling shouldn't care about the names, but I am not sure if this could
> possibly break productino scripts. The naming disconnect has broken
> some things, though, so I hope this doesn't end up being a pick your
> poision suggestion...

The problem is that typical admin command are sensitive to the controller
they are sent to, so a per-subsystem handle does not make whole lot
sense.  But I like the idea of the nvmeXcY names.  Maybe we just keep
the nvmeX names as an additional alias for the first found controller
in a subsystem and otherwise use nvmeXcY? 

[PATCH 1/3] nvme: enable multipathing by default

[Sagi Grimberg]

>> This is okay with me, but I have one little follow-on I'd like to RFC here:
>>
>> The /dev/nvme naming convention with multiplath has broken people's
>> minds, and making it default will invite a lot of repeat questions on
>> how to match up block to admin handles.
>>
>> Could we promote /dev/nvme0 to be a subsystem handle instead of a
>> controller handle, and add new per-controller handles, like /dev/nvme0c1,
>> nvme0c2, etc...? The subsystem handle's management interface can just use
>> the first active controller to forward admin commands, and per-controller
>> specific administration can be taught to use the new handles as needed.
>>
>> Tooling shouldn't care about the names, but I am not sure if this could
>> possibly break productino scripts. The naming disconnect has broken
>> some things, though, so I hope this doesn't end up being a pick your
>> poision suggestion...
> 
> The problem is that typical admin command are sensitive to the controller
> they are sent to, so a per-subsystem handle does not make whole lot
> sense.  But I like the idea of the nvmeXcY names.  Maybe we just keep
> the nvmeX names as an additional alias for the first found controller
> in a subsystem and otherwise use nvmeXcY?

Not sure if different naming conventions between controllers are a good
idea..

What is the problem of simply changing nvmeX to nvmeXcY? if nvmeX is
a must perhaps we can soft link nvmeX to nvmeXc1?

[PATCH 2/3] nvme: warn when finding multi-port subsystems without multipathing enabled

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi at grimberg.me>

[PATCH 3/3] nvme: deprecate the multipath=0 module option

[Sagi Grimberg]

On 11/20/18 8:27 AM, Christoph Hellwig wrote:
> This was intended as a bridge for pre-existing setups if there were
> any, and is not intended to build long-term solutions on top.
> Explicitly it

typo

[PATCH 1/3] nvme: enable multipathing by default

[Keith Busch]

On Wed, Nov 21, 2018@06:44:05PM -0800, Sagi Grimberg wrote:
> 
> >> This is okay with me, but I have one little follow-on I'd like to RFC here:
> >>
> >> The /dev/nvme naming convention with multiplath has broken people's
> >> minds, and making it default will invite a lot of repeat questions on
> >> how to match up block to admin handles.
> >>
> >> Could we promote /dev/nvme0 to be a subsystem handle instead of a
> >> controller handle, and add new per-controller handles, like /dev/nvme0c1,
> >> nvme0c2, etc...? The subsystem handle's management interface can just use
> >> the first active controller to forward admin commands, and per-controller
> >> specific administration can be taught to use the new handles as needed.
> >>
> >> Tooling shouldn't care about the names, but I am not sure if this could
> >> possibly break productino scripts. The naming disconnect has broken
> >> some things, though, so I hope this doesn't end up being a pick your
> >> poision suggestion...
> > 
> > The problem is that typical admin command are sensitive to the controller
> > they are sent to, so a per-subsystem handle does not make whole lot
> > sense.  But I like the idea of the nvmeXcY names.  Maybe we just keep
> > the nvmeX names as an additional alias for the first found controller
> > in a subsystem and otherwise use nvmeXcY?
> 
> Not sure if different naming conventions between controllers are a good
> idea..
> 
> What is the problem of simply changing nvmeX to nvmeXcY? if nvmeX is
> a must perhaps we can soft link nvmeX to nvmeXc1?

I am not aware of anything that has a hard dependency on /dev/nvmeX
being present, but I can't say if that applies to everyone. I'll check
with some internal folks if this will break there tools/workflows.

It will just take some minor shuffling of the initialization order to do
this since we currently assign the controller instance before we know
the subsystem instance. Otherwise, it should work.

I'll finish off the code and send separately. As far as this patch
series is concerned, looks good to me.

Reviewed-by: Keith Busch <keith.busch at intel.com>

[PATCH 3/3 v2] nvme: deprecate the multipath=0 module option

[Christoph Hellwig]

This was intended as a bridge for pre-existing setups if there were
any, and is not intended to build long-term solutions on top.
Deprecate it and print a warning for each controller found when it
is set.

Signed-off-by: Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/multipath.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 9901afd804ce..5f253839f517 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -273,9 +273,15 @@ int nvme_mpath_alloc_disk(struct nvme_ctrl *ctrl, struct nvme_ns_head *head)
 	 * We also do this for private namespaces as the namespace sharing data could
 	 * change after a rescan.
 	 */
-	if (!(ctrl->subsys->cmic & (1 << 1)) || !multipath)
+	if (!(ctrl->subsys->cmic & (1 << 1)))
 		return 0;
 
+	if (!multipath) {
+		dev_warn(ctrl->device,
+"multipath=0 module option is deprecated and will be removed in 2020.\n");
+		return 0;
+	}
+
 	q = blk_alloc_queue_node(GFP_KERNEL, NUMA_NO_NODE, NULL);
 	if (!q)
 		goto out;
-- 
2.19.1