[dm-devel] [PATCH v2 0/9] multipathd: remove udev settle dependency

[dm-devel] [PATCH v2 0/9] multipathd: remove udev settle dependency

[Benjamin Marzinski]

So, it turns out that commit 4ef67017 "libmultipath: add
update_pathvec_from_dm()" already does most of the hard work of making
multipath handle the uninitialized paths that exist during boot, after
the switch root, but before the all the coldplug uevents have been
processed. The only problem is that this code leaves the paths in a
partially initialized state, which doesn't completely get corrected
until a reconfigure happens.

This patchset makes multipath track these partially initailized paths,
and makes sure that they get fully initialized, or removed, as
necessary.

The first patch is a tangentially related bug I found when trying
(unsuccessfully) to recreate multipathd's problem with dropping
uninitialized paths. Multipathd was not removing completely missing
paths from maps that it found while starting up. The solution I chose
was just to make sure that a full reload will happen on these maps, even
if a weak reconfigure was requested. However, this means that multipath
still completely ignores these missing paths. A side effect of this is
that "multipath -l" does not show these paths, even though they exist as
part of the multipath table. The full reloads are necessary, to take
care of issues that update_pathvec_from_dm() can run into, but we might
also want to think about marking these missing paths as INIT_REMOVED,
instead of not adding them at all. This would make "multipath -l" still
show them, until they actually get removed from the table.

Patch 0005 makes sure to fully initialize the paths when their coldplug
event occurs, but if the path is already fully initialized in udev, but
multipathd finds out about it from update_pathvec_from_dm(), multipath
doesn't do anything to finish initializing the path and moving it to
INIT_OK, besides waiting for a uevent or a reconfigure. Patch 0006
handles this by triggering a uevent if the path has been in partial
for more than 3 minutes.

I've tested these patches both by rebooting with necessary and
unnecessary multipath devices in the initramfs and multipathd.service
set to make multipathd start up at various points after the switch root,
and by manually sending remove uevents to unintialize some paths, and
then starting multipathd to test specific combinations of path states.
But more testing is always welcome.

Notes on v2:

After playing around with the initialization states a bit, I decided
that cleaning them up is a bigger task than I want to do in this
patchset. This set just concentrates on cleaning up our handling of
paths that get added in update_pathvec_from_dm() to allow us to
drop the systemd-udev-settle dependency.

Changes from v1, base on suggestions by Martin Wilck.

0005: Made cli_add_path() verify the wwid can be gotten, and hasn't
changed, before attempting to finish initalizing the path. Also don't
intialize a path that relies on udev if the udev device isn't
initalized.

0006: New patch. If a patch is in INIT_PARTIAL for over 3 minutes,
trigger a uevent on it.  This is kind of a long waiting period, but I
want to avoid firing off another uevent in cases where the problem is
that udev is in the middle of a uevent storm, and the expected event
is delayed.

0009: Repost of Martin's new init state wildcard patch

Benjamin Marzinski (8):
  multipathd: remove missing paths on startup
  libmultipath: skip unneeded steps to get path name
  libmultipath: don't use fallback wwid in update_pathvec_from_dm
  libmultipath: always set INIT_REMOVED in set_path_removed
  multipathd: fully initialize paths added by update_pathvec_from_dm
  multipathd: retrigger uevent for partial paths
  multipathd: remove INIT_PARTIAL paths that aren't in a multipath
    device
  multipathd: Remove dependency on systemd-udev-settle.service

Martin Wilck (1):
  libmultipath: add path wildcard "%I" for init state

 libmultipath/configure.c          |  2 +
 libmultipath/devmapper.c          |  2 +
 libmultipath/discovery.c          |  7 +--
 libmultipath/discovery.h          |  2 +
 libmultipath/libmultipath.version |  2 +-
 libmultipath/print.c              | 21 +++++++++
 libmultipath/structs.h            |  9 ++++
 libmultipath/structs_vec.c        | 41 ++++++++++--------
 multipathd/cli_handlers.c         | 35 ++++++++++++++-
 multipathd/main.c                 | 71 +++++++++++++++++++++++++++++--
 multipathd/main.h                 |  1 +
 multipathd/multipathd.service     |  3 +-
 12 files changed, 167 insertions(+), 29 deletions(-)

-- 
2.17.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 1/9] multipathd: remove missing paths on startup

[Benjamin Marzinski]

If a path device was removed from the system while multipathd was not
running, multipathd would not remove the path from the multipath table
on start-up, or on a weak reconfigure. update_pathvec_from_dm() would
return that a reload was necessary, but that information wasn't
propigated back to where it could be used to reload the device.

Multipath devices now remember if they need to be reloaded, and if so,
force_reload is set in select_action().  This means that even when
configure is called with FORCE_RELOAD_WEAK, these devices will still be
reloaded.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c   | 2 ++
 libmultipath/devmapper.c   | 2 ++
 libmultipath/structs.h     | 1 +
 libmultipath/structs_vec.c | 1 +
 4 files changed, 6 insertions(+)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index eb8ec1bd..f1a890af 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -715,6 +715,8 @@ void select_action (struct multipath *mpp, const struct _vector *curmp,
 
 	cmpp = find_mp_by_wwid(curmp, mpp->wwid);
 	cmpp_by_name = find_mp_by_alias(curmp, mpp->alias);
+	if (mpp->need_reload || (cmpp && cmpp->need_reload))
+		force_reload = 1;
 
 	if (!cmpp_by_name) {
 		if (cmpp) {
diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index c05dc201..3e1a7260 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -522,6 +522,8 @@ freeout:
 addout:
 	dm_task_destroy (dmt);
 
+	if (r)
+		mpp->need_reload = false;
 	return r;
 }
 
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 399540e7..d0b266b7 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -355,6 +355,7 @@ struct multipath {
 	int retain_hwhandler;
 	int deferred_remove;
 	bool in_recovery;
+	bool need_reload;
 	int san_path_err_threshold;
 	int san_path_err_forget_rate;
 	int san_path_err_recovery_time;
diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 85d97ac1..e52db0c4 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -237,6 +237,7 @@ bool update_pathvec_from_dm(vector pathvec, struct multipath *mpp,
 		free_pathgroup(pgp, KEEP_PATHS);
 		must_reload = true;
 	}
+	mpp->need_reload = mpp->need_reload || must_reload;
 	return must_reload;
 }
 
-- 
2.17.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 2/9] libmultipath: skip unneeded steps to get path name

[Benjamin Marzinski]

The path already must have a udev device at this point, so it just
needs to copy the sysname from it.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/structs_vec.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index e52db0c4..4d56107a 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -1,6 +1,7 @@
 #include <stdio.h>
 #include <string.h>
 #include <unistd.h>
+#include <libudev.h>
 
 #include "util.h"
 #include "checkers.h"
@@ -174,8 +175,8 @@ bool update_pathvec_from_dm(vector pathvec, struct multipath *mpp,
 				} else {
 					int rc;
 
-					devt2devname(pp->dev, sizeof(pp->dev),
-						     pp->dev_t);
+					strlcpy(pp->dev, udev_device_get_sysname(pp->udev),
+						sizeof(pp->dev));
 					conf = get_multipath_config();
 					pthread_cleanup_push(put_multipath_config,
 							     conf);
-- 
2.17.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 3/9] libmultipath: don't use fallback wwid in update_pathvec_from_dm

[Benjamin Marzinski]

When new paths are added in update_pathvec_from_dm(). If they can't get
their regular wwid, they shouldn't try the getting the fallback wwid,
and should just copy the wwid of the multipath device.

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

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index f25fe9e3..5edf9593 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -2363,15 +2363,16 @@ int pathinfo(struct path *pp, struct config *conf, int mask)
 	}
 
 	if ((mask & DI_WWID) && !strlen(pp->wwid)) {
-		get_uid(pp, path_state, pp->udev,
-			(pp->retriggers >= conf->retrigger_tries));
+		int allow_fallback = ((mask & DI_NOFALLBACK) == 0 &&
+				      pp->retriggers >= conf->retrigger_tries);
+		get_uid(pp, path_state, pp->udev, allow_fallback);
 		if (!strlen(pp->wwid)) {
 			if (pp->bus == SYSFS_BUS_UNDEF)
 				return PATHINFO_SKIPPED;
 			if (pp->initialized != INIT_FAILED) {
 				pp->initialized = INIT_MISSING_UDEV;
 				pp->tick = conf->retrigger_delay;
-			} else if (pp->retriggers >= conf->retrigger_tries &&
+			} else if (allow_fallback &&
 				   (pp->state == PATH_UP || pp->state == PATH_GHOST)) {
 				/*
 				 * We have failed to read udev info for this path
diff --git a/libmultipath/discovery.h b/libmultipath/discovery.h
index a5446b4d..095657bb 100644
--- a/libmultipath/discovery.h
+++ b/libmultipath/discovery.h
@@ -70,6 +70,7 @@ enum discovery_mode {
 	__DI_WWID,
 	__DI_BLACKLIST,
 	__DI_NOIO,
+	__DI_NOFALLBACK,
 };
 
 #define DI_SYSFS	(1 << __DI_SYSFS)
@@ -79,6 +80,7 @@ enum discovery_mode {
 #define DI_WWID		(1 << __DI_WWID)
 #define DI_BLACKLIST	(1 << __DI_BLACKLIST)
 #define DI_NOIO		(1 << __DI_NOIO) /* Avoid IO on the device */
+#define DI_NOFALLBACK	(1 << __DI_NOFALLBACK) /* do not allow wwid fallback */
 
 #define DI_ALL		(DI_SYSFS  | DI_SERIAL | DI_CHECKER | DI_PRIO | \
 			 DI_WWID)
diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 4d56107a..d363e7f6 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -182,8 +182,7 @@ bool update_pathvec_from_dm(vector pathvec, struct multipath *mpp,
 							     conf);
 					pp->checkint = conf->checkint;
 					rc = pathinfo(pp, conf,
-						      DI_SYSFS|DI_WWID|DI_BLACKLIST|
-						      pathinfo_flags);
+						      DI_SYSFS|DI_WWID|DI_BLACKLIST|DI_NOFALLBACK|pathinfo_flags);
 					pthread_cleanup_pop(1);
 					if (rc != PATHINFO_OK) {
 						condlog(1, "%s: error %d in pathinfo, discarding path",
-- 
2.17.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 4/9] libmultipath: always set INIT_REMOVED in set_path_removed

[Benjamin Marzinski]

Avoiding this corner case simplifies a future patch

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/structs_vec.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index d363e7f6..fb26437a 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -326,10 +326,8 @@ void set_path_removed(struct path *pp)
 	 * Keep link to mpp. It will be removed when the path
 	 * is successfully removed from the map.
 	 */
-	if (!mpp) {
+	if (!mpp)
 		condlog(0, "%s: internal error: mpp == NULL", pp->dev);
-		return;
-	}
 	pp->mpp = mpp;
 	pp->initialized = INIT_REMOVED;
 }
-- 
2.17.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 5/9] multipathd: fully initialize paths added by update_pathvec_from_dm

[Benjamin Marzinski]

When paths are added by update_pathvec_from_dm(), udev may not have
initialized them. This means that it's possible that they are supposed
to be blacklisted by udev properties, but weren't.  Also, in order to
avoid doing potentially stalling IO, update_pathvec_from_dm() doesn't
get all the path information in pathinfo(). These paths end up in the
unexpected state of INIT_MISSING_UDEV or INIT_NEW, but with their mpp
and wwid set.

If udev has already initialized the path, but multipathed wasn't
monitoring it, the blacklist checks and wwid determination in
update_pathvec_from_dm() will work correctly, so paths added by it are
safe, but not completely initialized. The most likely reason why this
would happen is if the path was manually removed from multipathd
monitoring with "multipathd del path". The most common time when
uninitialized paths would in be part of multipath devices is during
boot, after the pivot root, but before the udev coldplug happens. These
paths are not necessarily safe. It's possible that /etc/multipath.conf
in the initramfs and regular filesystem differ, and they should now be
either blacklisted by udev_property, or have a different wwid. However
an "add" event should appear for them shortly.

Multipath now has a new state to deal with these devices, INIT_PARTIAL.
Devices in this state are treated mostly like INIT_OK devices, but
when "multipathd add path" is called or an add/change uevent happens
on these devices, multipathd will finish initializing them, and remove
them if necessary.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/structs.h     |  6 +++++
 libmultipath/structs_vec.c |  5 ++--
 multipathd/cli_handlers.c  | 35 ++++++++++++++++++++++++--
 multipathd/main.c          | 51 +++++++++++++++++++++++++++++++++++---
 multipathd/main.h          |  1 +
 5 files changed, 90 insertions(+), 8 deletions(-)

diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index d0b266b7..69409fd4 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -200,6 +200,12 @@ enum initialized_states {
 	 * mapped by some multipath map because of map reload failure.
 	 */
 	INIT_REMOVED,
+	/*
+	 * INIT_PARTIAL: paths added by update_pathvec_from_dm() will not
+	 * be fully initialized. This will be handled when an add or
+	 * change uevent is received.
+	 */
+	INIT_PARTIAL,
 };
 
 enum prkey_sources {
diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index fb26437a..1de9175e 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -194,6 +194,7 @@ bool update_pathvec_from_dm(vector pathvec, struct multipath *mpp,
 					}
 					condlog(2, "%s: adding new path %s",
 						mpp->alias, pp->dev);
+					pp->initialized = INIT_PARTIAL;
 					store_path(pathvec, pp);
 					pp->tick = 1;
 				}
@@ -392,12 +393,12 @@ extract_hwe_from_path(struct multipath * mpp)
 	condlog(4, "%s: searching paths for valid hwe", mpp->alias);
 	/* doing this in two passes seems like paranoia to me */
 	vector_foreach_slot(mpp->paths, pp, i) {
-		if (pp->state == PATH_UP &&
+		if (pp->state == PATH_UP && pp->initialized != INIT_PARTIAL &&
 		    pp->initialized != INIT_REMOVED && pp->hwe)
 			goto done;
 	}
 	vector_foreach_slot(mpp->paths, pp, i) {
-		if (pp->state != PATH_UP &&
+		if ((pp->state != PATH_UP || pp->initialized == INIT_PARTIAL) &&
 		    pp->initialized != INIT_REMOVED && pp->hwe)
 			goto done;
 	}
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 58b9916c..5de428f3 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -493,6 +493,33 @@ cli_reset_map_stats (void *v, struct strbuf *reply, void *data)
 	return 0;
 }
 
+static int
+add_partial_path(struct path *pp, struct vectors *vecs)
+{
+	char wwid[WWID_SIZE];
+	struct udev_device *udd;
+
+	udd = get_udev_device(pp->dev_t, DEV_DEVT);
+	if (!udd)
+		return 0;
+	strcpy(wwid, pp->wwid);
+	if (get_uid(pp, pp->state, udd, 0) != 0) {
+		strcpy(pp->wwid, wwid);
+		udev_device_unref(udd);
+		return 0;
+	}
+	if (strlen(wwid) && strncmp(wwid, pp->wwid, WWID_SIZE) != 0) {
+		condlog(0, "%s: path wwid changed from '%s' to '%s'. removing",
+			pp->dev, wwid, pp->wwid);
+		ev_remove_path(pp, vecs, 1);
+		udev_device_unref(udd);
+		return -1;
+	}
+	udev_device_unref(pp->udev);
+	pp->udev = udd;
+	return finish_path_init(pp, vecs);
+}
+
 static int
 cli_add_path (void *v, struct strbuf *reply, void *data)
 {
@@ -518,8 +545,12 @@ cli_add_path (void *v, struct strbuf *reply, void *data)
 	if (pp && pp->initialized != INIT_REMOVED) {
 		condlog(2, "%s: path already in pathvec", param);
 
-		if (pp->recheck_wwid == RECHECK_WWID_ON &&
-		    check_path_wwid_change(pp)) {
+		if (pp->initialized == INIT_PARTIAL) {
+			if (add_partial_path(pp, vecs) < 0)
+				return 1;
+		}
+		else if (pp->recheck_wwid == RECHECK_WWID_ON &&
+			 check_path_wwid_change(pp)) {
 			condlog(0, "%s: wwid changed. Removing device",
 				pp->dev);
 			handle_path_wwid_change(pp, vecs);
diff --git a/multipathd/main.c b/multipathd/main.c
index cc4a4a5d..8f6be6b9 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -976,12 +976,19 @@ check_path_wwid_change(struct path *pp)
 	return false;
 }
 
+/*
+ * uev_add_path can call uev_update_path, and uev_update_path can call
+ * uev_add_path
+ */
+static int uev_update_path (struct uevent *uev, struct vectors * vecs);
+
 static int
 uev_add_path (struct uevent *uev, struct vectors * vecs, int need_do_map)
 {
 	struct path *pp;
 	int ret = 0, i;
 	struct config *conf;
+	bool partial_init = false;
 
 	condlog(3, "%s: add path (uevent)", uev->kernel);
 	if (strstr(uev->kernel, "..") != NULL) {
@@ -1000,7 +1007,10 @@ uev_add_path (struct uevent *uev, struct vectors * vecs, int need_do_map)
 		int r;
 		struct multipath *prev_mpp = NULL;
 
-		if (pp->initialized == INIT_REMOVED) {
+		if (pp->initialized == INIT_PARTIAL) {
+			partial_init = true;
+			goto out;
+		} else if (pp->initialized == INIT_REMOVED) {
 			condlog(3, "%s: re-adding removed path", pp->dev);
 			pp->initialized = INIT_NEW;
 			prev_mpp = pp->mpp;
@@ -1110,6 +1120,8 @@ uev_add_path (struct uevent *uev, struct vectors * vecs, int need_do_map)
 	}
 out:
 	lock_cleanup_pop(vecs->lock);
+	if (partial_init)
+		return uev_update_path(uev, vecs);
 	return ret;
 }
 
@@ -1405,6 +1417,28 @@ fail:
 	return REMOVE_PATH_MAP_ERROR;
 }
 
+int
+finish_path_init(struct path *pp, struct vectors * vecs)
+{
+	int r;
+	struct config *conf;
+
+	if (pp->udev && pp->uid_attribute && *pp->uid_attribute &&
+	    !udev_device_get_is_initialized(pp->udev))
+		return 0;
+	conf = get_multipath_config();
+	pthread_cleanup_push(put_multipath_config, conf);
+	r = pathinfo(pp, conf, DI_ALL|DI_BLACKLIST);
+	pthread_cleanup_pop(1);
+
+	if (r == PATHINFO_OK)
+		return 0;
+
+	condlog(0, "%s: error fully initializing path, removing", pp->dev);
+	ev_remove_path(pp, vecs, 1);
+	return -1;
+}
+
 static int
 uev_update_path (struct uevent *uev, struct vectors * vecs)
 {
@@ -1443,7 +1477,7 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
 		}
 		/* Don't deal with other types of failed initialization
 		 * now. check_path will handle it */
-		if (!strlen(pp->wwid))
+		if (!strlen(pp->wwid) && pp->initialized != INIT_PARTIAL)
 			goto out;
 
 		strcpy(wwid, pp->wwid);
@@ -1451,12 +1485,20 @@ uev_update_path (struct uevent *uev, struct vectors * vecs)
 
 		if (rc != 0)
 			strcpy(pp->wwid, wwid);
-		else if (strncmp(wwid, pp->wwid, WWID_SIZE) != 0) {
+		else if (strlen(wwid) &&
+			 strncmp(wwid, pp->wwid, WWID_SIZE) != 0) {
 			condlog(0, "%s: path wwid changed from '%s' to '%s'",
 				uev->kernel, wwid, pp->wwid);
 			ev_remove_path(pp, vecs, 1);
 			needs_reinit = 1;
 			goto out;
+		} else if (pp->initialized == INIT_PARTIAL) {
+			udev_device_unref(pp->udev);
+			pp->udev = udev_device_ref(uev->udev);
+			if (finish_path_init(pp, vecs) < 0) {
+				retval = 1;
+				goto out;
+			}
 		} else {
 			udev_device_unref(pp->udev);
 			pp->udev = udev_device_ref(uev->udev);
@@ -1507,6 +1549,7 @@ out:
 
 		condlog(0, "%s: spurious uevent, path not found", uev->kernel);
 	}
+	/* pp->initalized must not be INIT_PARTIAL if needs_reinit is set */
 	if (needs_reinit)
 		retval = uev_add_path(uev, vecs, 1);
 	return retval;
@@ -2116,7 +2159,7 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 	int marginal_pathgroups, marginal_changed = 0;
 	int ret;
 
-	if (((pp->initialized == INIT_OK ||
+	if (((pp->initialized == INIT_OK || pp->initialized == INIT_PARTIAL ||
 	      pp->initialized == INIT_REQUESTED_UDEV) && !pp->mpp) ||
 	    pp->initialized == INIT_REMOVED)
 		return 0;
diff --git a/multipathd/main.h b/multipathd/main.h
index c8a1ce92..4acd1b8c 100644
--- a/multipathd/main.h
+++ b/multipathd/main.h
@@ -66,4 +66,5 @@ int reload_and_sync_map(struct multipath *mpp, struct vectors *vecs,
 
 void handle_path_wwid_change(struct path *pp, struct vectors *vecs);
 bool check_path_wwid_change(struct path *pp);
+int finish_path_init(struct path *pp, struct vectors * vecs);
 #endif /* MAIN_H */
-- 
2.17.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 6/9] multipathd: retrigger uevent for partial paths

[Benjamin Marzinski]

If a partial path appears and is not fully initialized within
180 seconds, trigger a uevent. If the udev device is not initialized
trigger an add event. Otherwise, trigger a change event.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/libmultipath.version |  2 +-
 libmultipath/structs.h            |  1 +
 libmultipath/structs_vec.c        |  1 +
 multipathd/main.c                 | 20 ++++++++++++++++++++
 4 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
index 6473091d..58a7d1be 100644
--- a/libmultipath/libmultipath.version
+++ b/libmultipath/libmultipath.version
@@ -31,7 +31,7 @@
  *   The new version inherits the previous ones.
  */
 
-LIBMULTIPATH_10.0.0 {
+LIBMULTIPATH_11.0.0 {
 global:
 	/* symbols referenced by multipath and multipathd */
 	add_foreign;
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 69409fd4..c21d1eda 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -318,6 +318,7 @@ struct path {
 	int fd;
 	int initialized;
 	int retriggers;
+	int partial_retrigger_delay;
 	unsigned int path_failures;
 	time_t dis_reinstate_time;
 	int disable_reinstate;
diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 1de9175e..9b6407bd 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -195,6 +195,7 @@ bool update_pathvec_from_dm(vector pathvec, struct multipath *mpp,
 					condlog(2, "%s: adding new path %s",
 						mpp->alias, pp->dev);
 					pp->initialized = INIT_PARTIAL;
+					pp->partial_retrigger_delay = 180;
 					store_path(pathvec, pp);
 					pp->tick = 1;
 				}
diff --git a/multipathd/main.c b/multipathd/main.c
index 8f6be6b9..b0037721 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1948,6 +1948,25 @@ retry_count_tick(vector mpvec)
 	}
 }
 
+static void
+partial_retrigger_tick(vector pathvec)
+{
+	struct path *pp;
+	unsigned int i;
+
+	vector_foreach_slot (pathvec, pp, i) {
+		if (pp->initialized == INIT_PARTIAL && pp->udev &&
+		    pp->partial_retrigger_delay > 0 &&
+		    --pp->partial_retrigger_delay == 0) {
+			const char *msg = udev_device_get_is_initialized(pp->udev) ?
+					  "change" : "add";
+
+			sysfs_attr_set_value(pp->udev, "uevent", msg,
+					     strlen(msg));
+		}
+	}
+}
+
 int update_prio(struct path *pp, int refresh_all)
 {
 	int oldpriority;
@@ -2566,6 +2585,7 @@ checkerloop (void *ap)
 		retry_count_tick(vecs->mpvec);
 		missing_uev_wait_tick(vecs);
 		ghost_delay_tick(vecs);
+		partial_retrigger_tick(vecs->pathvec);
 		lock_cleanup_pop(vecs->lock);
 
 		if (count)
-- 
2.17.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 7/9] multipathd: remove INIT_PARTIAL paths that aren't in a multipath device

[Benjamin Marzinski]

The only reason multipath is monitoring an INIT_PARTIAL path is because
it was discovered in a multipath device table. If it stops being part
of a multipath device before it gets fully initialized, drop it.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/structs_vec.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 9b6407bd..df5709a0 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -308,9 +308,12 @@ void orphan_paths(vector pathvec, struct multipath *mpp, const char *reason)
 
 	vector_foreach_slot (pathvec, pp, i) {
 		if (pp->mpp == mpp) {
-			if (pp->initialized == INIT_REMOVED) {
-				condlog(3, "%s: freeing path in removed state",
-					pp->dev);
+			if (pp->initialized == INIT_REMOVED ||
+			    pp->initialized == INIT_PARTIAL) {
+				condlog(3, "%s: freeing path in %s state",
+					pp->dev,
+					pp->initialized == INIT_REMOVED ?
+					"removed" : "partial");
 				vector_del_slot(pathvec, i--);
 				free_path(pp);
 			} else
@@ -469,11 +472,14 @@ static void check_removed_paths(const struct multipath *mpp, vector pathvec)
 	int i;
 
 	vector_foreach_slot(pathvec, pp, i) {
-		if (pp->initialized != INIT_REMOVED || pp->mpp != mpp)
-			continue;
-		if (!find_devt_in_pathgroups(mpp, pp->dev_t)) {
-			condlog(2, "%s: %s: freeing path in removed state",
-				__func__, pp->dev);
+		if (pp->mpp == mpp &&
+		    (pp->initialized == INIT_REMOVED ||
+		     pp->initialized == INIT_PARTIAL) &&
+		    !find_devt_in_pathgroups(mpp, pp->dev_t)) {
+			condlog(2, "%s: %s: freeing path in %s state",
+				__func__, pp->dev,
+				pp->initialized == INIT_REMOVED ?
+				"removed" : "partial");
 			vector_del_slot(pathvec, i--);
 			free_path(pp);
 		}
-- 
2.17.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 8/9] multipathd: Remove dependency on systemd-udev-settle.service

[Benjamin Marzinski]

multipathd can now handle starting up with incompletely initialized
paths, so it no longer needs to wait for the device Coldplug to
complete. However multipathd may need to write to /etc (for the wwids
and bindings files), so in needs to wait for the root filesystem to
be remounted read/write before starting.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/multipathd.service | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/multipathd/multipathd.service b/multipathd/multipathd.service
index 0b2ac814..87cb5349 100644
--- a/multipathd/multipathd.service
+++ b/multipathd/multipathd.service
@@ -1,9 +1,8 @@
 [Unit]
 Description=Device-Mapper Multipath Device Controller
-Wants=systemd-udev-trigger.service systemd-udev-settle.service
 Before=iscsi.service iscsid.service lvm2-activation-early.service
 Before=local-fs-pre.target blk-availability.service shutdown.target
-After=multipathd.socket systemd-udev-trigger.service systemd-udev-settle.service
+After=multipathd.socket systemd-remount-fs.service
 DefaultDependencies=no
 Conflicts=shutdown.target
 ConditionKernelCommandLine=!nompath
-- 
2.17.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 9/9] libmultipath: add path wildcard "%I" for init state

[Benjamin Marzinski]

From: Martin Wilck <mwilck@suse.com>

Enable printing pp->initialized with 'multipathd show paths format "%I"'.
This is supposed to go on top of Ben's "multipathd: remove udev settle
dependency" series, to simplify checking multipathd's state.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/print.c   | 21 +++++++++++++++++++++
 libmultipath/structs.h |  1 +
 2 files changed, 22 insertions(+)

diff --git a/libmultipath/print.c b/libmultipath/print.c
index d2ef0104..e61349f9 100644
--- a/libmultipath/print.c
+++ b/libmultipath/print.c
@@ -504,6 +504,26 @@ snprint_dm_path_state (struct strbuf *buff, const struct path * pp)
 	}
 }
 
+static int snprint_initialized(struct strbuf *buff, const struct path * pp)
+{
+	static const char *init_state_name[] = {
+		[INIT_NEW] = "new",
+		[INIT_FAILED] = "failed",
+		[INIT_MISSING_UDEV] = "udev-missing",
+		[INIT_REQUESTED_UDEV] = "udev-requested",
+		[INIT_OK] = "ok",
+		[INIT_REMOVED] = "removed",
+		[INIT_PARTIAL] = "partial",
+	};
+	const char *str;
+
+	if (pp->initialized < INIT_NEW || pp->initialized >= __INIT_LAST)
+		str = "undef";
+	else
+		str = init_state_name[pp->initialized];
+	return append_strbuf_str(buff, str);
+}
+
 static int
 snprint_vpr (struct strbuf *buff, const struct path * pp)
 {
@@ -804,6 +824,7 @@ struct path_data pd[] = {
 	{'g', "vpd page data", 0, snprint_path_vpd_data},
 	{'0', "failures",      0, snprint_path_failures},
 	{'P', "protocol",      0, snprint_path_protocol},
+	{'I', "init_st",       0, snprint_initialized},
 	{0, NULL, 0 , NULL}
 };
 
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index c21d1eda..c0f8929c 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -206,6 +206,7 @@ enum initialized_states {
 	 * change uevent is received.
 	 */
 	INIT_PARTIAL,
+	__INIT_LAST,
 };
 
 enum prkey_sources {
-- 
2.17.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 0/9] multipathd: remove udev settle dependency

[Martin Wilck]

On Wed, 2021-11-17 at 15:21 -0600, Benjamin Marzinski wrote:
> So, it turns out that commit 4ef67017 "libmultipath: add
> update_pathvec_from_dm()" already does most of the hard work of
> making
> multipath handle the uninitialized paths that exist during boot,
> after
> the switch root, but before the all the coldplug uevents have been
> processed. The only problem is that this code leaves the paths in a
> partially initialized state, which doesn't completely get corrected
> until a reconfigure happens.
> 
> This patchset makes multipath track these partially initailized
> paths,
> and makes sure that they get fully initialized, or removed, as
> necessary.
> 
> The first patch is a tangentially related bug I found when trying
> (unsuccessfully) to recreate multipathd's problem with dropping
> uninitialized paths. Multipathd was not removing completely missing
> paths from maps that it found while starting up. The solution I chose
> was just to make sure that a full reload will happen on these maps,
> even
> if a weak reconfigure was requested. However, this means that
> multipath
> still completely ignores these missing paths. A side effect of this
> is
> that "multipath -l" does not show these paths, even though they exist
> as
> part of the multipath table. The full reloads are necessary, to take
> care of issues that update_pathvec_from_dm() can run into, but we
> might
> also want to think about marking these missing paths as INIT_REMOVED,
> instead of not adding them at all. This would make "multipath -l"
> still
> show them, until they actually get removed from the table.
> 
> Patch 0005 makes sure to fully initialize the paths when their
> coldplug
> event occurs, but if the path is already fully initialized in udev,
> but
> multipathd finds out about it from update_pathvec_from_dm(),
> multipath
> doesn't do anything to finish initializing the path and moving it to
> INIT_OK, besides waiting for a uevent or a reconfigure. Patch 0006
> handles this by triggering a uevent if the path has been in partial
> for more than 3 minutes.
> 
> I've tested these patches both by rebooting with necessary and
> unnecessary multipath devices in the initramfs and multipathd.service
> set to make multipathd start up at various points after the switch
> root,
> and by manually sending remove uevents to unintialize some paths, and
> then starting multipathd to test specific combinations of path
> states.
> But more testing is always welcome.
> 
> Notes on v2:
> 
> After playing around with the initialization states a bit, I decided
> that cleaning them up is a bigger task than I want to do in this
> patchset. This set just concentrates on cleaning up our handling of
> paths that get added in update_pathvec_from_dm() to allow us to
> drop the systemd-udev-settle dependency.
> 
> Changes from v1, base on suggestions by Martin Wilck.
> 
> 0005: Made cli_add_path() verify the wwid can be gotten, and hasn't
> changed, before attempting to finish initalizing the path. Also don't
> intialize a path that relies on udev if the udev device isn't
> initalized.
> 
> 0006: New patch. If a patch is in INIT_PARTIAL for over 3 minutes,
> trigger a uevent on it.  This is kind of a long waiting period, but I
> want to avoid firing off another uevent in cases where the problem is
> that udev is in the middle of a uevent storm, and the expected event
> is delayed.
> 
> 0009: Repost of Martin's new init state wildcard patch
> 
> Benjamin Marzinski (8):
>   multipathd: remove missing paths on startup
>   libmultipath: skip unneeded steps to get path name
>   libmultipath: don't use fallback wwid in update_pathvec_from_dm
>   libmultipath: always set INIT_REMOVED in set_path_removed
>   multipathd: fully initialize paths added by update_pathvec_from_dm
>   multipathd: retrigger uevent for partial paths
>   multipathd: remove INIT_PARTIAL paths that aren't in a multipath
>     device
>   multipathd: Remove dependency on systemd-udev-settle.service
> 
> Martin Wilck (1):
>   libmultipath: add path wildcard "%I" for init state
> 
>  libmultipath/configure.c          |  2 +
>  libmultipath/devmapper.c          |  2 +
>  libmultipath/discovery.c          |  7 +--
>  libmultipath/discovery.h          |  2 +
>  libmultipath/libmultipath.version |  2 +-
>  libmultipath/print.c              | 21 +++++++++
>  libmultipath/structs.h            |  9 ++++
>  libmultipath/structs_vec.c        | 41 ++++++++++--------
>  multipathd/cli_handlers.c         | 35 ++++++++++++++-
>  multipathd/main.c                 | 71
> +++++++++++++++++++++++++++++--
>  multipathd/main.h                 |  1 +
>  multipathd/multipathd.service     |  3 +-
>  12 files changed, 167 insertions(+), 29 deletions(-)
> 

For the series:

Reviewed-by: Martin Wilck <mwilck@suse.com>
(You may have noticed by the tags in my re-sent series yesterday).

I still have to give it some more testing, but it's looking good so
far.

As these patches are based on top of my series, I'll apply this (and
the "reconfigure all" series) to the "queue" branch when that series is
finalized.

Regards
Martin






--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 0/9] multipathd: remove udev settle dependency

[Martin Wilck]

On Wed, 2021-11-17 at 15:21 -0600, Benjamin Marzinski wrote:
> So, it turns out that commit 4ef67017 "libmultipath: add
> update_pathvec_from_dm()" already does most of the hard work of
> making
> multipath handle the uninitialized paths that exist during boot,
> after
> the switch root, but before the all the coldplug uevents have been
> processed. The only problem is that this code leaves the paths in a
> partially initialized state, which doesn't completely get corrected
> until a reconfigure happens.
> 
>  [...]
>
> I've tested these patches both by rebooting with necessary and
> unnecessary multipath devices in the initramfs and multipathd.service
> set to make multipathd start up at various points after the switch
> root,
> and by manually sending remove uevents to unintialize some paths, and
> then starting multipathd to test specific combinations of path
> states.
> But more testing is always welcome.

My late testing has revealed an issue with this patch with explicit
ALUA. It's similar to what you solved with the "ghost_delay" parameter
in the past.

With this patch, multipathd now starts before SCSI device detection
begins, and as soon as multipathd sets up a map, I/O on this map may be
started. With arrays supporting Active/optimized and Active/non-
optimized states and explicit ALUA, this causes unnecessary path state
switching if paths in non-optimized state are detected before optimized
ones. I/O will cause scsi_dh_activate() to be called in the kernel, and
this will run an STPG, which always uses active/optimized as target
state.

With RDDAC, we'll have a similar problem. The other device handlers
don't distinguish active and optimal states, AFAICS.

I fear this behavior will not be welcome in some configurations. So far
I haven't made up my mind how, and if at all, we can fix it. I suppose
something similar to ghost_delay would be possible on the multipath-
tools side, but it's not straightforward, because non-optimized paths
simply count as PATH_UP in multipathd. Also, the delay should probably
be much shorter than for PATH_GHOST. In my testing against a LIO
target, it was a matter of milliseconds which path would appear first.

Alternatively, maybe we can consider the way scsi_dh_activate() works?
Perhaps it doesn't have to switch from active/non-optimized to
active/optimized state? OTOH, there are other situation (explicit path
group switch) where we'd want exactly that.

The other alternative would be waiting for udev settle again. I'd
really like to avoid that.

Ideas and thoughts highly welcome.

Regards,
Martin





--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 0/9] multipathd: remove udev settle dependency

[Martin Wilck]

On Mon, 2022-01-24 at 12:19 +0100, Martin Wilck wrote:
> 
> My late testing has revealed an issue with this patch with explicit
> ALUA. It's similar to what you solved with the "ghost_delay"
> parameter
> in the past.
> 
> With this patch, multipathd now starts before SCSI device detection
> begins, and as soon as multipathd sets up a map, I/O on this map may
> be
> started. With arrays supporting Active/optimized and Active/non-
> optimized states and explicit ALUA, this causes unnecessary path
> state
> switching if paths in non-optimized state are detected before
> optimized
> ones. I/O will cause scsi_dh_activate() to be called in the kernel,
> and
> this will run an STPG, which always uses active/optimized as target
> state.
> 
> With RDDAC, we'll have a similar problem. The other device handlers
> don't distinguish active and optimal states, AFAICS.
> 
> I fear this behavior will not be welcome in some configurations. So
> far
> I haven't made up my mind how, and if at all, we can fix it. I
> suppose
> something similar to ghost_delay would be possible on the multipath-
> tools side, but it's not straightforward, because non-optimized paths
> simply count as PATH_UP in multipathd. Also, the delay should
> probably
> be much shorter than for PATH_GHOST. In my testing against a LIO
> target, it was a matter of milliseconds which path would appear
> first.
> 
> Alternatively, maybe we can consider the way scsi_dh_activate()
> works?
> Perhaps it doesn't have to switch from active/non-optimized to
> active/optimized state? OTOH, there are other situation (explicit
> path
> group switch) where we'd want exactly that.

In discussion with Hannes, we came to the conclusion:

 - for ALUA, the effect mentioned in my post can be avoided using
   the kernel parameter "scsi_dh_alua.optimize_stpg=1". Confirmed by
   testing.
 - even if this parameter is not used, spurious switching between 
   non-optimized and optimized state is non fatal, and much less
   resource-intensive on the storage side than switching between active
   and standby states.

So, it's not a big issue, after all...

> The other alternative would be waiting for udev settle again. I'd
> really like to avoid that.

... and this won't be necessary.

Martin


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 0/9] multipathd: remove udev settle dependency

[Benjamin Marzinski]

On Mon, Jan 24, 2022 at 11:19:53AM +0000, Martin Wilck wrote:
> On Wed, 2021-11-17 at 15:21 -0600, Benjamin Marzinski wrote:
> 
> My late testing has revealed an issue with this patch with explicit
> ALUA. It's similar to what you solved with the "ghost_delay" parameter
> in the past.
> 
> With this patch, multipathd now starts before SCSI device detection
> begins, and as soon as multipathd sets up a map, I/O on this map may be
> started. With arrays supporting Active/optimized and Active/non-
> optimized states and explicit ALUA, this causes unnecessary path state
> switching if paths in non-optimized state are detected before optimized
> ones. I/O will cause scsi_dh_activate() to be called in the kernel, and
> this will run an STPG, which always uses active/optimized as target
> state.
> 
> With RDDAC, we'll have a similar problem. The other device handlers
> don't distinguish active and optimal states, AFAICS.

Just to clarify things, is there a difference between this and the case
that has always existed, where multipathd is up and running and a new
storage array gets discovered? The active/optimized paths aren't always
discovered first.

-Ben
 
> I fear this behavior will not be welcome in some configurations. So far
> I haven't made up my mind how, and if at all, we can fix it. I suppose
> something similar to ghost_delay would be possible on the multipath-
> tools side, but it's not straightforward, because non-optimized paths
> simply count as PATH_UP in multipathd. Also, the delay should probably
> be much shorter than for PATH_GHOST. In my testing against a LIO
> target, it was a matter of milliseconds which path would appear first.
> 
> Alternatively, maybe we can consider the way scsi_dh_activate() works?
> Perhaps it doesn't have to switch from active/non-optimized to
> active/optimized state? OTOH, there are other situation (explicit path
> group switch) where we'd want exactly that.
> 
> The other alternative would be waiting for udev settle again. I'd
> really like to avoid that.
> 
> Ideas and thoughts highly welcome.
> 
> Regards,
> Martin
> 
> 
> 

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 0/9] multipathd: remove udev settle dependency

[Martin Wilck]

On Wed, 2022-01-26 at 09:51 -0600, Benjamin Marzinski wrote:
> 
> Just to clarify things, is there a difference between this and the
> case
> that has always existed, where multipathd is up and running and a new
> storage array gets discovered? The active/optimized paths aren't
> always
> discovered first.

No, that's the same thing. Just that now, multipathd is always up
before the paths show up.

As I said in my other post, I don't think it's a big issue. But it's a
significant change in how booting with multipath works.

Martin


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel