[PATCH 00/80] multipath-tools series part VI: incomplete udev initialization

[PATCH 00/80] multipath-tools series part VI: incomplete udev initialization

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Hi Christophe, hi Ben,

This is part VI of a larger patch series for multipath-tools I've been preparing.
It's based on the previously submitted part V.

The full series will also be available here:
https://github.com/mwilck/multipath-tools/tree/ups/submit-2007

There are tags in that repo for each part of the series.
This part is tagged "submit-200709-6".

The series handles an issue observed in certain partner installations, where
DM devices were incompletely initialized by udev - during initrd procesing,
the "add" event had been processed, but the subsequent "change" event had not,
because udevd had been killed before getting around to handle them.

My first attempt to fix this was based on udev rules ("11-dm-mpath.rules: Fix
udev rule processing during coldplug"), but this patch was wrong. We have to
add logic in multipathd itself. The most important patch in the series that
fixed the actual customer problem is patch 76. Patch 75 was supposed to handle
a slightly different incarnation of the same problem, which so far hasn't been
actually observed. But I think having this patch "just in case" doesn't hurt,
either.

Patch 78 and 79 rename "update_path_groups()", which over time has grown
to be the main entry point for reloading maps.

Patch 80 fixes an issue which I observed while testing the first 3 patches.

Regards,
Martin

Martin Wilck (6):
  multipathd: uev_trigger(): handle incomplete ADD events
  libmultipath: select_action(): force udev reload for uninitialized
    maps
  libmultipath: log dm_task_run() errors
  libmultipath: move reload_map() to multipathd
  multipathd: rename update_path_groups() -> reload_and_sync_map()
  libmultipath: select_action(): don't drop map if alias clashes

 libmultipath/configure.c  | 112 +++++++++++++++-----------------------
 libmultipath/configure.h  |   3 +-
 libmultipath/devmapper.c  |  61 +++++++++++++++++----
 libmultipath/devmapper.h  |   4 ++
 multipathd/cli_handlers.c |   8 +--
 multipathd/dmevents.c     |   4 +-
 multipathd/main.c         |  71 ++++++++++++++++++++++--
 multipathd/main.h         |   4 +-
 multipathd/waiter.c       |   2 +
 9 files changed, 178 insertions(+), 91 deletions(-)

-- 
2.26.2

[PATCH 75/80] multipathd: uev_trigger(): handle incomplete ADD events

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Udev may be killed after handling the ADD event for a multipath map,
but before handling the subsequent CHANGE event that populates the
udev data base with the device properties (e.g. during initrd processing).
If this happens, the ADD uevent sent during coldplug will only provide a
subset of the device properties. We need another CHANGE event to make the map
available to the system.

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

diff --git a/multipathd/main.c b/multipathd/main.c
index 66ca4e3..29227cd 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1508,6 +1508,31 @@ uev_trigger (struct uevent * uev, void * trigger_data)
 			uev_pathfail_check(uev, vecs);
 		} else if (!strncmp(uev->action, "remove", 6)) {
 			r = uev_remove_map(uev, vecs);
+		} else if (!strncmp(uev->action, "add", 3)) {
+			const char *ev_name;
+			char *dm_name;
+			int major = -1, minor = -1;
+
+			/*
+			 * If DM_NAME is not set for a valid map, trigger a
+			 * change event. This can happen during coldplug
+			 * if udev was killed between handling the 'add' and
+			 * 'change' events before.
+			 */
+			ev_name = uevent_get_dm_name(uev);
+			if (!ev_name) {
+				major = uevent_get_major(uev);
+				minor = uevent_get_minor(uev);
+				dm_name = dm_mapname(major, minor);
+				if (dm_name && *dm_name) {
+					condlog(2, "%s: received incomplete 'add' uevent, triggering change",
+						dm_name);
+					udev_device_set_sysattr_value(uev->udev,
+								      "uevent",
+								      "change");
+					free(dm_name);
+				}
+			}
 		}
 		goto out;
 	}
-- 
2.26.2

[PATCH 76/80] libmultipath: select_action(): force udev reload for uninitialized maps

[mwilck]

From: Martin Wilck <mwilck@suse.com>

If we are in the reconfigure() code path, and we encounter maps to
be reloaded, we usually set the DM_SUBSYSTEM_UDEV_FLAG0 flag to tell
udev not to repeat device detection steps above the multipath layer.
However, if the map was previously uninitialized, we have to force
udev to reload.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c | 61 ++++++++++++++++++++++++----------------
 1 file changed, 37 insertions(+), 24 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 2509053..efb5808 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -660,6 +660,32 @@ sysfs_set_max_sectors_kb(struct multipath *mpp, int is_reload)
 	return err;
 }
 
+static void
+select_reload_action(struct multipath *mpp, const char *reason)
+{
+	struct udev_device *mpp_ud;
+	const char *env;
+
+	/*
+	 * MPATH_DEVICE_READY != 1 can mean two things:
+	 *  (a) no usable paths
+	 *  (b) device was never fully processed (e.g. udev killed)
+	 * If we are in this code path (startup or forced reconfigure),
+	 * (b) can mean that upper layers like kpartx have never been
+	 * run for this map. Thus force udev reload.
+	 */
+
+	mpp_ud = get_udev_for_mpp(mpp);
+	env = udev_device_get_property_value(mpp_ud, "MPATH_DEVICE_READY");
+	if (!env || strcmp(env, "1"))
+		mpp->force_udev_reload = 1;
+	udev_device_unref(mpp_ud);
+	mpp->action = ACT_RELOAD;
+	condlog(3, "%s: set ACT_RELOAD (%s%s)", mpp->alias,
+		mpp->force_udev_reload ? "forced, " : "",
+		reason);
+}
+
 static void
 select_action (struct multipath * mpp, vector curmp, int force_reload)
 {
@@ -728,9 +754,7 @@ select_action (struct multipath * mpp, vector curmp, int force_reload)
 	if (mpp->no_path_retry != NO_PATH_RETRY_UNDEF &&
 	    !!strstr(mpp->features, "queue_if_no_path") !=
 	    !!strstr(cmpp->features, "queue_if_no_path")) {
-		mpp->action =  ACT_RELOAD;
-		condlog(3, "%s: set ACT_RELOAD (no_path_retry change)",
-			mpp->alias);
+		select_reload_action(cmpp, "no_path_retry change");
 		return;
 	}
 	if ((mpp->retain_hwhandler != RETAIN_HWHANDLER_ON ||
@@ -738,9 +762,7 @@ select_action (struct multipath * mpp, vector curmp, int force_reload)
 	    (strlen(cmpp->hwhandler) != strlen(mpp->hwhandler) ||
 	     strncmp(cmpp->hwhandler, mpp->hwhandler,
 		    strlen(mpp->hwhandler)))) {
-		mpp->action = ACT_RELOAD;
-		condlog(3, "%s: set ACT_RELOAD (hwhandler change)",
-			mpp->alias);
+		select_reload_action(cmpp, "hwhandler change");
 		return;
 	}
 
@@ -748,9 +770,7 @@ select_action (struct multipath * mpp, vector curmp, int force_reload)
 	    !!strstr(mpp->features, "retain_attached_hw_handler") !=
 	    !!strstr(cmpp->features, "retain_attached_hw_handler") &&
 	    get_linux_version_code() < KERNEL_VERSION(4, 3, 0)) {
-		mpp->action = ACT_RELOAD;
-		condlog(3, "%s: set ACT_RELOAD (retain_hwhandler change)",
-			mpp->alias);
+		select_reload_action(cmpp, "retain_hwhandler change");
 		return;
 	}
 
@@ -762,9 +782,10 @@ select_action (struct multipath * mpp, vector curmp, int force_reload)
 		remove_feature(&cmpp_feat, "queue_if_no_path");
 		remove_feature(&cmpp_feat, "retain_attached_hw_handler");
 		if (strncmp(mpp_feat, cmpp_feat, PARAMS_SIZE)) {
-			mpp->action =  ACT_RELOAD;
-			condlog(3, "%s: set ACT_RELOAD (features change)",
-				mpp->alias);
+			select_reload_action(cmpp, "features change");
+			FREE(cmpp_feat);
+			FREE(mpp_feat);
+			return;
 		}
 	}
 	FREE(cmpp_feat);
@@ -772,27 +793,19 @@ select_action (struct multipath * mpp, vector curmp, int force_reload)
 
 	if (!cmpp->selector || strncmp(cmpp->selector, mpp->selector,
 		    strlen(mpp->selector))) {
-		mpp->action = ACT_RELOAD;
-		condlog(3, "%s: set ACT_RELOAD (selector change)",
-			mpp->alias);
+		select_reload_action(cmpp, "selector change");
 		return;
 	}
 	if (cmpp->minio != mpp->minio) {
-		mpp->action = ACT_RELOAD;
-		condlog(3, "%s: set ACT_RELOAD (minio change, %u->%u)",
-			mpp->alias, cmpp->minio, mpp->minio);
+		select_reload_action(cmpp, "minio change");
 		return;
 	}
 	if (!cmpp->pg || VECTOR_SIZE(cmpp->pg) != VECTOR_SIZE(mpp->pg)) {
-		mpp->action = ACT_RELOAD;
-		condlog(3, "%s: set ACT_RELOAD (path group number change)",
-			mpp->alias);
+		select_reload_action(cmpp, "path group number change");
 		return;
 	}
 	if (pgcmp(mpp, cmpp)) {
-		mpp->action = ACT_RELOAD;
-		condlog(3, "%s: set ACT_RELOAD (path group topology change)",
-			mpp->alias);
+		select_reload_action(cmpp, "path group topology change");
 		return;
 	}
 	if (cmpp->nextpg != mpp->bestpg) {
-- 
2.26.2

[PATCH 77/80] libmultipath: log dm_task_run() errors

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Log the ioctl error messages from libdm.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c | 22 ++++++++-------
 libmultipath/devmapper.c | 61 ++++++++++++++++++++++++++++++++--------
 libmultipath/devmapper.h |  4 +++
 multipathd/dmevents.c    |  4 ++-
 multipathd/waiter.c      |  2 ++
 5 files changed, 70 insertions(+), 23 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index efb5808..faead8c 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -661,7 +661,8 @@ sysfs_set_max_sectors_kb(struct multipath *mpp, int is_reload)
 }
 
 static void
-select_reload_action(struct multipath *mpp, const char *reason)
+select_reload_action(struct multipath *mpp, const struct multipath *cmpp,
+		     const char *reason)
 {
 	struct udev_device *mpp_ud;
 	const char *env;
@@ -675,8 +676,9 @@ select_reload_action(struct multipath *mpp, const char *reason)
 	 * run for this map. Thus force udev reload.
 	 */
 
-	mpp_ud = get_udev_for_mpp(mpp);
+	mpp_ud = get_udev_for_mpp(cmpp);
 	env = udev_device_get_property_value(mpp_ud, "MPATH_DEVICE_READY");
+	condlog(3, "%s: %s: \"%s\"\n", __func__, mpp->alias, env);
 	if (!env || strcmp(env, "1"))
 		mpp->force_udev_reload = 1;
 	udev_device_unref(mpp_ud);
@@ -754,7 +756,7 @@ select_action (struct multipath * mpp, vector curmp, int force_reload)
 	if (mpp->no_path_retry != NO_PATH_RETRY_UNDEF &&
 	    !!strstr(mpp->features, "queue_if_no_path") !=
 	    !!strstr(cmpp->features, "queue_if_no_path")) {
-		select_reload_action(cmpp, "no_path_retry change");
+		select_reload_action(mpp, cmpp, "no_path_retry change");
 		return;
 	}
 	if ((mpp->retain_hwhandler != RETAIN_HWHANDLER_ON ||
@@ -762,7 +764,7 @@ select_action (struct multipath * mpp, vector curmp, int force_reload)
 	    (strlen(cmpp->hwhandler) != strlen(mpp->hwhandler) ||
 	     strncmp(cmpp->hwhandler, mpp->hwhandler,
 		    strlen(mpp->hwhandler)))) {
-		select_reload_action(cmpp, "hwhandler change");
+		select_reload_action(mpp, cmpp, "hwhandler change");
 		return;
 	}
 
@@ -770,7 +772,7 @@ select_action (struct multipath * mpp, vector curmp, int force_reload)
 	    !!strstr(mpp->features, "retain_attached_hw_handler") !=
 	    !!strstr(cmpp->features, "retain_attached_hw_handler") &&
 	    get_linux_version_code() < KERNEL_VERSION(4, 3, 0)) {
-		select_reload_action(cmpp, "retain_hwhandler change");
+		select_reload_action(mpp, cmpp, "retain_hwhandler change");
 		return;
 	}
 
@@ -782,7 +784,7 @@ select_action (struct multipath * mpp, vector curmp, int force_reload)
 		remove_feature(&cmpp_feat, "queue_if_no_path");
 		remove_feature(&cmpp_feat, "retain_attached_hw_handler");
 		if (strncmp(mpp_feat, cmpp_feat, PARAMS_SIZE)) {
-			select_reload_action(cmpp, "features change");
+			select_reload_action(mpp, cmpp, "features change");
 			FREE(cmpp_feat);
 			FREE(mpp_feat);
 			return;
@@ -793,19 +795,19 @@ select_action (struct multipath * mpp, vector curmp, int force_reload)
 
 	if (!cmpp->selector || strncmp(cmpp->selector, mpp->selector,
 		    strlen(mpp->selector))) {
-		select_reload_action(cmpp, "selector change");
+		select_reload_action(mpp, cmpp, "selector change");
 		return;
 	}
 	if (cmpp->minio != mpp->minio) {
-		select_reload_action(cmpp, "minio change");
+		select_reload_action(mpp, cmpp, "minio change");
 		return;
 	}
 	if (!cmpp->pg || VECTOR_SIZE(cmpp->pg) != VECTOR_SIZE(mpp->pg)) {
-		select_reload_action(cmpp, "path group number change");
+		select_reload_action(mpp, cmpp, "path group number change");
 		return;
 	}
 	if (pgcmp(mpp, cmpp)) {
-		select_reload_action(cmpp, "path group topology change");
+		select_reload_action(mpp, cmpp, "path group topology change");
 		return;
 	}
 	if (cmpp->nextpg != mpp->bestpg) {
diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index fb7675c..46c7ec6 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -182,6 +182,7 @@ dm_tgt_version (unsigned int * version, char * str)
 	dm_task_no_open_count(dmt);
 
 	if (!dm_task_run(dmt)) {
+		dm_log_error(2, DM_DEVICE_LIST_VERSIONS, dmt);
 		condlog(0, "Can not communicate with kernel DM");
 		goto out;
 	}
@@ -320,6 +321,8 @@ dm_simplecmd (int task, const char *name, int no_flush, int need_sync, uint16_t
 		goto out;
 
 	r = dm_task_run (dmt);
+	if (!r)
+		dm_log_error(2, task, dmt);
 
 	if (udev_wait_flag)
 			dm_udev_wait(cookie);
@@ -410,6 +413,8 @@ dm_addmap (int task, const char *target, struct multipath *mpp,
 		goto freeout;
 
 	r = dm_task_run (dmt);
+	if (!r)
+		dm_log_error(2, task, dmt);
 
 	if (task == DM_DEVICE_CREATE)
 			dm_udev_wait(cookie);
@@ -521,8 +526,10 @@ do_get_info(const char *name, struct dm_info *info)
 
 	dm_task_no_open_count(dmt);
 
-	if (!dm_task_run(dmt))
+	if (!dm_task_run(dmt)) {
+		dm_log_error(3, DM_DEVICE_INFO, dmt);
 		goto out;
+	}
 
 	if (!dm_task_get_info(dmt, info))
 		goto out;
@@ -561,6 +568,7 @@ int dm_get_map(const char *name, unsigned long long *size, char *outparams)
 
 	errno = 0;
 	if (!dm_task_run(dmt)) {
+		dm_log_error(3, DM_DEVICE_TABLE, dmt);
 		if (dm_task_get_errno(dmt) == ENXIO)
 			r = DMP_NOT_FOUND;
 		goto out;
@@ -601,8 +609,10 @@ dm_get_prefixed_uuid(const char *name, char *uuid, int uuid_len)
 	if (!dm_task_set_name (dmt, name))
 		goto uuidout;
 
-	if (!dm_task_run(dmt))
+	if (!dm_task_run(dmt)) {
+		dm_log_error(3, DM_DEVICE_INFO, dmt);
 		goto uuidout;
+	}
 
 	uuidtmp = dm_task_get_uuid(dmt);
 	if (uuidtmp)
@@ -671,6 +681,7 @@ int dm_get_status(const char *name, char *outstatus)
 
 	errno = 0;
 	if (!dm_task_run(dmt)) {
+		dm_log_error(3, DM_DEVICE_STATUS, dmt);
 		if (dm_task_get_errno(dmt) == ENXIO)
 			r = DMP_NOT_FOUND;
 		goto out;
@@ -722,8 +733,10 @@ int dm_type(const char *name, char *type)
 
 	dm_task_no_open_count(dmt);
 
-	if (!dm_task_run(dmt))
+	if (!dm_task_run(dmt)) {
+		dm_log_error(3, DM_DEVICE_TABLE, dmt);
 		goto out;
+	}
 
 	/* Fetch 1st target */
 	if (dm_get_next_target(dmt, NULL, &start, &length,
@@ -764,8 +777,10 @@ int dm_is_mpath(const char *name)
 
 	dm_task_no_open_count(dmt);
 
-	if (!dm_task_run(dmt))
+	if (!dm_task_run(dmt)) {
+		dm_log_error(3, DM_DEVICE_TABLE, dmt);
 		goto out_task;
+	}
 
 	if (!dm_task_get_info(dmt, &info))
 		goto out_task;
@@ -826,8 +841,10 @@ dm_map_present_by_uuid(const char *uuid)
 	if (!dm_task_set_uuid(dmt, prefixed_uuid))
 		goto out_task;
 
-	if (!dm_task_run(dmt))
+	if (!dm_task_run(dmt)) {
+		dm_log_error(3, DM_DEVICE_INFO, dmt);
 		goto out_task;
+	}
 
 	if (!dm_task_get_info(dmt, &info))
 		goto out_task;
@@ -870,8 +887,10 @@ dm_get_opencount (const char * mapname)
 	if (!dm_task_set_name(dmt, mapname))
 		goto out;
 
-	if (!dm_task_run(dmt))
+	if (!dm_task_run(dmt)) {
+		dm_log_error(3, DM_DEVICE_INFO, dmt);
 		goto out;
+	}
 
 	if (!dm_task_get_info(dmt, &info))
 		goto out;
@@ -1028,8 +1047,10 @@ int dm_flush_maps (int need_suspend, int retries)
 
 	dm_task_no_open_count(dmt);
 
-	if (!dm_task_run (dmt))
+	if (!dm_task_run (dmt)) {
+		dm_log_error(3, DM_DEVICE_LIST, dmt);
 		goto out;
+	}
 
 	if (!(names = dm_task_get_names (dmt)))
 		goto out;
@@ -1072,8 +1093,10 @@ dm_message(const char * mapname, char * message)
 
 	dm_task_no_open_count(dmt);
 
-	if (!dm_task_run(dmt))
+	if (!dm_task_run(dmt)) {
+		dm_log_error(2, DM_DEVICE_TARGET_MSG, dmt);
 		goto out;
+	}
 
 	r = 0;
 out:
@@ -1190,8 +1213,10 @@ dm_get_maps (vector mp)
 
 	dm_task_no_open_count(dmt);
 
-	if (!dm_task_run(dmt))
+	if (!dm_task_run(dmt)) {
+		dm_log_error(3, DM_DEVICE_LIST, dmt);
 		goto out;
+	}
 
 	if (!(names = dm_task_get_names(dmt)))
 		goto out;
@@ -1280,6 +1305,7 @@ dm_mapname(int major, int minor)
 	}
 
 	if (!r) {
+		dm_log_error(2, DM_DEVICE_STATUS, dmt);
 		condlog(0, "%i:%i: timeout fetching map name", major, minor);
 		goto bad;
 	}
@@ -1315,8 +1341,10 @@ do_foreach_partmaps (const char * mapname,
 
 	dm_task_no_open_count(dmt);
 
-	if (!dm_task_run(dmt))
+	if (!dm_task_run(dmt)) {
+		dm_log_error(3, DM_DEVICE_LIST, dmt);
 		goto out;
+	}
 
 	if (!(names = dm_task_get_names(dmt)))
 		goto out;
@@ -1550,6 +1578,8 @@ dm_rename (const char * old, char * new, char *delim, int skip_kpartx)
 	if (!dm_task_set_cookie(dmt, &cookie, udev_flags))
 		goto out;
 	r = dm_task_run(dmt);
+	if (!r)
+		dm_log_error(2, DM_DEVICE_RENAME, dmt);
 
 	dm_udev_wait(cookie);
 
@@ -1593,8 +1623,10 @@ int dm_reassign_table(const char *name, char *old, char *new)
 
 	dm_task_no_open_count(dmt);
 
-	if (!dm_task_run(dmt))
+	if (!dm_task_run(dmt)) {
+		dm_log_error(3, DM_DEVICE_TABLE, dmt);
 		goto out;
+	}
 	if (!(reload_dmt = libmp_dm_task_create(DM_DEVICE_RELOAD)))
 		goto out;
 	if (!dm_task_set_name(reload_dmt, name))
@@ -1625,6 +1657,7 @@ int dm_reassign_table(const char *name, char *old, char *new)
 		dm_task_no_open_count(reload_dmt);
 
 		if (!dm_task_run(reload_dmt)) {
+			dm_log_error(3, DM_DEVICE_RELOAD, reload_dmt);
 			condlog(3, "%s: failed to reassign targets", name);
 			goto out_reload;
 		}
@@ -1670,8 +1703,10 @@ int dm_reassign(const char *mapname)
 
 	dm_task_no_open_count(dmt);
 
-	if (!dm_task_run(dmt))
+	if (!dm_task_run(dmt)) {
+		dm_log_error(3, DM_DEVICE_DEPS, dmt);
 		goto out;
+	}
 
 	if (!dm_task_get_info(dmt, &info))
 		goto out;
@@ -1737,6 +1772,8 @@ int dm_setgeometry(struct multipath *mpp)
 	}
 
 	r = dm_task_run(dmt);
+	if (!r)
+		dm_log_error(3, DM_DEVICE_SET_GEOMETRY, dmt);
 out:
 	dm_task_destroy(dmt);
 
diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
index 6dd178c..f568ab5 100644
--- a/libmultipath/devmapper.h
+++ b/libmultipath/devmapper.h
@@ -85,4 +85,8 @@ struct multipath *dm_get_multipath(const char *name);
 	((v[0] == minv[0]) && (v[1] == minv[1]) && (v[2] >= minv[2])) \
 )
 
+#define dm_log_error(lvl, cmd, dmt)			      \
+	condlog(lvl, "%s: libdm task=%d error: %s", __func__, \
+		cmd, strerror(dm_task_get_errno(dmt)))	      \
+
 #endif /* _DEVMAPPER_H */
diff --git a/multipathd/dmevents.c b/multipathd/dmevents.c
index b22b47d..b235dd4 100644
--- a/multipathd/dmevents.c
+++ b/multipathd/dmevents.c
@@ -156,8 +156,10 @@ static int dm_get_events(void)
 
 	dm_task_no_open_count(dmt);
 
-	if (!dm_task_run(dmt))
+	if (!dm_task_run(dmt)) {
+		dm_log_error(3, DM_DEVICE_LIST, dmt);
 		goto fail;
+	}
 
 	if (!(names = dm_task_get_names(dmt)))
 		goto fail;
diff --git a/multipathd/waiter.c b/multipathd/waiter.c
index e645766..16fbdc8 100644
--- a/multipathd/waiter.c
+++ b/multipathd/waiter.c
@@ -119,6 +119,8 @@ static int waiteventloop (struct event_thread *waiter)
 
 	pthread_testcancel();
 	r = dm_task_run(waiter->dmt);
+	if (!r)
+		dm_log_error(2, DM_DEVICE_WAITEVENT, waiter->dmt);
 	pthread_testcancel();
 
 	pthread_sigmask(SIG_SETMASK, &oldset, NULL);
-- 
2.26.2

[PATCH 78/80] libmultipath: move reload_map() to multipathd

[mwilck]

From: Martin Wilck <mwilck@suse.com>

reload_map() is only used by multipathd. We don't have less exported
symbols though, because select_action() now needs to be exported.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c | 40 +---------------------------------------
 libmultipath/configure.h |  2 +-
 multipathd/main.c        | 37 +++++++++++++++++++++++++++++++++++++
 3 files changed, 39 insertions(+), 40 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index faead8c..b77c2a8 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -688,8 +688,7 @@ select_reload_action(struct multipath *mpp, const struct multipath *cmpp,
 		reason);
 }
 
-static void
-select_action (struct multipath * mpp, vector curmp, int force_reload)
+void select_action (struct multipath *mpp, vector curmp, int force_reload)
 {
 	struct multipath * cmpp;
 	struct multipath * cmpp_by_name;
@@ -1478,40 +1477,3 @@ int get_refwwid(enum mpath_cmds cmd, const char *dev, enum devtypes dev_type,
 	pthread_cleanup_pop(1);
 	return ret;
 }
-
-int reload_map(struct vectors *vecs, struct multipath *mpp, int refresh,
-	       int is_daemon)
-{
-	char params[PARAMS_SIZE] = {0};
-	struct path *pp;
-	int i, r;
-
-	update_mpp_paths(mpp, vecs->pathvec);
-	if (refresh) {
-		vector_foreach_slot (mpp->paths, pp, i) {
-			struct config *conf = get_multipath_config();
-			pthread_cleanup_push(put_multipath_config, conf);
-			r = pathinfo(pp, conf, DI_PRIO);
-			pthread_cleanup_pop(1);
-			if (r) {
-				condlog(2, "%s: failed to refresh pathinfo",
-					mpp->alias);
-				return 1;
-			}
-		}
-	}
-	if (setup_map(mpp, params, PARAMS_SIZE, vecs)) {
-		condlog(0, "%s: failed to setup map", mpp->alias);
-		return 1;
-	}
-	select_action(mpp, vecs->mpvec, 1);
-
-	r = domap(mpp, params, is_daemon);
-	if (r == DOMAP_FAIL || r == DOMAP_RETRY) {
-		condlog(3, "%s: domap (%u) failure "
-			"for reload map", mpp->alias, r);
-		return 1;
-	}
-
-	return 0;
-}
diff --git a/libmultipath/configure.h b/libmultipath/configure.h
index d9a7de6..9907775 100644
--- a/libmultipath/configure.h
+++ b/libmultipath/configure.h
@@ -49,11 +49,11 @@ struct vectors;
 
 int setup_map (struct multipath * mpp, char * params, int params_size,
 	       struct vectors *vecs );
+void select_action (struct multipath *mpp, vector curmp, int force_reload);
 int domap (struct multipath * mpp, char * params, int is_daemon);
 int reinstate_paths (struct multipath *mpp);
 int coalesce_paths (struct vectors *vecs, vector curmp, char * refwwid, int force_reload, enum mpath_cmds cmd);
 int get_refwwid (enum mpath_cmds cmd, const char *dev, enum devtypes dev_type,
 		 vector pathvec, char **wwid);
-int reload_map(struct vectors *vecs, struct multipath *mpp, int refresh, int is_daemon);
 struct udev_device *get_udev_device(const char *dev, enum devtypes dev_type);
 void trigger_paths_udev_change(struct multipath *mpp, bool is_mpath);
diff --git a/multipathd/main.c b/multipathd/main.c
index 29227cd..aa08807 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1909,6 +1909,43 @@ int update_prio(struct path *pp, int refresh_all)
 	return 1;
 }
 
+static int reload_map(struct vectors *vecs, struct multipath *mpp, int refresh,
+		      int is_daemon)
+{
+	char params[PARAMS_SIZE] = {0};
+	struct path *pp;
+	int i, r;
+
+	update_mpp_paths(mpp, vecs->pathvec);
+	if (refresh) {
+		vector_foreach_slot (mpp->paths, pp, i) {
+			struct config *conf = get_multipath_config();
+			pthread_cleanup_push(put_multipath_config, conf);
+			r = pathinfo(pp, conf, DI_PRIO);
+			pthread_cleanup_pop(1);
+			if (r) {
+				condlog(2, "%s: failed to refresh pathinfo",
+					mpp->alias);
+				return 1;
+			}
+		}
+	}
+	if (setup_map(mpp, params, PARAMS_SIZE, vecs)) {
+		condlog(0, "%s: failed to setup map", mpp->alias);
+		return 1;
+	}
+	select_action(mpp, vecs->mpvec, 1);
+
+	r = domap(mpp, params, is_daemon);
+	if (r == DOMAP_FAIL || r == DOMAP_RETRY) {
+		condlog(3, "%s: domap (%u) failure "
+			"for reload map", mpp->alias, r);
+		return 1;
+	}
+
+	return 0;
+}
+
 int update_path_groups(struct multipath *mpp, struct vectors *vecs, int refresh)
 {
 	if (reload_map(vecs, mpp, refresh, 1))
-- 
2.26.2

[PATCH 79/80] multipathd: rename update_path_groups() -> reload_and_sync_map()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This function doesn't just update the path groups. It completely
rebuilds the multipath, reloads the kernel map, and syncs path
states. That should be reflected in the function name, which should
use the term "map", like all other functions that modify kernel state.

Todo: there's a large functional overlap between this function
and update_map(). Perhaps we should unify the two.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/cli_handlers.c | 8 ++++----
 multipathd/main.c         | 9 +++++----
 multipathd/main.h         | 4 ++--
 3 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 679fd57..d76fe90 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -941,7 +941,7 @@ cli_reload(void *v, char **reply, int *len, void *data)
 		return 1;
 	}
 
-	return update_path_groups(mpp, vecs, 0);
+	return reload_and_sync_map(mpp, vecs, 0);
 }
 
 int resize_map(struct multipath *mpp, unsigned long long size,
@@ -1621,7 +1621,7 @@ int cli_set_marginal(void * v, char ** reply, int * len, void * data)
 	}
 	pp->marginal = 1;
 
-	return update_path_groups(pp->mpp, vecs, 0);
+	return reload_and_sync_map(pp->mpp, vecs, 0);
 }
 
 int cli_unset_marginal(void * v, char ** reply, int * len, void * data)
@@ -1648,7 +1648,7 @@ int cli_unset_marginal(void * v, char ** reply, int * len, void * data)
 	}
 	pp->marginal = 0;
 
-	return update_path_groups(pp->mpp, vecs, 0);
+	return reload_and_sync_map(pp->mpp, vecs, 0);
 }
 
 int cli_unset_all_marginal(void * v, char ** reply, int * len, void * data)
@@ -1685,5 +1685,5 @@ int cli_unset_all_marginal(void * v, char ** reply, int * len, void * data)
 		vector_foreach_slot (pgp->paths, pp, j)
 			pp->marginal = 0;
 
-	return update_path_groups(mpp, vecs, 0);
+	return reload_and_sync_map(mpp, vecs, 0);
 }
diff --git a/multipathd/main.c b/multipathd/main.c
index aa08807..e6d4652 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1327,7 +1327,7 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
 			else {
 				if (ro == 1)
 					pp->mpp->force_readonly = 1;
-				retval = update_path_groups(mpp, vecs, 0);
+				retval = reload_and_sync_map(mpp, vecs, 0);
 				if (retval == 2)
 					condlog(2, "%s: map removed during reload", pp->dev);
 				else {
@@ -1946,7 +1946,8 @@ static int reload_map(struct vectors *vecs, struct multipath *mpp, int refresh,
 	return 0;
 }
 
-int update_path_groups(struct multipath *mpp, struct vectors *vecs, int refresh)
+int reload_and_sync_map(struct multipath *mpp,
+			struct vectors *vecs, int refresh)
 {
 	if (reload_map(vecs, mpp, refresh, 1))
 		return 1;
@@ -2379,11 +2380,11 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 	condlog(4, "path prio refresh");
 
 	if (marginal_changed)
-		update_path_groups(pp->mpp, vecs, 1);
+		reload_and_sync_map(pp->mpp, vecs, 1);
 	else if (update_prio(pp, new_path_up) &&
 	    (pp->mpp->pgpolicyfn == (pgpolicyfn *)group_by_prio) &&
 	     pp->mpp->pgfailback == -FAILBACK_IMMEDIATE)
-		update_path_groups(pp->mpp, vecs, !new_path_up);
+		reload_and_sync_map(pp->mpp, vecs, !new_path_up);
 	else if (need_switch_pathgroup(pp->mpp, 0)) {
 		if (pp->mpp->pgfailback > 0 &&
 		    (new_path_up || pp->mpp->failback_tick <= 0))
diff --git a/multipathd/main.h b/multipathd/main.h
index 5dff17e..9aa2a0f 100644
--- a/multipathd/main.h
+++ b/multipathd/main.h
@@ -46,7 +46,7 @@ int __setup_multipath (struct vectors * vecs, struct multipath * mpp,
 		       int reset);
 #define setup_multipath(vecs, mpp) __setup_multipath(vecs, mpp, 1)
 int update_multipath (struct vectors *vecs, char *mapname, int reset);
-int update_path_groups(struct multipath *mpp, struct vectors *vecs,
-		       int refresh);
+int reload_and_sync_map(struct multipath *mpp, struct vectors *vecs,
+			int refresh);
 
 #endif /* MAIN_H */
-- 
2.26.2

[PATCH 80/80] libmultipath: select_action(): don't drop map if alias clashes

[mwilck]

From: Martin Wilck <mwilck@suse.com>

If for a given map, if we find that the requested alias is already
used by a map with different WWID, while the map's own WWID is
not used yet, give up the alias and use the WWID instead. This
is safer than trying to destroy the existing map, which is likely
to fail.

This allows us to make use const for the "curmp" parameter.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c | 13 +++++++------
 libmultipath/configure.h |  3 ++-
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index b77c2a8..7931045 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -688,7 +688,8 @@ select_reload_action(struct multipath *mpp, const struct multipath *cmpp,
 		reason);
 }
 
-void select_action (struct multipath *mpp, vector curmp, int force_reload)
+void select_action (struct multipath *mpp, const struct _vector *curmp,
+		    int force_reload)
 {
 	struct multipath * cmpp;
 	struct multipath * cmpp_by_name;
@@ -716,12 +717,12 @@ void select_action (struct multipath *mpp, vector curmp, int force_reload)
 	}
 
 	if (!cmpp) {
-		condlog(2, "%s: remove (wwid changed)", mpp->alias);
-		dm_flush_map(mpp->alias);
-		strlcpy(cmpp_by_name->wwid, mpp->wwid, WWID_SIZE);
-		drop_multipath(curmp, cmpp_by_name->wwid, KEEP_PATHS);
+		condlog(1, "%s: can't use alias \"%s\" used by %s, falling back to WWID",
+			mpp->wwid, mpp->alias, cmpp_by_name->wwid);
+		/* We can do this because wwid wasn't found */
+		strlcpy(mpp->alias, mpp->wwid, sizeof(mpp->alias));
 		mpp->action = ACT_CREATE;
-		condlog(3, "%s: set ACT_CREATE (map wwid change)",
+		condlog(3, "%s: set ACT_CREATE (map does not exist, name changed)",
 			mpp->alias);
 		return;
 	}
diff --git a/libmultipath/configure.h b/libmultipath/configure.h
index 9907775..6b23ccb 100644
--- a/libmultipath/configure.h
+++ b/libmultipath/configure.h
@@ -49,7 +49,8 @@ struct vectors;
 
 int setup_map (struct multipath * mpp, char * params, int params_size,
 	       struct vectors *vecs );
-void select_action (struct multipath *mpp, vector curmp, int force_reload);
+void select_action (struct multipath *mpp, const struct _vector *curmp,
+		    int force_reload);
 int domap (struct multipath * mpp, char * params, int is_daemon);
 int reinstate_paths (struct multipath *mpp);
 int coalesce_paths (struct vectors *vecs, vector curmp, char * refwwid, int force_reload, enum mpath_cmds cmd);
-- 
2.26.2

Re: [PATCH 76/80] libmultipath: select_action(): force udev reload for uninitialized maps

[Benjamin Marzinski]

On Thu, Jul 09, 2020 at 01:03:26PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> If we are in the reconfigure() code path, and we encounter maps to
> be reloaded, we usually set the DM_SUBSYSTEM_UDEV_FLAG0 flag to tell
> udev not to repeat device detection steps above the multipath layer.
> However, if the map was previously uninitialized, we have to force
> udev to reload.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/configure.c | 61 ++++++++++++++++++++++++----------------
>  1 file changed, 37 insertions(+), 24 deletions(-)
> 
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index 2509053..efb5808 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -660,6 +660,32 @@ sysfs_set_max_sectors_kb(struct multipath *mpp, int is_reload)
>  	return err;
>  }
>  
> +static void
> +select_reload_action(struct multipath *mpp, const char *reason)
> +{
> +	struct udev_device *mpp_ud;
> +	const char *env;
> +
> +	/*
> +	 * MPATH_DEVICE_READY != 1 can mean two things:
> +	 *  (a) no usable paths
> +	 *  (b) device was never fully processed (e.g. udev killed)
> +	 * If we are in this code path (startup or forced reconfigure),
> +	 * (b) can mean that upper layers like kpartx have never been
> +	 * run for this map. Thus force udev reload.
> +	 */
> +

This looks like it wants multipath to check if there are valid devices
before setting force reload.  But looking at the udev rules, I'm pretty
sure it's safe. If we have no valid paths, then we will have

ENV{DM_NOSCAN}="1 and ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="1"

which means it doesn't matter that force_udev_reload will cause

ENV{DM_ACTIVATION}="1" and ENV{MPATH_UNCHANGED}=""

It does get sort of confusing with the number of udev properties that can
effect things.  So nothing really needs to get done for this to be
correct, but perhaps this code should only set force reload is there are
valid paths, just to be clear.

-Ben

> +	mpp_ud = get_udev_for_mpp(mpp);
> +	env = udev_device_get_property_value(mpp_ud, "MPATH_DEVICE_READY");
> +	if (!env || strcmp(env, "1"))
> +		mpp->force_udev_reload = 1;
> +	udev_device_unref(mpp_ud);
> +	mpp->action = ACT_RELOAD;
> +	condlog(3, "%s: set ACT_RELOAD (%s%s)", mpp->alias,
> +		mpp->force_udev_reload ? "forced, " : "",
> +		reason);
> +}
> +
>  static void
>  select_action (struct multipath * mpp, vector curmp, int force_reload)
>  {
> @@ -728,9 +754,7 @@ select_action (struct multipath * mpp, vector curmp, int force_reload)
>  	if (mpp->no_path_retry != NO_PATH_RETRY_UNDEF &&
>  	    !!strstr(mpp->features, "queue_if_no_path") !=
>  	    !!strstr(cmpp->features, "queue_if_no_path")) {
> -		mpp->action =  ACT_RELOAD;
> -		condlog(3, "%s: set ACT_RELOAD (no_path_retry change)",
> -			mpp->alias);
> +		select_reload_action(cmpp, "no_path_retry change");
>  		return;
>  	}
>  	if ((mpp->retain_hwhandler != RETAIN_HWHANDLER_ON ||
> @@ -738,9 +762,7 @@ select_action (struct multipath * mpp, vector curmp, int force_reload)
>  	    (strlen(cmpp->hwhandler) != strlen(mpp->hwhandler) ||
>  	     strncmp(cmpp->hwhandler, mpp->hwhandler,
>  		    strlen(mpp->hwhandler)))) {
> -		mpp->action = ACT_RELOAD;
> -		condlog(3, "%s: set ACT_RELOAD (hwhandler change)",
> -			mpp->alias);
> +		select_reload_action(cmpp, "hwhandler change");
>  		return;
>  	}
>  
> @@ -748,9 +770,7 @@ select_action (struct multipath * mpp, vector curmp, int force_reload)
>  	    !!strstr(mpp->features, "retain_attached_hw_handler") !=
>  	    !!strstr(cmpp->features, "retain_attached_hw_handler") &&
>  	    get_linux_version_code() < KERNEL_VERSION(4, 3, 0)) {
> -		mpp->action = ACT_RELOAD;
> -		condlog(3, "%s: set ACT_RELOAD (retain_hwhandler change)",
> -			mpp->alias);
> +		select_reload_action(cmpp, "retain_hwhandler change");
>  		return;
>  	}
>  
> @@ -762,9 +782,10 @@ select_action (struct multipath * mpp, vector curmp, int force_reload)
>  		remove_feature(&cmpp_feat, "queue_if_no_path");
>  		remove_feature(&cmpp_feat, "retain_attached_hw_handler");
>  		if (strncmp(mpp_feat, cmpp_feat, PARAMS_SIZE)) {
> -			mpp->action =  ACT_RELOAD;
> -			condlog(3, "%s: set ACT_RELOAD (features change)",
> -				mpp->alias);
> +			select_reload_action(cmpp, "features change");
> +			FREE(cmpp_feat);
> +			FREE(mpp_feat);
> +			return;
>  		}
>  	}
>  	FREE(cmpp_feat);
> @@ -772,27 +793,19 @@ select_action (struct multipath * mpp, vector curmp, int force_reload)
>  
>  	if (!cmpp->selector || strncmp(cmpp->selector, mpp->selector,
>  		    strlen(mpp->selector))) {
> -		mpp->action = ACT_RELOAD;
> -		condlog(3, "%s: set ACT_RELOAD (selector change)",
> -			mpp->alias);
> +		select_reload_action(cmpp, "selector change");
>  		return;
>  	}
>  	if (cmpp->minio != mpp->minio) {
> -		mpp->action = ACT_RELOAD;
> -		condlog(3, "%s: set ACT_RELOAD (minio change, %u->%u)",
> -			mpp->alias, cmpp->minio, mpp->minio);
> +		select_reload_action(cmpp, "minio change");
>  		return;
>  	}
>  	if (!cmpp->pg || VECTOR_SIZE(cmpp->pg) != VECTOR_SIZE(mpp->pg)) {
> -		mpp->action = ACT_RELOAD;
> -		condlog(3, "%s: set ACT_RELOAD (path group number change)",
> -			mpp->alias);
> +		select_reload_action(cmpp, "path group number change");
>  		return;
>  	}
>  	if (pgcmp(mpp, cmpp)) {
> -		mpp->action = ACT_RELOAD;
> -		condlog(3, "%s: set ACT_RELOAD (path group topology change)",
> -			mpp->alias);
> +		select_reload_action(cmpp, "path group topology change");
>  		return;
>  	}
>  	if (cmpp->nextpg != mpp->bestpg) {
> -- 
> 2.26.2

Re: [PATCH 00/80] multipath-tools series part VI: incomplete udev initialization

[Benjamin Marzinski]

On Thu, Jul 09, 2020 at 01:03:24PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Hi Christophe, hi Ben,
> 
> This is part VI of a larger patch series for multipath-tools I've been preparing.
> It's based on the previously submitted part V.
> 
> The full series will also be available here:
> https://github.com/mwilck/multipath-tools/tree/ups/submit-2007
> 
> There are tags in that repo for each part of the series.
> This part is tagged "submit-200709-6".

for the part
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
 
> The series handles an issue observed in certain partner installations, where
> DM devices were incompletely initialized by udev - during initrd procesing,
> the "add" event had been processed, but the subsequent "change" event had not,
> because udevd had been killed before getting around to handle them.
> 
> My first attempt to fix this was based on udev rules ("11-dm-mpath.rules: Fix
> udev rule processing during coldplug"), but this patch was wrong. We have to
> add logic in multipathd itself. The most important patch in the series that
> fixed the actual customer problem is patch 76. Patch 75 was supposed to handle
> a slightly different incarnation of the same problem, which so far hasn't been
> actually observed. But I think having this patch "just in case" doesn't hurt,
> either.
> 
> Patch 78 and 79 rename "update_path_groups()", which over time has grown
> to be the main entry point for reloading maps.
> 
> Patch 80 fixes an issue which I observed while testing the first 3 patches.
> 
> Regards,
> Martin
> 
> Martin Wilck (6):
>   multipathd: uev_trigger(): handle incomplete ADD events
>   libmultipath: select_action(): force udev reload for uninitialized
>     maps
>   libmultipath: log dm_task_run() errors
>   libmultipath: move reload_map() to multipathd
>   multipathd: rename update_path_groups() -> reload_and_sync_map()
>   libmultipath: select_action(): don't drop map if alias clashes
> 
>  libmultipath/configure.c  | 112 +++++++++++++++-----------------------
>  libmultipath/configure.h  |   3 +-
>  libmultipath/devmapper.c  |  61 +++++++++++++++++----
>  libmultipath/devmapper.h  |   4 ++
>  multipathd/cli_handlers.c |   8 +--
>  multipathd/dmevents.c     |   4 +-
>  multipathd/main.c         |  71 ++++++++++++++++++++++--
>  multipathd/main.h         |   4 +-
>  multipathd/waiter.c       |   2 +
>  9 files changed, 178 insertions(+), 91 deletions(-)
> 
> -- 
> 2.26.2

Re: [PATCH 76/80] libmultipath: select_action(): force udev reload for uninitialized maps

[Martin Wilck]

On Sun, 2020-07-19 at 22:44 -0500, Benjamin Marzinski wrote:
> On Thu, Jul 09, 2020 at 01:03:26PM +0200, mwilck@suse.com wrote:
> > From: Martin Wilck <mwilck@suse.com>
> > 
> > If we are in the reconfigure() code path, and we encounter maps to
> > be reloaded, we usually set the DM_SUBSYSTEM_UDEV_FLAG0 flag to
> > tell
> > udev not to repeat device detection steps above the multipath
> > layer.
> > However, if the map was previously uninitialized, we have to force
> > udev to reload.
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  libmultipath/configure.c | 61 ++++++++++++++++++++++++----------
> > ------
> >  1 file changed, 37 insertions(+), 24 deletions(-)
> > 
> > diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> > index 2509053..efb5808 100644
> > --- a/libmultipath/configure.c
> > +++ b/libmultipath/configure.c
> > @@ -660,6 +660,32 @@ sysfs_set_max_sectors_kb(struct multipath
> > *mpp, int is_reload)
> >  	return err;
> >  }
> >  
> > +static void
> > +select_reload_action(struct multipath *mpp, const char *reason)
> > +{
> > +	struct udev_device *mpp_ud;
> > +	const char *env;
> > +
> > +	/*
> > +	 * MPATH_DEVICE_READY != 1 can mean two things:
> > +	 *  (a) no usable paths
> > +	 *  (b) device was never fully processed (e.g. udev killed)
> > +	 * If we are in this code path (startup or forced reconfigure),
> > +	 * (b) can mean that upper layers like kpartx have never been
> > +	 * run for this map. Thus force udev reload.
> > +	 */
> > +
> 
> This looks like it wants multipath to check if there are valid
> devices
> before setting force reload.  But looking at the udev rules, I'm
> pretty
> sure it's safe. If we have no valid paths, then we will have
> 
> ENV{DM_NOSCAN}="1 and ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="1"
> 
> which means it doesn't matter that force_udev_reload will cause
> 
> ENV{DM_ACTIVATION}="1" and ENV{MPATH_UNCHANGED}=""
> 
> It does get sort of confusing with the number of udev properties that
> can
> effect things.  So nothing really needs to get done for this to be
> correct, but perhaps this code should only set force reload is there
> are
> valid paths, just to be clear.

Will do. Full ack wrt the confusing udev rules (patches welcome :D)

Note btw that multipathd triggers a synthetic change event in
reconfigure if no changes were applied; but that code is hardly ever
executed because we normally set queue_if_no_path, and during startup
multipathd will never encounter a queueing map.

Martin