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

[PATCH v2 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-2008
This part is tagged "submit-200812-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.
Logically, it rather belongs to part III.

Changes v1 -> v2, as suggested by Ben Marzinski:

 * 76/80 "libmultipath: select_action(): force udev reload for uninitialized
   maps"
    - fetch udevice from cmpp, not mpp (was fixed in "libmultipath: log
      dm_task_run() errors" in v1)
    - force reload only if active paths are available
 * 77/80 "libmultipath: log dm_task_run() errors"
    - moved cmpp fix to previous patch, where it belongs
    - remove pointless debug msg
 * 79/80 "multipathd: rename update_path_groups() -> reload_and_sync_map()"
    - context changes
 * 80/80 "libmultipath: select_action(): don't drop map if alias clashes"
    - fixed wrong strlcpy() (mpp->alias is a pointer)

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.28.0

[PATCH v2 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.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
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 cc54818..ac57b88 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(cmpp);
+	env = udev_device_get_property_value(mpp_ud, "MPATH_DEVICE_READY");
+	if ((!env || strcmp(env, "1")) && count_active_paths(mpp) > 0)
+		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.28.0

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

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Log the ioctl error messages from libdm.

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

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index ac57b88..a492e0a 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;
@@ -754,7 +755,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 +763,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 +771,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 +783,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 +794,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 38d1990..cc2de1d 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);
@@ -522,8 +527,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;
@@ -562,6 +569,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;
@@ -602,8 +610,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)
@@ -672,6 +682,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;
@@ -723,8 +734,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,
@@ -765,8 +778,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;
@@ -827,8 +842,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;
@@ -871,8 +888,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;
@@ -1029,8 +1048,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;
@@ -1073,8 +1094,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:
@@ -1191,8 +1214,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;
@@ -1283,6 +1308,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;
 	}
@@ -1318,8 +1344,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;
@@ -1552,6 +1580,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);
 
@@ -1595,8 +1625,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))
@@ -1627,6 +1659,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;
 		}
@@ -1672,8 +1705,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;
@@ -1739,6 +1774,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.28.0

[PATCH v2 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.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
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 c60182f..8db3796 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -946,7 +946,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,
@@ -1626,7 +1626,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)
@@ -1653,7 +1653,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)
@@ -1690,5 +1690,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 720495d..7f95d46 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;
@@ -2377,14 +2378,14 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 	if (marginal_changed) {
 		condlog(2, "%s: path is %s marginal", pp->dev,
 			(pp->marginal)? "now" : "no longer");
-		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) {
 		condlog(2, "%s: path priorities changed. reloading",
 			pp->mpp->alias);
-		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.28.0

[PATCH v2 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.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c | 14 ++++++++------
 libmultipath/configure.h |  3 ++-
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index b9cb26a..5fb5767 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -687,7 +687,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;
@@ -715,12 +716,13 @@ 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 */
+		free(mpp->alias);
+		mpp->alias = strdup(mpp->wwid);
 		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.28.0

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

[Benjamin Marzinski]

On Wed, Aug 12, 2020 at 01:35:40PM +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.

Actually, this patch looks all broken now. select_reload_action()
doesn't have a cmpp argument, but still has

mpp_ud = get_udev_for_mpp(cmpp);

Also, it's setting the action on cmpp from select_action, not mpp. I'm
pretty sure that the next patch makes everything work o.k. again.

-Ben

> 
> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> 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 cc54818..ac57b88 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(cmpp);
> +	env = udev_device_get_property_value(mpp_ud, "MPATH_DEVICE_READY");
> +	if ((!env || strcmp(env, "1")) && count_active_paths(mpp) > 0)
> +		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.28.0

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

[Benjamin Marzinski]

On Wed, Aug 12, 2020 at 01:35:41PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Log the ioctl error messages from libdm.
> 

I assume that the configure.c code from this patch belongs in the last
one.

-Ben

> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/configure.c | 19 +++++++------
>  libmultipath/devmapper.c | 61 ++++++++++++++++++++++++++++++++--------
>  libmultipath/devmapper.h |  4 +++
>  multipathd/dmevents.c    |  4 ++-
>  multipathd/waiter.c      |  2 ++
>  5 files changed, 68 insertions(+), 22 deletions(-)
> 
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index ac57b88..a492e0a 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;
> @@ -754,7 +755,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 +763,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 +771,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 +783,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 +794,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 38d1990..cc2de1d 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);
> @@ -522,8 +527,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;
> @@ -562,6 +569,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;
> @@ -602,8 +610,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)
> @@ -672,6 +682,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;
> @@ -723,8 +734,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,
> @@ -765,8 +778,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;
> @@ -827,8 +842,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;
> @@ -871,8 +888,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;
> @@ -1029,8 +1048,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;
> @@ -1073,8 +1094,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:
> @@ -1191,8 +1214,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;
> @@ -1283,6 +1308,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;
>  	}
> @@ -1318,8 +1344,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;
> @@ -1552,6 +1580,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);
>  
> @@ -1595,8 +1625,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))
> @@ -1627,6 +1659,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;
>  		}
> @@ -1672,8 +1705,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;
> @@ -1739,6 +1774,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.28.0

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

[Martin Wilck]

On Mon, 2020-08-17 at 16:31 -0500, Benjamin Marzinski wrote:
> On Wed, Aug 12, 2020 at 01:35:40PM +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.
> 
> Actually, this patch looks all broken now. select_reload_action()
> doesn't have a cmpp argument, but still has
> 
> mpp_ud = get_udev_for_mpp(cmpp);
> 
> Also, it's setting the action on cmpp from select_action, not mpp.
> I'm
> pretty sure that the next patch makes everything work o.k. again.

Right. Thank you for reviewing and spotting it, and apologies for the
mistake.

Martin