[PATCH 1/5] libmultipath: nvme: fix path detection for kernel 4.16

[PATCH 1/5] libmultipath: nvme: fix path detection for kernel 4.16

[Martin Wilck]

The path detection logic of the NVMe code relies on the "slaves"
symlinks from NVMe subsys to controllers in sysfs, which have
been removed in the 4.16 kernel.

With this patch, we use the symlinks on the NVMe subsys level
instead.

Example: a multipathed NVMe subsystem with 2 controllers:

/sys/devices/virtual/nvme-subsystem/nvme-subsys2/nvme2n1
/sys/devices/virtual/nvme-fabrics/ctl/nvme2/nvme2c6n1
/sys/devices/virtual/nvme-fabrics/ctl/nvme3/nvme2c8n1

The controllers are found from the subsystem like this:

/sys/devices/virtual/nvme-subsystem/nvme-subsys2/nvme2 ->
    ../../nvme-fabrics/ctl/nvme2
/sys/devices/virtual/nvme-subsystem/nvme-subsys2/nvme3 ->
    ../../nvme-fabrics/ctl/nvme3

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/foreign/nvme.c | 153 ++++++++++++++++++++++++++++++------
 1 file changed, 127 insertions(+), 26 deletions(-)

diff --git a/libmultipath/foreign/nvme.c b/libmultipath/foreign/nvme.c
index 280b6bd2..fca3235f 100644
--- a/libmultipath/foreign/nvme.c
+++ b/libmultipath/foreign/nvme.c
@@ -26,6 +26,7 @@
 #include <limits.h>
 #include <dirent.h>
 #include <errno.h>
+#include <ctype.h>
 #include "vector.h"
 #include "generic.h"
 #include "foreign.h"
@@ -442,17 +443,92 @@ _find_path_by_syspath(struct nvme_map *map, const char *syspath)
 	return NULL;
 }
 
-static int no_dotfiles(const struct dirent *di)
+static void _udev_device_unref(void *p)
 {
-	return di->d_name[0] != '.';
+	udev_device_unref(p);
 }
 
-static void _find_slaves(struct context *ctx, struct nvme_map *map)
+static void _udev_enumerate_unref(void *p)
 {
-	char pathbuf[PATH_MAX];
+	udev_enumerate_unref(p);
+}
+
+static int _dirent_controller(const struct dirent *di)
+{
+	static const char nvme_prefix[] = "nvme";
+	const char *p;
+
+#ifdef _DIRENT_HAVE_D_TYPE
+	if (di->d_type != DT_LNK)
+		return 0;
+#endif
+	if (strncmp(di->d_name, nvme_prefix, sizeof(nvme_prefix) - 1))
+		return 0;
+	p = di->d_name + sizeof(nvme_prefix) - 1;
+	if (*p == '\0' || !isdigit(*p))
+		return 0;
+	for (++p; *p != '\0'; ++p)
+		if (!isdigit(*p))
+			return 0;
+	return 1;
+}
+
+/* Find the block device for a given nvme controller */
+struct udev_device *get_ctrl_blkdev(const struct context *ctx,
+				    struct udev_device *ctrl)
+{
+	struct udev_list_entry *item;
+	struct udev_device *blkdev = NULL;
+	struct udev_enumerate *enm = udev_enumerate_new(ctx->udev);
+
+	if (enm == NULL)
+		return NULL;
+
+	pthread_cleanup_push(_udev_enumerate_unref, enm);
+	if (udev_enumerate_add_match_parent(enm, ctrl) < 0)
+		goto out;
+	if (udev_enumerate_add_match_subsystem(enm, "block"))
+		goto out;
+
+	if (udev_enumerate_scan_devices(enm) < 0) {
+		condlog(1, "%s: %s: error enumerating devices", __func__, THIS);
+		goto out;
+	}
+
+	for (item = udev_enumerate_get_list_entry(enm);
+	     item != NULL;
+	     item = udev_list_entry_get_next(item)) {
+		struct udev_device *tmp;
+
+		tmp = udev_device_new_from_syspath(ctx->udev,
+					   udev_list_entry_get_name(item));
+		if (tmp == NULL)
+			continue;
+		if (!strcmp(udev_device_get_devtype(tmp), "disk")) {
+			blkdev = tmp;
+			break;
+		} else
+			udev_device_unref(tmp);
+	}
+
+	if (blkdev == NULL)
+		condlog(1, "%s: %s: failed to get blockdev for %s",
+			__func__, THIS, udev_device_get_sysname(ctrl));
+	else
+		condlog(5, "%s: %s: got %s", __func__, THIS,
+			udev_device_get_sysname(blkdev));
+out:
+	pthread_cleanup_pop(1);
+	return blkdev;
+}
+
+static void _find_controllers(struct context *ctx, struct nvme_map *map)
+{
+	char pathbuf[PATH_MAX], realbuf[PATH_MAX];
 	struct dirent **di = NULL;
+	struct udev_device *subsys;
 	struct nvme_path *path;
-	int r, i;
+	int r, i, n;
 
 	if (map == NULL || map->udev == NULL)
 		return;
@@ -460,33 +536,65 @@ static void _find_slaves(struct context *ctx, struct nvme_map *map)
 	vector_foreach_slot(map->pathvec, path, i)
 		path->seen = false;
 
-	snprintf(pathbuf, sizeof(pathbuf),
-		"%s/slaves",
-		udev_device_get_syspath(map->udev));
+	subsys = udev_device_get_parent_with_subsystem_devtype(map->udev,
+							       "nvme-subsystem",
+							       NULL);
+	if (subsys == NULL) {
+		condlog(1, "%s: %s: BUG: no NVME subsys for %s", __func__, THIS,
+			udev_device_get_sysname(map->udev));
+		return;
+	}
 
-	r = scandir(pathbuf, &di, no_dotfiles, alphasort);
+	n = snprintf(pathbuf, sizeof(pathbuf), "%s",
+		     udev_device_get_syspath(subsys));
+	r = scandir(pathbuf, &di, _dirent_controller, alphasort);
 
 	if (r == 0) {
-		condlog(3, "%s: %s: no paths for %s", __func__, THIS,
+		condlog(3, "%s: %s: no controllers for %s", __func__, THIS,
 			udev_device_get_sysname(map->udev));
 		return;
 	} else if (r < 0) {
-		condlog(1, "%s: %s: error %d scanning paths of %s", __func__,
-			THIS, errno, udev_device_get_sysname(map->udev));
+		condlog(1, "%s: %s: error %d scanning controllers of %s",
+			__func__, THIS, errno,
+			udev_device_get_sysname(map->udev));
 		return;
 	}
 
 	pthread_cleanup_push(free, di);
 	for (i = 0; i < r; i++) {
 		char *fn = di[i]->d_name;
-		struct udev_device *udev;
+		struct udev_device *ctrl, *udev;
+
+		if (snprintf(pathbuf + n, sizeof(pathbuf) - n, "/%s", fn)
+		    >= sizeof(pathbuf) - n)
+			continue;
+		if (realpath(pathbuf, realbuf) == NULL) {
+			condlog(3, "%s: %s: realpath: %s", __func__, THIS,
+				strerror(errno));
+			continue;
+		}
+		condlog(4, "%s: %s: found %s", __func__, THIS, realbuf);
 
-		if (snprintf(pathbuf, sizeof(pathbuf), "%s/slaves/%s",
-			     udev_device_get_syspath(map->udev), fn)
-		    >= sizeof(pathbuf))
+		ctrl = udev_device_new_from_syspath(ctx->udev, realbuf);
+		if (ctrl == NULL) {
+			condlog(1, "%s: %s: failed to get udev device for %s",
+				__func__, THIS, realbuf);
+			continue;
+		}
+
+		pthread_cleanup_push(_udev_device_unref, ctrl);
+		udev = get_ctrl_blkdev(ctx, ctrl);
+		/*
+		 * We give up the reference to the nvme device here and get
+		 * it back from the child below.
+		 * This way we don't need to worry about unreffing it.
+		 */
+		pthread_cleanup_pop(1);
+
+		if (udev == NULL)
 			continue;
 
-		path = _find_path_by_syspath(map, pathbuf);
+		path = _find_path_by_syspath(map, udev_device_get_syspath(udev));
 		if (path != NULL) {
 			path->seen = true;
 			condlog(4, "%s: %s already known",
@@ -494,13 +602,6 @@ static void _find_slaves(struct context *ctx, struct nvme_map *map)
 			continue;
 		}
 
-		udev = udev_device_new_from_syspath(ctx->udev, pathbuf);
-		if (udev == NULL) {
-			condlog(1, "%s: %s: failed to get udev device for %s",
-				__func__, THIS, fn);
-			continue;
-		}
-
 		path = calloc(1, sizeof(*path));
 		if (path == NULL)
 			continue;
@@ -591,7 +692,7 @@ static int _add_map(struct context *ctx, struct udev_device *ud,
 		return FOREIGN_ERR;
 	}
 	vector_set_slot(ctx->mpvec, map);
-	_find_slaves(ctx, map);
+	_find_controllers(ctx, map);
 
 	return FOREIGN_CLAIMED;
 }
@@ -688,7 +789,7 @@ void _check(struct context *ctx)
 	vector_foreach_slot(ctx->mpvec, gm, i) {
 		struct nvme_map *map = gen_mp_to_nvme(gm);
 
-		_find_slaves(ctx, map);
+		_find_controllers(ctx, map);
 	}
 }
 
-- 
2.18.0

[PATCH 2/5] libmultipath: nvme: shorter topology output

[Martin Wilck]

The nvme foreign code maps the NVMe subsys NQN to the "%n"
wildcard ("alias"). Some real-world devices use very lengthy
expressions for the subsys NQN (counted 95 characters on one
system here), making the "multipath -ll" output hardly readable
for humans. Use a shorter and more concise printout instead,
based on the WWID only.

The subsys NQN is still available via the "%n" wildcard:
"multipathd show maps format %n".

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/foreign/nvme.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/libmultipath/foreign/nvme.c b/libmultipath/foreign/nvme.c
index fca3235f..8887a755 100644
--- a/libmultipath/foreign/nvme.c
+++ b/libmultipath/foreign/nvme.c
@@ -286,10 +286,18 @@ static int snprint_nvme_path(const struct gen_path *gp,
 	return 0;
 }
 
+static int nvme_style(const struct gen_multipath* gm,
+		      char *buf, int len, int verbosity)
+{
+	int n = snprintf(buf, len, "%%w [%%G]:%%d %%s");
+
+	return (n < len ? n : len - 1);
+}
+
 static const struct gen_multipath_ops nvme_map_ops = {
 	.get_pathgroups = nvme_mp_get_pgs,
 	.rel_pathgroups = nvme_mp_rel_pgs,
-	.style = generic_style,
+	.style = nvme_style,
 	.snprint = snprint_nvme_map,
 };
 
-- 
2.18.0

Re: [PATCH 2/5] libmultipath: nvme: shorter topology output

[Benjamin Marzinski]

On Fri, Sep 14, 2018 at 02:51:00PM +0200, Martin Wilck wrote:

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

> The nvme foreign code maps the NVMe subsys NQN to the "%n"
> wildcard ("alias"). Some real-world devices use very lengthy
> expressions for the subsys NQN (counted 95 characters on one
> system here), making the "multipath -ll" output hardly readable
> for humans. Use a shorter and more concise printout instead,
> based on the WWID only.
> 
> The subsys NQN is still available via the "%n" wildcard:
> "multipathd show maps format %n".
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/foreign/nvme.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/libmultipath/foreign/nvme.c b/libmultipath/foreign/nvme.c
> index fca3235f..8887a755 100644
> --- a/libmultipath/foreign/nvme.c
> +++ b/libmultipath/foreign/nvme.c
> @@ -286,10 +286,18 @@ static int snprint_nvme_path(const struct gen_path *gp,
>  	return 0;
>  }
>  
> +static int nvme_style(const struct gen_multipath* gm,
> +		      char *buf, int len, int verbosity)
> +{
> +	int n = snprintf(buf, len, "%%w [%%G]:%%d %%s");
> +
> +	return (n < len ? n : len - 1);
> +}
> +
>  static const struct gen_multipath_ops nvme_map_ops = {
>  	.get_pathgroups = nvme_mp_get_pgs,
>  	.rel_pathgroups = nvme_mp_rel_pgs,
> -	.style = generic_style,
> +	.style = nvme_style,
>  	.snprint = snprint_nvme_map,
>  };
>  
> -- 
> 2.18.0

[PATCH 3/5] libmultipath: pathinfo: skip hidden devices

[Martin Wilck]

Hidden block devices (in practice: members of nvme native multipath
devices) can't be used by multipath anyway. Current multipath code
(with default blacklisting) skips them, too, but emits a misleading
"blacklisted: udev property missing" message.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/discovery.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 0b1855dd..11da64ba 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1858,9 +1858,18 @@ int pathinfo(struct path *pp, struct config *conf, int mask)
 	 * limited by DI_BLACKLIST and occurs before this debug
 	 * message with the mask value.
 	 */
-	if (pp->udev && (is_claimed_by_foreign(pp->udev) ||
-			 filter_property(conf, pp->udev) > 0))
-		return PATHINFO_SKIPPED;
+	if (pp->udev) {
+		const char *hidden =
+			udev_device_get_sysattr_value(pp->udev, "hidden");
+
+		if (hidden && !strcmp(hidden, "1")) {
+			condlog(3, "%s: hidden", pp->dev);
+			return PATHINFO_SKIPPED;
+		}
+		if (is_claimed_by_foreign(pp->udev) ||
+			 filter_property(conf, pp->udev) > 0)
+			return PATHINFO_SKIPPED;
+	}
 
 	if (filter_devnode(conf->blist_devnode,
 			   conf->elist_devnode,
-- 
2.18.0

Re: [PATCH 3/5] libmultipath: pathinfo: skip hidden devices

[Benjamin Marzinski]

On Fri, Sep 14, 2018 at 02:51:01PM +0200, Martin Wilck wrote:

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

> Hidden block devices (in practice: members of nvme native multipath
> devices) can't be used by multipath anyway. Current multipath code
> (with default blacklisting) skips them, too, but emits a misleading
> "blacklisted: udev property missing" message.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/discovery.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 0b1855dd..11da64ba 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -1858,9 +1858,18 @@ int pathinfo(struct path *pp, struct config *conf, int mask)
>  	 * limited by DI_BLACKLIST and occurs before this debug
>  	 * message with the mask value.
>  	 */
> -	if (pp->udev && (is_claimed_by_foreign(pp->udev) ||
> -			 filter_property(conf, pp->udev) > 0))
> -		return PATHINFO_SKIPPED;
> +	if (pp->udev) {
> +		const char *hidden =
> +			udev_device_get_sysattr_value(pp->udev, "hidden");
> +
> +		if (hidden && !strcmp(hidden, "1")) {
> +			condlog(3, "%s: hidden", pp->dev);
> +			return PATHINFO_SKIPPED;
> +		}
> +		if (is_claimed_by_foreign(pp->udev) ||
> +			 filter_property(conf, pp->udev) > 0)
> +			return PATHINFO_SKIPPED;
> +	}
>  
>  	if (filter_devnode(conf->blist_devnode,
>  			   conf->elist_devnode,
> -- 
> 2.18.0

[PATCH 4/5] multipathd: fix version check for DM_DEV_ARM_POLL ioctl

[Martin Wilck]

Make sure the checks in dmevent_poll_supported() and
arm_dm_event_poll() use the same logic. Currently, the
version check check fails in arm_dm_event_poll() if
libdevmapper's DM_VERSION is newer than the kernel's.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/dmevents.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/multipathd/dmevents.c b/multipathd/dmevents.c
index e98a974c..31e64a7e 100644
--- a/multipathd/dmevents.c
+++ b/multipathd/dmevents.c
@@ -50,16 +50,20 @@ struct dmevent_waiter {
 };
 
 static struct dmevent_waiter *waiter;
+/*
+ * DM_VERSION_MINOR hasn't been updated when DM_DEV_ARM_POLL
+ * was added in kernel 4.13. 4.37.0 (4.14) has it, safely.
+ */
+static const unsigned int DM_VERSION_FOR_ARM_POLL[] = {4, 37, 0};
 
 int dmevent_poll_supported(void)
 {
-	unsigned int minv[3] = {4, 37, 0};
 	unsigned int v[3];
 
 	if (dm_drv_version(v))
 		return 0;
 
-	if (VERSION_GE(v, minv))
+	if (VERSION_GE(v, DM_VERSION_FOR_ARM_POLL))
 		return 1;
 	return 0;
 }
@@ -120,9 +124,9 @@ static int arm_dm_event_poll(int fd)
 {
 	struct dm_ioctl dmi;
 	memset(&dmi, 0, sizeof(dmi));
-	dmi.version[0] = DM_VERSION_MAJOR;
-	dmi.version[1] = DM_VERSION_MINOR;
-	dmi.version[2] = DM_VERSION_PATCHLEVEL;
+	dmi.version[0] = DM_VERSION_FOR_ARM_POLL[0];
+	dmi.version[1] = DM_VERSION_FOR_ARM_POLL[1];
+	dmi.version[2] = DM_VERSION_FOR_ARM_POLL[2];
 	/* This flag currently does nothing. It simply exists to
 	 * duplicate the behavior of libdevmapper */
 	dmi.flags = 0x4;
-- 
2.18.0

Re: [PATCH 4/5] multipathd: fix version check for DM_DEV_ARM_POLL ioctl

[Benjamin Marzinski]

On Fri, Sep 14, 2018 at 02:51:02PM +0200, Martin Wilck wrote:

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

> Make sure the checks in dmevent_poll_supported() and
> arm_dm_event_poll() use the same logic. Currently, the
> version check check fails in arm_dm_event_poll() if
> libdevmapper's DM_VERSION is newer than the kernel's.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipathd/dmevents.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/multipathd/dmevents.c b/multipathd/dmevents.c
> index e98a974c..31e64a7e 100644
> --- a/multipathd/dmevents.c
> +++ b/multipathd/dmevents.c
> @@ -50,16 +50,20 @@ struct dmevent_waiter {
>  };
>  
>  static struct dmevent_waiter *waiter;
> +/*
> + * DM_VERSION_MINOR hasn't been updated when DM_DEV_ARM_POLL
> + * was added in kernel 4.13. 4.37.0 (4.14) has it, safely.
> + */
> +static const unsigned int DM_VERSION_FOR_ARM_POLL[] = {4, 37, 0};
>  
>  int dmevent_poll_supported(void)
>  {
> -	unsigned int minv[3] = {4, 37, 0};
>  	unsigned int v[3];
>  
>  	if (dm_drv_version(v))
>  		return 0;
>  
> -	if (VERSION_GE(v, minv))
> +	if (VERSION_GE(v, DM_VERSION_FOR_ARM_POLL))
>  		return 1;
>  	return 0;
>  }
> @@ -120,9 +124,9 @@ static int arm_dm_event_poll(int fd)
>  {
>  	struct dm_ioctl dmi;
>  	memset(&dmi, 0, sizeof(dmi));
> -	dmi.version[0] = DM_VERSION_MAJOR;
> -	dmi.version[1] = DM_VERSION_MINOR;
> -	dmi.version[2] = DM_VERSION_PATCHLEVEL;
> +	dmi.version[0] = DM_VERSION_FOR_ARM_POLL[0];
> +	dmi.version[1] = DM_VERSION_FOR_ARM_POLL[1];
> +	dmi.version[2] = DM_VERSION_FOR_ARM_POLL[2];
>  	/* This flag currently does nothing. It simply exists to
>  	 * duplicate the behavior of libdevmapper */
>  	dmi.flags = 0x4;
> -- 
> 2.18.0

[PATCH 5/5] multipathd: decrease log level of uevent messages

[Martin Wilck]

The messages "multipathd: add path (uevent)" etc. are displayed
frequently, and often for devices that don't matter for multipathd.
If real action needs to be taken, such as adding or removing paths
or maps from the internal structures, multipathd emits other log
messages at level 2 later on.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index cc493c18..bc95c65f 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -743,7 +743,7 @@ uev_remove_map (struct uevent * uev, struct vectors * vecs)
 	int minor;
 	struct multipath *mpp;
 
-	condlog(2, "%s: remove map (uevent)", uev->kernel);
+	condlog(3, "%s: remove map (uevent)", uev->kernel);
 	alias = uevent_get_dm_name(uev);
 	if (!alias) {
 		condlog(3, "%s: No DM_NAME in uevent, ignoring", uev->kernel);
@@ -803,7 +803,7 @@ uev_add_path (struct uevent *uev, struct vectors * vecs, int need_do_map)
 	int ret = 0, i;
 	struct config *conf;
 
-	condlog(2, "%s: add path (uevent)", uev->kernel);
+	condlog(3, "%s: add path (uevent)", uev->kernel);
 	if (strstr(uev->kernel, "..") != NULL) {
 		/*
 		 * Don't allow relative device names in the pathvec
@@ -911,7 +911,8 @@ ev_add_path (struct path * pp, struct vectors * vecs, int need_do_map)
 	     (pathcount(mpp, PATH_GHOST) > 0 && pp->tpgs != TPGS_IMPLICIT &&
 	      mpp->ghost_delay_tick <= 0))) {
 		/* if wait_for_udev is set and valid paths exist */
-		condlog(2, "%s: delaying path addition until %s is fully initialized", pp->dev, mpp->alias);
+		condlog(3, "%s: delaying path addition until %s is fully initialized",
+			pp->dev, mpp->alias);
 		mpp->wait_for_udev = 2;
 		orphan_path(pp, "waiting for create to complete");
 		return 0;
@@ -1038,7 +1039,7 @@ uev_remove_path (struct uevent *uev, struct vectors * vecs, int need_do_map)
 	struct path *pp;
 	int ret;
 
-	condlog(2, "%s: remove path (uevent)", uev->kernel);
+	condlog(3, "%s: remove path (uevent)", uev->kernel);
 	delete_foreign(uev->udev);
 
 	pthread_cleanup_push(cleanup_lock, &vecs->lock);
-- 
2.18.0

Re: [PATCH 5/5] multipathd: decrease log level of uevent messages

[Benjamin Marzinski]

On Fri, Sep 14, 2018 at 02:51:03PM +0200, Martin Wilck wrote:

I agree that these messages are largely useless for users, but I often
find them helpful to figure out what multipath was doing when things go
badly.  I'm not against this patch going in. I just wish that log there
was a log level between 2 and 3 that just included notifications about
what was happening, instead of having to go to level 3 to get the
information, which adds a lot of debugging noise. Any way

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

> The messages "multipathd: add path (uevent)" etc. are displayed
> frequently, and often for devices that don't matter for multipathd.
> If real action needs to be taken, such as adding or removing paths
> or maps from the internal structures, multipathd emits other log
> messages at level 2 later on.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipathd/main.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index cc493c18..bc95c65f 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -743,7 +743,7 @@ uev_remove_map (struct uevent * uev, struct vectors * vecs)
>  	int minor;
>  	struct multipath *mpp;
>  
> -	condlog(2, "%s: remove map (uevent)", uev->kernel);
> +	condlog(3, "%s: remove map (uevent)", uev->kernel);
>  	alias = uevent_get_dm_name(uev);
>  	if (!alias) {
>  		condlog(3, "%s: No DM_NAME in uevent, ignoring", uev->kernel);
> @@ -803,7 +803,7 @@ uev_add_path (struct uevent *uev, struct vectors * vecs, int need_do_map)
>  	int ret = 0, i;
>  	struct config *conf;
>  
> -	condlog(2, "%s: add path (uevent)", uev->kernel);
> +	condlog(3, "%s: add path (uevent)", uev->kernel);
>  	if (strstr(uev->kernel, "..") != NULL) {
>  		/*
>  		 * Don't allow relative device names in the pathvec
> @@ -911,7 +911,8 @@ ev_add_path (struct path * pp, struct vectors * vecs, int need_do_map)
>  	     (pathcount(mpp, PATH_GHOST) > 0 && pp->tpgs != TPGS_IMPLICIT &&
>  	      mpp->ghost_delay_tick <= 0))) {
>  		/* if wait_for_udev is set and valid paths exist */
> -		condlog(2, "%s: delaying path addition until %s is fully initialized", pp->dev, mpp->alias);
> +		condlog(3, "%s: delaying path addition until %s is fully initialized",
> +			pp->dev, mpp->alias);
>  		mpp->wait_for_udev = 2;
>  		orphan_path(pp, "waiting for create to complete");
>  		return 0;
> @@ -1038,7 +1039,7 @@ uev_remove_path (struct uevent *uev, struct vectors * vecs, int need_do_map)
>  	struct path *pp;
>  	int ret;
>  
> -	condlog(2, "%s: remove path (uevent)", uev->kernel);
> +	condlog(3, "%s: remove path (uevent)", uev->kernel);
>  	delete_foreign(uev->udev);
>  
>  	pthread_cleanup_push(cleanup_lock, &vecs->lock);
> -- 
> 2.18.0

Re: [PATCH 1/5] libmultipath: nvme: fix path detection for kernel 4.16

[Benjamin Marzinski]

On Fri, Sep 14, 2018 at 02:50:59PM +0200, Martin Wilck wrote:

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

> The path detection logic of the NVMe code relies on the "slaves"
> symlinks from NVMe subsys to controllers in sysfs, which have
> been removed in the 4.16 kernel.
> 
> With this patch, we use the symlinks on the NVMe subsys level
> instead.
> 
> Example: a multipathed NVMe subsystem with 2 controllers:
> 
> /sys/devices/virtual/nvme-subsystem/nvme-subsys2/nvme2n1
> /sys/devices/virtual/nvme-fabrics/ctl/nvme2/nvme2c6n1
> /sys/devices/virtual/nvme-fabrics/ctl/nvme3/nvme2c8n1
> 
> The controllers are found from the subsystem like this:
> 
> /sys/devices/virtual/nvme-subsystem/nvme-subsys2/nvme2 ->
>     ../../nvme-fabrics/ctl/nvme2
> /sys/devices/virtual/nvme-subsystem/nvme-subsys2/nvme3 ->
>     ../../nvme-fabrics/ctl/nvme3
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/foreign/nvme.c | 153 ++++++++++++++++++++++++++++++------
>  1 file changed, 127 insertions(+), 26 deletions(-)
> 
> diff --git a/libmultipath/foreign/nvme.c b/libmultipath/foreign/nvme.c
> index 280b6bd2..fca3235f 100644
> --- a/libmultipath/foreign/nvme.c
> +++ b/libmultipath/foreign/nvme.c
> @@ -26,6 +26,7 @@
>  #include <limits.h>
>  #include <dirent.h>
>  #include <errno.h>
> +#include <ctype.h>
>  #include "vector.h"
>  #include "generic.h"
>  #include "foreign.h"
> @@ -442,17 +443,92 @@ _find_path_by_syspath(struct nvme_map *map, const char *syspath)
>  	return NULL;
>  }
>  
> -static int no_dotfiles(const struct dirent *di)
> +static void _udev_device_unref(void *p)
>  {
> -	return di->d_name[0] != '.';
> +	udev_device_unref(p);
>  }
>  
> -static void _find_slaves(struct context *ctx, struct nvme_map *map)
> +static void _udev_enumerate_unref(void *p)
>  {
> -	char pathbuf[PATH_MAX];
> +	udev_enumerate_unref(p);
> +}
> +
> +static int _dirent_controller(const struct dirent *di)
> +{
> +	static const char nvme_prefix[] = "nvme";
> +	const char *p;
> +
> +#ifdef _DIRENT_HAVE_D_TYPE
> +	if (di->d_type != DT_LNK)
> +		return 0;
> +#endif
> +	if (strncmp(di->d_name, nvme_prefix, sizeof(nvme_prefix) - 1))
> +		return 0;
> +	p = di->d_name + sizeof(nvme_prefix) - 1;
> +	if (*p == '\0' || !isdigit(*p))
> +		return 0;
> +	for (++p; *p != '\0'; ++p)
> +		if (!isdigit(*p))
> +			return 0;
> +	return 1;
> +}
> +
> +/* Find the block device for a given nvme controller */
> +struct udev_device *get_ctrl_blkdev(const struct context *ctx,
> +				    struct udev_device *ctrl)
> +{
> +	struct udev_list_entry *item;
> +	struct udev_device *blkdev = NULL;
> +	struct udev_enumerate *enm = udev_enumerate_new(ctx->udev);
> +
> +	if (enm == NULL)
> +		return NULL;
> +
> +	pthread_cleanup_push(_udev_enumerate_unref, enm);
> +	if (udev_enumerate_add_match_parent(enm, ctrl) < 0)
> +		goto out;
> +	if (udev_enumerate_add_match_subsystem(enm, "block"))
> +		goto out;
> +
> +	if (udev_enumerate_scan_devices(enm) < 0) {
> +		condlog(1, "%s: %s: error enumerating devices", __func__, THIS);
> +		goto out;
> +	}
> +
> +	for (item = udev_enumerate_get_list_entry(enm);
> +	     item != NULL;
> +	     item = udev_list_entry_get_next(item)) {
> +		struct udev_device *tmp;
> +
> +		tmp = udev_device_new_from_syspath(ctx->udev,
> +					   udev_list_entry_get_name(item));
> +		if (tmp == NULL)
> +			continue;
> +		if (!strcmp(udev_device_get_devtype(tmp), "disk")) {
> +			blkdev = tmp;
> +			break;
> +		} else
> +			udev_device_unref(tmp);
> +	}
> +
> +	if (blkdev == NULL)
> +		condlog(1, "%s: %s: failed to get blockdev for %s",
> +			__func__, THIS, udev_device_get_sysname(ctrl));
> +	else
> +		condlog(5, "%s: %s: got %s", __func__, THIS,
> +			udev_device_get_sysname(blkdev));
> +out:
> +	pthread_cleanup_pop(1);
> +	return blkdev;
> +}
> +
> +static void _find_controllers(struct context *ctx, struct nvme_map *map)
> +{
> +	char pathbuf[PATH_MAX], realbuf[PATH_MAX];
>  	struct dirent **di = NULL;
> +	struct udev_device *subsys;
>  	struct nvme_path *path;
> -	int r, i;
> +	int r, i, n;
>  
>  	if (map == NULL || map->udev == NULL)
>  		return;
> @@ -460,33 +536,65 @@ static void _find_slaves(struct context *ctx, struct nvme_map *map)
>  	vector_foreach_slot(map->pathvec, path, i)
>  		path->seen = false;
>  
> -	snprintf(pathbuf, sizeof(pathbuf),
> -		"%s/slaves",
> -		udev_device_get_syspath(map->udev));
> +	subsys = udev_device_get_parent_with_subsystem_devtype(map->udev,
> +							       "nvme-subsystem",
> +							       NULL);
> +	if (subsys == NULL) {
> +		condlog(1, "%s: %s: BUG: no NVME subsys for %s", __func__, THIS,
> +			udev_device_get_sysname(map->udev));
> +		return;
> +	}
>  
> -	r = scandir(pathbuf, &di, no_dotfiles, alphasort);
> +	n = snprintf(pathbuf, sizeof(pathbuf), "%s",
> +		     udev_device_get_syspath(subsys));
> +	r = scandir(pathbuf, &di, _dirent_controller, alphasort);
>  
>  	if (r == 0) {
> -		condlog(3, "%s: %s: no paths for %s", __func__, THIS,
> +		condlog(3, "%s: %s: no controllers for %s", __func__, THIS,
>  			udev_device_get_sysname(map->udev));
>  		return;
>  	} else if (r < 0) {
> -		condlog(1, "%s: %s: error %d scanning paths of %s", __func__,
> -			THIS, errno, udev_device_get_sysname(map->udev));
> +		condlog(1, "%s: %s: error %d scanning controllers of %s",
> +			__func__, THIS, errno,
> +			udev_device_get_sysname(map->udev));
>  		return;
>  	}
>  
>  	pthread_cleanup_push(free, di);
>  	for (i = 0; i < r; i++) {
>  		char *fn = di[i]->d_name;
> -		struct udev_device *udev;
> +		struct udev_device *ctrl, *udev;
> +
> +		if (snprintf(pathbuf + n, sizeof(pathbuf) - n, "/%s", fn)
> +		    >= sizeof(pathbuf) - n)
> +			continue;
> +		if (realpath(pathbuf, realbuf) == NULL) {
> +			condlog(3, "%s: %s: realpath: %s", __func__, THIS,
> +				strerror(errno));
> +			continue;
> +		}
> +		condlog(4, "%s: %s: found %s", __func__, THIS, realbuf);
>  
> -		if (snprintf(pathbuf, sizeof(pathbuf), "%s/slaves/%s",
> -			     udev_device_get_syspath(map->udev), fn)
> -		    >= sizeof(pathbuf))
> +		ctrl = udev_device_new_from_syspath(ctx->udev, realbuf);
> +		if (ctrl == NULL) {
> +			condlog(1, "%s: %s: failed to get udev device for %s",
> +				__func__, THIS, realbuf);
> +			continue;
> +		}
> +
> +		pthread_cleanup_push(_udev_device_unref, ctrl);
> +		udev = get_ctrl_blkdev(ctx, ctrl);
> +		/*
> +		 * We give up the reference to the nvme device here and get
> +		 * it back from the child below.
> +		 * This way we don't need to worry about unreffing it.
> +		 */
> +		pthread_cleanup_pop(1);
> +
> +		if (udev == NULL)
>  			continue;
>  
> -		path = _find_path_by_syspath(map, pathbuf);
> +		path = _find_path_by_syspath(map, udev_device_get_syspath(udev));
>  		if (path != NULL) {
>  			path->seen = true;
>  			condlog(4, "%s: %s already known",
> @@ -494,13 +602,6 @@ static void _find_slaves(struct context *ctx, struct nvme_map *map)
>  			continue;
>  		}
>  
> -		udev = udev_device_new_from_syspath(ctx->udev, pathbuf);
> -		if (udev == NULL) {
> -			condlog(1, "%s: %s: failed to get udev device for %s",
> -				__func__, THIS, fn);
> -			continue;
> -		}
> -
>  		path = calloc(1, sizeof(*path));
>  		if (path == NULL)
>  			continue;
> @@ -591,7 +692,7 @@ static int _add_map(struct context *ctx, struct udev_device *ud,
>  		return FOREIGN_ERR;
>  	}
>  	vector_set_slot(ctx->mpvec, map);
> -	_find_slaves(ctx, map);
> +	_find_controllers(ctx, map);
>  
>  	return FOREIGN_CLAIMED;
>  }
> @@ -688,7 +789,7 @@ void _check(struct context *ctx)
>  	vector_foreach_slot(ctx->mpvec, gm, i) {
>  		struct nvme_map *map = gen_mp_to_nvme(gm);
>  
> -		_find_slaves(ctx, map);
> +		_find_controllers(ctx, map);
>  	}
>  }
>  
> -- 
> 2.18.0