Don't blacklist nvme

Signed-off-by Keith Busch <[email protected]>

---
Pat of this is dependent on udev updates. Pull request sent to systemd here:

https://github.com/systemd/systemd/pull/5348

Can always add that line by hand in the mean time.

 libmultipath/discovery.c | 36 ++++++++++++++++++++++++++++++++++++
 libmultipath/hwtable.c   |  9 +++++++++
 libmultipath/structs.h   |  1 +
 3 files changed, 46 insertions(+)

libmultipath/discovery.c +12 -0

index d1aec31..db7b04a 100644
@@ -1187,6 +1187,37 @@ scsi_sysfs_pathinfo (struct path * pp, vector hwtable)
 }
 
 static int
+nvme_sysfs_pathinfo (struct path * pp, vector hwtable)
+{
+	struct udev_device *parent, *nvme = NULL;
+
+	parent = pp->udev;
+	while (parent) {
+		const char *subsys = udev_device_get_subsystem(parent);
+
+		if (subsys && !strncmp(subsys, "nvme", 4)) {
+			nvme = parent;
+			break;
+		}

libmultipath/discovery.c +14 -0

+
+	condlog(3, "%s: vendor:%s product:%s serial:%s rev:%s wwid:%s", pp->dev,
+		pp->vendor_id, pp->product_id, pp->serial, pp->rev, pp->wwid);
+	pp->hwe = find_hwe(hwtable, pp->vendor_id, pp->product_id, NULL);
+
+	return 0;
+}
+
+static int
 rbd_sysfs_pathinfo (struct path * pp, vector hwtable)
 {
 	sprintf(pp->vendor_id, "Ceph");

@@ -1405,6 +1436,8 @@ sysfs_pathinfo(struct path * pp, vector hwtable)
 		pp->bus = SYSFS_BUS_SCSI;
 	if (!strncmp(pp->dev,"rbd", 3))
 		pp->bus = SYSFS_BUS_RBD;
+	if (!strncmp(pp->dev,"nvme", 4))
+		pp->bus = SYSFS_BUS_NVME;
 
 	if (pp->bus == SYSFS_BUS_UNDEF)
 		return 0;

@@ -1420,6 +1453,9 @@ sysfs_pathinfo(struct path * pp, vector hwtable)
 	} else if (pp->bus == SYSFS_BUS_RBD) {
 		if (rbd_sysfs_pathinfo(pp, hwtable))
 			return 1;
+	} else if (pp->bus == SYSFS_BUS_NVME) {
+		if (nvme_sysfs_pathinfo(pp, hwtable))
+			return 1;
 	}
 	return 0;
 }

libmultipath/hwtable.c +9 -0

index f5a5f7b..c55998a 100644
@@ -1073,6 +1073,15 @@ static struct hwentry default_hw[] = {
 		.prio_name     = PRIO_ALUA,
 		.no_path_retry = 30,
 	},
+	/*
+	 * Generic NVMe devices
+	 */
+	{
+		.vendor	       = "NVME",
+		.product       = ".*",
+		.uid_attribute = "ID_WWN",
+		.checker_name  = DIRECTIO,
+	},
 #if 0
 	/*
 	 * Copy this TEMPLATE to add new hardware.

libmultipath/structs.h +1 -0

index 6edd927..dfd65ae 100644
@@ -53,6 +53,7 @@ enum sysfs_buses {
 	SYSFS_BUS_CCW,
 	SYSFS_BUS_CCISS,
 	SYSFS_BUS_RBD,
+	SYSFS_BUS_NVME,
 };
 
 enum pathstates {

Signature · 6 lines

2.5.5

--
dm-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/dm-devel

libmultipath/discovery.c scsi_sysfs_pathinfo (struct path * pp, vector hwtable) · line 1211

Quoted 6 lines

libmultipath/discovery.c +6 -0

+
+	snprintf(pp->vendor_id, SCSI_VENDOR_SIZE, "NVME");
+	snprintf(pp->product_id, SCSI_PRODUCT_SIZE, "%s", udev_device_get_sysattr_value(nvme, "model"));
+	snprintf(pp->serial, SERIAL_SIZE, "%s", udev_device_get_sysattr_value(nvme, "serial"));
+	snprintf(pp->rev, SCSI_REV_SIZE, "%s", udev_device_get_sysattr_value(nvme, "firmware_rev"));
+	snprintf(pp->wwid, WWID_SIZE, "%s", udev_device_get_sysattr_value(pp->udev, "wwid"));

With your udev rules change, you shouldn't need to set the wwid here. It should just get dealt with by the get_uid call in pathinfo (or with the new uevent merging code, before multipathd ever calls pathinfo).

Quoted 3 lines · Keith Busch

These devices are mulitpath capable, and have been able to stack with dm-mpath since kernel 4.2.

libmultipath/blacklist.c setup_default_blist (struct config * conf) · line 172

Quoted 2 lines

libmultipath/blacklist.c +1 -1

-	str = STRDUP("^(ram|raw|loop|fd|md|dm-|sr|scd|st|dcssblk|nvme)[0-9]");
+	str = STRDUP("^(ram|raw|loop|fd|md|dm-|sr|scd|st|dcssblk)[0-9]");

Have you checked whether dm-mpath works properly with nvme? Last time I tried that dm-mpath crashed when it called scsi_dh_attach() because that last function assumes that it is passed a SCSI device queue instead of checking whether or not the queue passed to that function is a SCSI device queue.

Good point. I was unknowingly running with CONFIG_SCSI_DH disabled, and blissfully unaware of its existence! After enabling that option, I see what you mean.

Quoted 3 lines · Keith Busch

Good point. I was unknowingly running with CONFIG_SCSI_DH disabled, and blissfully unaware of its existence! After enabling that option, I see what you mean.

Someone needs to fix that crash ASAP. I though Hannes had patches for them a long time ago in the SuSE tree, but they never went anywhere.

Quoted 5 lines · Christoph Hellwig

Quoted 3 lines · Keith Busch

Good point. I was unknowingly running with CONFIG_SCSI_DH disabled, and blissfully unaware of its existence! After enabling that option, I see what you mean.

Quoted 2 lines

Someone needs to fix that crash ASAP. I though Hannes had patches for them a long time ago in the SuSE tree, but they never went anywhere.

Adding Hannes.

I can work around this by setting the parameters from multipath-tools specific to NVMe, but that seems worng. Blindly calling scsi specific directly from dm-mpath isn't a good idea.

Quoted 4 lines · Bart Van Assche

Quoted 3 lines · Keith Busch

These devices are mulitpath capable, and have been able to stack with dm-mpath since kernel 4.2.

Quoted 8 lines

libmultipath/blacklist.c setup_default_blist (struct config * conf) · line 172

Quoted 2 lines

libmultipath/blacklist.c +1 -1

-	str = STRDUP("^(ram|raw|loop|fd|md|dm-|sr|scd|st|dcssblk|nvme)[0-9]");
+	str = STRDUP("^(ram|raw|loop|fd|md|dm-|sr|scd|st|dcssblk)[0-9]");

Have you checked whether dm-mpath works properly with nvme? Last time I tried that dm-mpath crashed when it called scsi_dh_attach() because that last function assumes that it is passed a SCSI device queue instead of checking whether or not the queue passed to that function is a SCSI device queue.

Good point. I was unknowingly running with CONFIG_SCSI_DH disabled, and blissfully unaware of its existence! After enabling that option, I see what you mean.

If we don't want to mess with the kernel, I can change the multipath-tools to get around that by appending the following to NVMe hwentry in the second patch in this series:

.retain_hwhandler = RETAIN_HWHANDLER_OFF,

And the problem goes away.

Quoted 22 lines · Keith Busch

Quoted 4 lines · Bart Van Assche

Quoted 3 lines · Keith Busch

These devices are mulitpath capable, and have been able to stack with dm-mpath since kernel 4.2.

Quoted 8 lines

libmultipath/blacklist.c setup_default_blist (struct config * conf) · line 172

Quoted 2 lines

libmultipath/blacklist.c +1 -1

-	str = STRDUP("^(ram|raw|loop|fd|md|dm-|sr|scd|st|dcssblk|nvme)[0-9]");
+	str = STRDUP("^(ram|raw|loop|fd|md|dm-|sr|scd|st|dcssblk)[0-9]");

Have you checked whether dm-mpath works properly with nvme? Last time I tried that dm-mpath crashed when it called scsi_dh_attach() because that last function assumes that it is passed a SCSI device queue instead of checking whether or not the queue passed to that function is a SCSI device queue.

Good point. I was unknowingly running with CONFIG_SCSI_DH disabled, and blissfully unaware of its existence! After enabling that option, I see what you mean.

If we don't want to mess with the kernel, I can change the multipath-tools to get around that by appending the following to NVMe hwentry in the second patch in this series:

Quoted 3 lines

.retain_hwhandler = RETAIN_HWHANDLER_OFF,

And the problem goes away.

That gives me a clue, I'll take a look.

These devices are mulitpath capable, and have been able to stack with dm-mpath since kernel 4.2.

Quoted 2 lines · Keith Busch

These devices are mulitpath capable, and have been able to stack with dm-mpath since kernel 4.2.

Can we make this conditional on something? I have native NVMe multipathing almost ready for the next merge window which is a lot easier to use and faster than dm. So I don't want us to be locked into this mode just before that.

Nobody has interest in Linux multipathing becoming fragmented.

If every transport implemented their own multipathing the end-user would be amazingly screwed trying to keep track of all the quirks/configuration/management of each.

Not saying multipath-tools is great, nor that DM multipath is god's gift. But substantiating _why_ you need this "native NVMe multipathing" would go a really long way to justifying your effort.

For starters, how about you show just how much better than DM multipath this native NVMe multipathing performs? NOTE: it'd imply you put effort to making DM multipath work with NVMe.. if you've sat on that code too that'd be amazingly unfortunate/frustrating.

Quoted 10 lines · Mike Snitzer

Nobody has interest in Linux multipathing becoming fragmented.

If every transport implemented their own multipathing the end-user would be amazingly screwed trying to keep track of all the quirks/configuration/management of each.

Not saying multipath-tools is great, nor that DM multipath is god's gift. But substantiating _why_ you need this "native NVMe multipathing" would go a really long way to justifying your effort.

Quoted 4 lines

For starters, how about you show just how much better than DM multipath this native NVMe multipathing performs? NOTE: it'd imply you put effort to making DM multipath work with NVMe.. if you've sat on that code too that'd be amazingly unfortunate/frustrating.

Another question is what your attitude is towards dm-mpath changes? Last time I posted a series of patches that significantly clean up and improve readability of the dm-mpath code you refused to take these upstream.

Weird. I did push back on those changes initially (just felt like churn) but I ultimately did take them:

$ git log --oneline --author=bart drivers/md/dm-mpath.c
6599c84 dm mpath: do not modify *__clone if blk_mq_alloc_request() fails
4813577 dm mpath: change return type of pg_init_all_paths() from int to void
9f4c3f8 dm: convert wait loops to use autoremove_wake_function()

Did I miss any?

Quoted 8 lines · Mike Snitzer

Weird. I did push back on those changes initially (just felt like churn) but I ultimately did take them:

$ git log --oneline --author=bart drivers/md/dm-mpath.c
6599c84 dm mpath: do not modify *__clone if blk_mq_alloc_request() fails
4813577 dm mpath: change return type of pg_init_all_paths() from int to void
9f4c3f8 dm: convert wait loops to use autoremove_wake_function()

Quoted 1 line

Did I miss any?

Hello Mike,

Thank you for having accepted these patches. However, I was not referring to these patches but to the eight patches available at
https://github.com/bvanassche/linux/tree/dm-mpath

Or you can wait to rebase on v4.11-rc1 in ~2 weeks.

going to LSF/MM?). Yet you're expecting to just drop it into the tree without a care in the world about the implications.

Quoted 2 lines · Mike Snitzer

going to LSF/MM?). Yet you're expecting to just drop it into the tree without a care in the world about the implications.

I am planning to post it for comments, and then plan to "drop it in the tree" exactly because I think of the implications.

Keith did that

But once we already do the discovery of the path relations in the transport (e.g scsi_dh) we can just move the path selectors (for which I'm reusing the DM code anyway btw) and the bouncing of I/O to the block layer and cut out the middle man.

The main reason is that things will just work (TM) instead of having to carry around additional userspace to configure a an unneded additional device layer that just causes confusion. Beyond the scsi_dh equivalent there is basically no new code in nvme,

just a little new code in the block layer, and a move of the path selectors from dm to the block layer. I would not call this fragmentation.

Anyway, there is very little point in having an abstract discussion here, I'll try to get the code ready ASAP, although until the end of next week I'm really pressed with other deadlines.

Quoted 3 lines · Christoph Hellwig

just a little new code in the block layer, and a move of the path selectors from dm to the block layer. I would not call this fragmentation.

I'm fine with the path selectors getting moved out; maybe it'll encourage new path selectors to be developed.

But there will need to be some userspace interface stood up to support your native NVMe multipathing (you may not think it needed but think in time there will be a need to configure _something_). That is the fragmentation I'm referring to.

Maybe I'm not seeing the bigger picture. Is there some part to multipath that the kernel is not in a better position to handle?

Quoted 2 lines · Keith Busch

Maybe I'm not seeing the bigger picture. Is there some part to multipath that the kernel is not in a better position to handle?

Does this mean that the code to parse /etc/multipath.conf will be moved into the kernel? Or will we lose the ability to configure the policies that /etc/multipath.conf allows to configure?

Quoted 7 lines · Bart Van Assche

Quoted 2 lines · Keith Busch

Maybe I'm not seeing the bigger picture. Is there some part to multipath that the kernel is not in a better position to handle?

Does this mean that the code to parse /etc/multipath.conf will be moved into the kernel? Or will we lose the ability to configure the policies that /etc/multipath.conf allows to configure?

No, I'm just considering the settings for a device that won't work at all if multipath.conf is wrong. For example, the uuid attributes, path priority, or path checker. These can't be considered configurable policies if all but one of them are invalid for a specific device type.

It shouldn't even be an option to let a user select TUR path checker for NVMe, and the only checkers multipath-tools provide that even make sense for NVMe are deprecated.

Then undeprecate them. Decisions like marking a path checker deprecated were _not_ made with NVMe in mind. They must predate NVMe.

multipath-tools has tables that specify all the defaults for a given target backend. NVMe will just be yet another. Yes some user _could_ shoot themselves in the foot by overriding the proper configuration but since when are we motivated by _not_ giving users the power to hang themselves?

As for configurability (chosing between N valid configs/settings): At some point the user will want one behaviour vs another. Thinking otherwise is just naive. Think error timeouts, etc. Any multipath kernel implementation (which dm-multipath is BTW) will eventually find itself at a crossroads where the underlying fabric could be tweaked in different ways. Thinking you can just hardcode these attributes and settings is foolish.

multipath-tools has tables that specify all the defaults for a given target backend. NVMe will just be yet another.

Quoted 2 lines · Mike Snitzer

multipath-tools has tables that specify all the defaults for a given target backend. NVMe will just be yet another.

No, if we get things right it won't. ALUA already got rid of most of the parameter people would have to set under normal conditions, and I plan to make sure the NVMe equivalent will do it for all parameters. I am active in the NVMe working group and will do my best to get there. There's a few others folks here that are more or less active there as well (Keith, Martin, Jens for example), so I think we have a chance.

That beeing said Keith is right that we'll always have odd setups where we need to overrid things, and we will have to provide tunables for that. It's no different from any other kernel subsystem in that.

I'm fine with the path selectors getting moved out; maybe it'll encourage new path selectors to be developed.

But there will need to be some userspace interface stood up to support your native NVMe multipathing (you may not think it needed but think in time there will be a need to configure _something_). That is the fragmentation I'm referring to.

I guess one config option that we'd need is multibus vs. failover which are used per use-case.

Quoted 2 lines · Sagi Grimberg

I guess one config option that we'd need is multibus vs. failover which are used per use-case.

Which fundamentally is a property of the target first, and it should tell us that. There might be the occasional need for an override, but certainly not locally maintained tables of targets and their preferences.

Not following what you're saying Keith did. Please feel free to clarify.

The middle man is useful if it can support all transports. If it only supports some then yeah the utility is certainly reduced.

I'm going to look at removing any scsi_dh code from DM multipath (someone already proposed removing the 'retain_attached_hw_handler' feature). Not much point having anything in DM multipath now that scsi discovery has the ability to auto-attach the right scsi_dh via scsi_dh's .match hook.

As a side-effect it will fix Keith's scsi_dh crash (when operating on NVMe request_queue).

My hope is that your NVMe equivalent for scsi_dh will "just work" (TM) like scsi_dh auto-attach does. There isn't a finished ALUA equivalent standard for NVMe so I'd imagine at this point you have a single device handler for NVMe to do error translation?

Quoted 2 lines · Mike Snitzer

Not following what you're saying Keith did. Please feel free to clarify.

Keith demonstrated what it takes to support NVMe with dm. He also gave a couple presentations on it in addition to various ptches on the list.

Quoted 2 lines

The middle man is useful if it can support all transports. If it only supports some then yeah the utility is certainly reduced.

Again let's look at what multipathing involves:

- discovery of multiple paths for a device, and path preferences:
Storage protocol specific

- handling of path state and grouping changes:
Storage protocol specific

- handling of path up / down events:
Storage protocol / transport specific if provided

- keep alive / path checking:
Storage protocol specific with possible generic fallback

- path selection:
Generic, although building heavily on protocol / transport specific information

So most of the hard work is transport specific anyway. And I fully agree that generic code should be, well generic. And with generic I mean right in the block layer instead of involving a layer block driver that relies on lots of low-level driver information and setup from user space.

Quoted 5 lines

I'm going to look at removing any scsi_dh code from DM multipath (someone already proposed removing the 'retain_attached_hw_handler' feature). Not much point having anything in DM multipath now that scsi discovery has the ability to auto-attach the right scsi_dh via scsi_dh's .match hook.

Great.

Quoted 2 lines

As a side-effect it will fix Keith's scsi_dh crash (when operating on NVMe request_queue).

I think we'll need to have a quick fix for that ASAP, though.

Quoted 2 lines · Keith Busch

These devices are mulitpath capable, and have been able to stack with dm-mpath since kernel 4.2.

Can we make this conditional on something? I have native NVMe multipathing almost ready for the next merge window which is a lot easier to use and faster than dm. So I don't want us to be locked into this mode just before that.

--
dm-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/dm-devel

These devices are mulitpath capable, and have been able to stack with dm-mpath since kernel 4.2.

Signed-off-by Keith Busch <[email protected]>

---
 libmultipath/blacklist.c   | 2 +-
 multipath/multipath.conf.5 | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

libmultipath/blacklist.c +1 -1

index 36af282..ee396e2 100644
@@ -169,7 +169,7 @@ setup_default_blist (struct config * conf)
 	char * str;
 	int i;
 
-	str = STRDUP("^(ram|raw|loop|fd|md|dm-|sr|scd|st|dcssblk|nvme)[0-9]");
+	str = STRDUP("^(ram|raw|loop|fd|md|dm-|sr|scd|st|dcssblk)[0-9]");
 	if (!str)
 		return 1;
 	if (store_ble(conf->blist_devnode, str, ORIGIN_DEFAULT))

multipath/multipath.conf.5 +0 -0

index 7de8bc7..d6c6c52 100644

These devices are mulitpath capable, and have been able to stack with dm-mpath since kernel 4.2.

Signed-off-by Keith Busch <[email protected]>

---
 libmultipath/blacklist.c   | 2 +-
 multipath/multipath.conf.5 | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

libmultipath/blacklist.c +1 -1

index 36af282..ee396e2 100644
@@ -169,7 +169,7 @@ setup_default_blist (struct config * conf)
        char * str;
        int i;
 
-       str = STRDUP("^(ram|raw|loop|fd|md|dm-|sr|scd|st|dcssblk|nvme)[0-
 9]");
+       str = STRDUP("^(ram|raw|loop|fd|md|dm-|sr|scd|st|dcssblk)[0-9]");
        if (!str)
                return 1;

        if (store_ble(conf->blist_devnode, str, ORIGIN_DEFAULT))

multipath/multipath.conf.5 +0 -1

index 7de8bc7..d6c6c52 100644
@@ -943,7 +943,7 @@ The following keywords are recognized:
 Regular expression of the device nodes to be excluded.
 .RS
 .TP
-The default is: \fB^(ram|raw|loop|fd|md|dm-|sr|scd|st|dcssblk|nvme)[0-9]\fR

and \fB^(td|hd|vd)[a-z]\fR
+The default is: \fB^(ram|raw|loop|fd|md|dm-|sr|scd|st|dcssblk)[0-9]\fR and \fB^(td|hd|vd)[a-z]\fR
.RE
.TP
.B wwid
--
2.5.5