[PATCH 00/74] multipath-tools series part V: removed path handling

[PATCH 00/74] multipath-tools series part V: removed path handling

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Hi Christophe, hi Ben,

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

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-5".

The series addresses several issues with missing, unintialized, and removed
paths. The motivation was that I strongly dislike the side effects of
disassemble_map(), adding paths and setting WWIDs while parsing map
parameters. IMO this has always been a layering violation. This patch set adds
a new function dedicated to handling paths which have not been detected via
udev but are present as members of dm maps. That makes it much easier to
track and improve the logic applied when such devices are detected.

I believe that the new logic will also fix the issue recently reported by
Chongyun ("libmultipath/dmparser: add missing path with good status when sync
state with dm kernel"). At least, the approach is very similar, although
the call to pathinfo() for new devices now happens in update_pathvec_from_dm()
rather than in disassemble_map().

The patch set also gets rid of the "is_daemon" argument for disassemble_map(),
causing semantics in daemon and non-daemon mode. The reason for this parameter
goes way back into history; it was avoiding to re-add manually removed paths
because they were still showing up in maps. But OTOH we must add missing
devices which we've failed to detect. The patch set handles this by tracking
the state of "being removed" as a new init state, and only actually removing
the paths from pathvec when they don't show up in maps any more.

Regards
Martin

Martin Wilck (21):
  libmultipath: protect use of pp->udev
  libmultipath: add uninitialize_path()
  multipath-tools: introduce INIT_REMOVED state
  libmultipath: update_mpp_paths(): handle INIT_REMOVED
  libmultipath: verify_paths(): don't delete paths from pathvec
  libmultipath: sync_paths(): handle INIT_REMOVED
  libmultipath: orphan_paths(): delete paths in INIT_REMOVED state
  libmultipath: adopt_paths(): skip removed paths
  multipathd: ev_remove_path(): use INIT_REMOVED
  multipathd: deal with INIT_REMOVED during path addition
  multipathd: check_path(): set retrigger_delay if necessary
  libmultipath: add update_pathvec_from_dm()
  libmultipath: update_pathvec_from_dm: handle pp->mpp mismatch
  libmultipath: disassemble_map(): always search paths by dev_t
  libmultipath: disassemble_map(): require non-NULL pathvec
  libmultipath: disassemble_map(): get rid of "is_daemon" argument
  libmultipath: disassemble_map(): do not change pathvec and WWIDs
  multipath: use update_pathvec_from_dm()
  libmpathpersist: use update_pathvec_from_dm()
  libmultipath: decrease loglevel in store_path()
  libmultipath: dmparser: constify function arguments

 libmpathpersist/mpath_persist.c       |  56 +-----
 libmultipath/configure.c              |   2 +-
 libmultipath/discovery.c              |  15 +-
 libmultipath/dmparser.c               |  70 ++-----
 libmultipath/dmparser.h               |   4 +-
 libmultipath/prioritizers/alua_rtpg.c |   6 +-
 libmultipath/structs.c                |  21 +-
 libmultipath/structs.h                |   6 +
 libmultipath/structs_vec.c            | 263 +++++++++++++++++++++++---
 libmultipath/structs_vec.h            |  11 +-
 multipath/main.c                      |  71 +------
 multipathd/cli_handlers.c             |  49 ++++-
 multipathd/main.c                     | 113 ++++++++---
 13 files changed, 445 insertions(+), 242 deletions(-)

-- 
2.26.2

[PATCH 54/74] libmultipath: protect use of pp->udev

[mwilck]

From: Martin Wilck <mwilck@suse.com>

We could never be 100% certain that pp->udev was always set.
With the upcoming change, we can be even less certain. Always
check pp->udev before using it.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/discovery.c              | 11 +++++++++--
 libmultipath/prioritizers/alua_rtpg.c |  6 ++++--
 libmultipath/structs_vec.c            |  2 +-
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index caabfef..c202d58 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -339,7 +339,10 @@ sysfs_get_tgt_nodename(struct path *pp, char *node)
 	struct udev_device *parent, *tgtdev;
 	int host, channel, tgtid = -1;
 
-	parent = udev_device_get_parent_with_subsystem_devtype(pp->udev, "scsi", "scsi_device");
+	if (!pp->udev)
+		return 1;
+	parent = udev_device_get_parent_with_subsystem_devtype(pp->udev,
+							 "scsi", "scsi_device");
 	if (!parent)
 		return 1;
 	/* Check for SAS */
@@ -1378,7 +1381,8 @@ nvme_sysfs_pathinfo (struct path *pp, const struct _vector *hwtable)
 	const char *attr_path = NULL;
 	const char *attr;
 
-	attr_path = udev_device_get_sysname(pp->udev);
+	if (pp->udev)
+		attr_path = udev_device_get_sysname(pp->udev);
 	if (!attr_path)
 		return PATHINFO_FAILED;
 
@@ -1958,6 +1962,9 @@ static ssize_t uid_fallback(struct path *pp, int path_state,
 		}
 	} else if (pp->bus == SYSFS_BUS_NVME) {
 		char value[256];
+
+		if (!pp->udev)
+			return -1;
 		len = sysfs_attr_get_value(pp->udev, "wwid", value,
 					   sizeof(value));
 		if (len <= 0)
diff --git a/libmultipath/prioritizers/alua_rtpg.c b/libmultipath/prioritizers/alua_rtpg.c
index bbf5aac..420a2e3 100644
--- a/libmultipath/prioritizers/alua_rtpg.c
+++ b/libmultipath/prioritizers/alua_rtpg.c
@@ -188,9 +188,11 @@ retry:
 int do_inquiry(const struct path *pp, int evpd, unsigned int codepage,
 	       void *resp, int resplen, unsigned int timeout)
 {
-	struct udev_device *ud;
+	struct udev_device *ud = NULL;
 
-	ud = udev_device_get_parent_with_subsystem_devtype(pp->udev, "scsi",
+	if (pp->udev)
+		ud = udev_device_get_parent_with_subsystem_devtype(pp->udev,
+								   "scsi",
 							   "scsi_device");
 	if (ud != NULL) {
 		int rc;
diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index bc47d1e..0b8c548 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -494,7 +494,7 @@ int verify_paths(struct multipath *mpp, struct vectors *vecs)
 		/*
 		 * see if path is in sysfs
 		 */
-		if (sysfs_attr_get_value(pp->udev, "dev",
+		if (!pp->udev || sysfs_attr_get_value(pp->udev, "dev",
 					 pp->dev_t, BLK_DEV_SIZE) < 0) {
 			if (pp->state != PATH_DOWN) {
 				condlog(1, "%s: removing valid path %s in state %d",
-- 
2.26.2

[PATCH 55/74] libmultipath: add uninitialize_path()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This helper clears all fields of struct path (except pp->udev) that must be
re-ininitialized if the path ever is to be used again.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/structs.c     | 19 +++++++++++++++++--
 libmultipath/structs.h     |  1 +
 libmultipath/structs_vec.c |  9 +--------
 3 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/libmultipath/structs.c b/libmultipath/structs.c
index 9407462..056a205 100644
--- a/libmultipath/structs.c
+++ b/libmultipath/structs.c
@@ -114,19 +114,34 @@ alloc_path (void)
 }
 
 void
-free_path (struct path * pp)
+uninitialize_path(struct path *pp)
 {
 	if (!pp)
 		return;
 
+	pp->dmstate = PSTATE_UNDEF;
+	pp->uid_attribute = NULL;
+	pp->getuid = NULL;
+
 	if (checker_selected(&pp->checker))
 		checker_put(&pp->checker);
 
 	if (prio_selected(&pp->prio))
 		prio_put(&pp->prio);
 
-	if (pp->fd >= 0)
+	if (pp->fd >= 0) {
 		close(pp->fd);
+		pp->fd = -1;
+	}
+}
+
+void
+free_path (struct path * pp)
+{
+	if (!pp)
+		return;
+
+	uninitialize_path(pp);
 
 	if (pp->udev) {
 		udev_device_unref(pp->udev);
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index d69bc2e..894099d 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -416,6 +416,7 @@ struct host_group {
 struct path * alloc_path (void);
 struct pathgroup * alloc_pathgroup (void);
 struct multipath * alloc_multipath (void);
+void uninitialize_path(struct path *pp);
 void free_path (struct path *);
 void free_pathvec (vector vec, enum free_path_mode free_paths);
 void free_pathgroup (struct pathgroup * pgp, enum free_path_mode free_paths);
diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 0b8c548..05c8626 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -98,14 +98,7 @@ void orphan_path(struct path *pp, const char *reason)
 {
 	condlog(3, "%s: orphan path, %s", pp->dev, reason);
 	pp->mpp = NULL;
-	pp->dmstate = PSTATE_UNDEF;
-	pp->uid_attribute = NULL;
-	pp->getuid = NULL;
-	prio_put(&pp->prio);
-	checker_put(&pp->checker);
-	if (pp->fd >= 0)
-		close(pp->fd);
-	pp->fd = -1;
+	uninitialize_path(pp);
 }
 
 void orphan_paths(vector pathvec, struct multipath *mpp, const char *reason)
-- 
2.26.2

[PATCH 56/74] multipath-tools: introduce INIT_REMOVED state

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Introduce a new state for pp->initialized, INIT_REMOVED. This state
means that the path is about to be removed, either by a remove uevent
or by the operator. It will normally be a very short-lived state, because
the path will be deleted from pathvec quickly after setting this state.
Only if the path is member of a multipath map, and reloading the map
fails, this state will persist until a later map reload or flush eventually
cancels the membership in the map.

Paths in INIT_REMOVED state are treated as if they didn't exist.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/discovery.c   |  4 ++++
 libmultipath/structs.h     |  5 +++++
 libmultipath/structs_vec.c | 17 +++++++++++++++++
 libmultipath/structs_vec.h |  1 +
 multipathd/main.c          |  5 +++--
 5 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index c202d58..efcef67 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -2078,6 +2078,10 @@ int pathinfo(struct path *pp, struct config *conf, int mask)
 	if (!pp || !conf)
 		return PATHINFO_FAILED;
 
+	/* Treat removed paths as if they didn't exist */
+	if (pp->initialized == INIT_REMOVED)
+		return PATHINFO_FAILED;
+
 	/*
 	 * For behavior backward-compatibility with multipathd,
 	 * the blacklisting by filter_property|devnode() is not
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 894099d..1f0a78a 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -193,6 +193,11 @@ enum initialized_states {
 	INIT_MISSING_UDEV,
 	INIT_REQUESTED_UDEV,
 	INIT_OK,
+	/*
+	 * INIT_REMOVED: supposed to be removed from pathvec, but still
+	 * mapped by some multipath map because of map reload failure.
+	 */
+	INIT_REMOVED,
 };
 
 enum prkey_sources {
diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 05c8626..27d6547 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -113,6 +113,23 @@ void orphan_paths(vector pathvec, struct multipath *mpp, const char *reason)
 	}
 }
 
+void set_path_removed(struct path *pp)
+{
+	struct multipath *mpp = pp->mpp;
+
+	orphan_path(pp, "removed");
+	/*
+	 * Keep link to mpp. It will be removed when the path
+	 * is successfully removed from the map.
+	 */
+	if (!mpp) {
+		condlog(0, "%s: internal error: mpp == NULL", pp->dev);
+		return;
+	}
+	pp->mpp = mpp;
+	pp->initialized = INIT_REMOVED;
+}
+
 void
 remove_map(struct multipath * mpp, struct vectors * vecs, int purge_vec)
 {
diff --git a/libmultipath/structs_vec.h b/libmultipath/structs_vec.h
index 4b3b8b7..cf7d569 100644
--- a/libmultipath/structs_vec.h
+++ b/libmultipath/structs_vec.h
@@ -18,6 +18,7 @@ int adopt_paths (vector pathvec, struct multipath * mpp);
 void orphan_paths(vector pathvec, struct multipath *mpp,
 		  const char *reason);
 void orphan_path (struct path * pp, const char *reason);
+void set_path_removed(struct path *pp);
 
 int verify_paths(struct multipath * mpp, struct vectors * vecs);
 int update_mpp_paths(struct multipath * mpp, vector pathvec);
diff --git a/multipathd/main.c b/multipathd/main.c
index cd0e29b..4a5aa17 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1962,8 +1962,9 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 	int marginal_pathgroups, marginal_changed = 0;
 	int ret;
 
-	if ((pp->initialized == INIT_OK ||
-	     pp->initialized == INIT_REQUESTED_UDEV) && !pp->mpp)
+	if (((pp->initialized == INIT_OK ||
+	      pp->initialized == INIT_REQUESTED_UDEV) && !pp->mpp) ||
+	    pp->initialized == INIT_REMOVED)
 		return 0;
 
 	if (pp->tick)
-- 
2.26.2

[PATCH 57/74] libmultipath: update_mpp_paths(): handle INIT_REMOVED

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Since the ancient commit b96dead ("[multipathd] remove the retry login in
uev_remove_path()"), update_mpp_paths() was used to check whether devices
found in disassembled maps were still in the pathvec, and to skip them
while re-assembling the new params string on reload. The reason was to
deal with failed reloads. With the introduction of INIT_REMOVED, we
need to skip paths in that state, too. Moreover, past reloads may have
succeeded in removing REMOVED paths from the map, so check if any
INIT_REMOVED paths can now be deleted for good. This is the right
place to do this check, because update_mpp_paths() is called after
obtaining the current kernel state, and before reloading.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/structs_vec.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 27d6547..8999552 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -29,6 +29,7 @@ int update_mpp_paths(struct multipath *mpp, vector pathvec)
 	struct pathgroup * pgp;
 	struct path * pp;
 	int i,j;
+	bool store_failure = false;
 
 	if (!mpp || !mpp->pg)
 		return 0;
@@ -39,13 +40,23 @@ int update_mpp_paths(struct multipath *mpp, vector pathvec)
 
 	vector_foreach_slot (mpp->pg, pgp, i) {
 		vector_foreach_slot (pgp->paths, pp, j) {
-			if (!find_path_by_devt(mpp->paths, pp->dev_t) &&
-			    (find_path_by_devt(pathvec, pp->dev_t)) &&
-			    store_path(mpp->paths, pp))
-				return 1;
+			if (!find_path_by_devt(mpp->paths, pp->dev_t)) {
+				struct path *pp1;
+
+				/*
+				 * Avoid adding removed paths to the map again
+				 * when we reload it. Such paths may exist if
+				 * domap fails in ev_remove_path().
+				 */
+				pp1 = find_path_by_devt(pathvec, pp->dev_t);
+				if (pp1 && pp->initialized != INIT_REMOVED &&
+				    store_path(mpp->paths, pp))
+					store_failure = true;
+			}
 		}
 	}
-	return 0;
+
+	return store_failure;
 }
 
 int adopt_paths(vector pathvec, struct multipath *mpp)
-- 
2.26.2

[PATCH 58/74] libmultipath: verify_paths(): don't delete paths from pathvec

[mwilck]

From: Martin Wilck <mwilck@suse.com>

If we encounter a non-existing device verify_paths(), just set
it to INIT_REMOVED state. Actual path deletion is postponed until
we don't see that path in the kernel map any more.

This allows us to get rid of the "pathvec" argument to this function.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c   |  2 +-
 libmultipath/structs_vec.c | 16 +++++++++-------
 libmultipath/structs_vec.h |  2 +-
 multipathd/main.c          |  4 ++--
 4 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 48426cd..2509053 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -1196,7 +1196,7 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
 				set_bit_in_bitfield(i, size_mismatch_seen);
 			}
 		}
-		verify_paths(mpp, vecs);
+		verify_paths(mpp);
 
 		if (does_alias_exist(newmp, mpp)) {
 			remove_map(mpp, vecs, PURGE_VEC);
diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 8999552..5634101 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -502,11 +502,11 @@ out:
 	return NULL;
 }
 
-int verify_paths(struct multipath *mpp, struct vectors *vecs)
+int verify_paths(struct multipath *mpp)
 {
 	struct path * pp;
 	int count = 0;
-	int i, j;
+	int i;
 
 	if (!mpp)
 		return 0;
@@ -521,7 +521,7 @@ int verify_paths(struct multipath *mpp, struct vectors *vecs)
 				condlog(1, "%s: removing valid path %s in state %d",
 					mpp->alias, pp->dev, pp->state);
 			} else {
-				condlog(3, "%s: failed to access path %s",
+				condlog(2, "%s: failed to access path %s",
 					mpp->alias, pp->dev);
 			}
 			count++;
@@ -534,10 +534,12 @@ int verify_paths(struct multipath *mpp, struct vectors *vecs)
 			 */
 			if (mpp->hwe == pp->hwe)
 				mpp->hwe = NULL;
-			if ((j = find_slot(vecs->pathvec,
-					   (void *)pp)) != -1)
-				vector_del_slot(vecs->pathvec, j);
-			free_path(pp);
+			/*
+			 * Don't delete path from pathvec yet. We'll do this
+			 * after the path has been removed from the map, in
+			 * sync_paths().
+			 */
+			set_path_removed(pp);
 		} else {
 			condlog(4, "%s: verified path %s dev_t %s",
 				mpp->alias, pp->dev, pp->dev_t);
diff --git a/libmultipath/structs_vec.h b/libmultipath/structs_vec.h
index cf7d569..cd3ef76 100644
--- a/libmultipath/structs_vec.h
+++ b/libmultipath/structs_vec.h
@@ -20,7 +20,7 @@ void orphan_paths(vector pathvec, struct multipath *mpp,
 void orphan_path (struct path * pp, const char *reason);
 void set_path_removed(struct path *pp);
 
-int verify_paths(struct multipath * mpp, struct vectors * vecs);
+int verify_paths(struct multipath *mpp);
 int update_mpp_paths(struct multipath * mpp, vector pathvec);
 int update_multipath_strings (struct multipath *mpp, vector pathvec,
 			      int is_daemon);
diff --git a/multipathd/main.c b/multipathd/main.c
index 4a5aa17..402e179 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -492,7 +492,7 @@ retry:
 		retries = -1;
 		goto fail;
 	}
-	verify_paths(mpp, vecs);
+	verify_paths(mpp);
 	mpp->action = ACT_RELOAD;
 
 	if (setup_map(mpp, params, PARAMS_SIZE, vecs)) {
@@ -959,7 +959,7 @@ rescan:
 		if (adopt_paths(vecs->pathvec, mpp))
 			goto fail; /* leave path added to pathvec */
 
-		verify_paths(mpp, vecs);
+		verify_paths(mpp);
 		mpp->action = ACT_RELOAD;
 	} else {
 		if (!should_multipath(pp, vecs->pathvec, vecs->mpvec)) {
-- 
2.26.2

[PATCH 59/74] libmultipath: sync_paths(): handle INIT_REMOVED

[mwilck]

From: Martin Wilck <mwilck@suse.com>

sync_paths() is the function which is called after getting kernel
state with disassemble_map(). This is the place where we should
check if paths that can eventually be deleted.

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

diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 5634101..faa1a2a 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -264,6 +264,38 @@ update_multipath_status (struct multipath *mpp)
 	return DMP_OK;
 }
 
+static struct path *find_devt_in_pathgroups(const struct multipath *mpp,
+					    const char *dev_t)
+{
+	struct pathgroup  *pgp;
+	struct path *pp;
+	int j;
+
+	vector_foreach_slot(mpp->pg, pgp, j) {
+		pp = find_path_by_devt(pgp->paths, dev_t);
+		if (pp)
+			return pp;
+	}
+	return NULL;
+}
+
+static void check_removed_paths(const struct multipath *mpp, vector pathvec)
+{
+	struct path *pp;
+	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);
+			vector_del_slot(pathvec, i--);
+			free_path(pp);
+		}
+	}
+}
+
 void sync_paths(struct multipath *mpp, vector pathvec)
 {
 	struct path *pp;
@@ -284,6 +316,7 @@ void sync_paths(struct multipath *mpp, vector pathvec)
 			orphan_path(pp, "path removed externally");
 		}
 	}
+	check_removed_paths(mpp, pathvec);
 	update_mpp_paths(mpp, pathvec);
 	vector_foreach_slot (mpp->paths, pp, i)
 		pp->mpp = mpp;
-- 
2.26.2

[PATCH 60/74] libmultipath: orphan_paths(): delete paths in INIT_REMOVED state

[mwilck]

From: Martin Wilck <mwilck@suse.com>

A path in INIT_REMOVED state is only waiting for its last association
with a multipath map to be dropped. When orphan_paths() encounters
such a path, rather than orphaning it, free it.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/structs_vec.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index faa1a2a..7c68592 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -119,7 +119,13 @@ void orphan_paths(vector pathvec, struct multipath *mpp, const char *reason)
 
 	vector_foreach_slot (pathvec, pp, i) {
 		if (pp->mpp == mpp) {
-			orphan_path(pp, reason);
+			if (pp->initialized == INIT_REMOVED) {
+				condlog(3, "%s: freeing path in removed state",
+					pp->dev);
+				vector_del_slot(pathvec, i--);
+				free_path(pp);
+			} else
+				orphan_path(pp, reason);
 		}
 	}
 }
-- 
2.26.2

[PATCH 61/74] libmultipath: adopt_paths(): skip removed paths

[mwilck]

From: Martin Wilck <mwilck@suse.com>

If we don't do this, pathinfo() will fail on these paths, causing
adopt_paths() to fail.

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

diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 7c68592..1e0f109 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -79,6 +79,8 @@ int adopt_paths(vector pathvec, struct multipath *mpp)
 					pp->dev, mpp->alias);
 				continue;
 			}
+			if (pp->initialized == INIT_REMOVED)
+				continue;
 			condlog(3, "%s: ownership set to %s",
 				pp->dev, mpp->alias);
 			pp->mpp = mpp;
-- 
2.26.2

Re: [PATCH 61/74] libmultipath: adopt_paths(): skip removed paths

[Benjamin Marzinski]

On Thu, Jul 09, 2020 at 12:51:32PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> If we don't do this, pathinfo() will fail on these paths, causing
> adopt_paths() to fail.
> 

This is probably unnecessary, but it seems safer to make sure that
pp->mpp is set to mpp, before bailing out on removed paths.

-Ben

> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/structs_vec.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
> index 7c68592..1e0f109 100644
> --- a/libmultipath/structs_vec.c
> +++ b/libmultipath/structs_vec.c
> @@ -79,6 +79,8 @@ int adopt_paths(vector pathvec, struct multipath *mpp)
>  					pp->dev, mpp->alias);
>  				continue;
>  			}
> +			if (pp->initialized == INIT_REMOVED)
> +				continue;
>  			condlog(3, "%s: ownership set to %s",
>  				pp->dev, mpp->alias);
>  			pp->mpp = mpp;
> -- 
> 2.26.2

[PATCH 62/74] multipathd: ev_remove_path(): use INIT_REMOVED

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Set paths belonging to a map to INIT_REMOVED state before attempting
to reload or flush the map. If the map association is successfully removed,
the path will be actually deleted, either via flush_map() -> orphan_paths(),
or in the update_multipath_strings()->sync_paths() code path.

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

diff --git a/multipathd/main.c b/multipathd/main.c
index 402e179..545eb6d 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1102,6 +1102,18 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
 			goto fail;
 		}
 
+		/*
+		 * Mark the path as removed. In case of success, we
+		 * will delete it for good. Otherwise, it will be deleted
+		 * later, unless all attempts to reload this map fail.
+		 * Note: we have to explicitly remove pp from mpp->paths,
+		 * update_mpp_paths() doesn't do that.
+		 */
+		set_path_removed(pp);
+		i = find_slot(mpp->paths, pp);
+		if (i != -1)
+			vector_del_slot(mpp->paths, i);
+
 		/*
 		 * Make sure mpp->hwe doesn't point to freed memory
 		 * We call extract_hwe_from_path() below to restore mpp->hwe
@@ -1109,9 +1121,6 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
 		if (mpp->hwe == pp->hwe)
 			mpp->hwe = NULL;
 
-		if ((i = find_slot(mpp->paths, (void *)pp)) != -1)
-			vector_del_slot(mpp->paths, i);
-
 		/*
 		 * remove the map IF removing the last path
 		 */
@@ -1135,6 +1144,7 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
 					" removing all paths",
 					alias);
 				retval = 0;
+				/* flush_map() has freed the path */
 				goto out;
 			}
 			/*
@@ -1171,21 +1181,27 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
 			/*
 			 * update our state from kernel
 			 */
+			char devt[BLK_DEV_SIZE];
+
+			strlcpy(devt, pp->dev_t, sizeof(devt));
 			if (setup_multipath(vecs, mpp))
 				return 1;
+			/*
+			 * Successful map reload without this path:
+			 * sync_map_state() will free it.
+			 */
 			sync_map_state(mpp);
 
-			condlog(2, "%s [%s]: path removed from map %s",
-				pp->dev, pp->dev_t, mpp->alias);
+			condlog(2, "%s: path removed from map %s",
+				devt, mpp->alias);
 		}
+	} else {
+		/* mpp == NULL */
+		if ((i = find_slot(vecs->pathvec, (void *)pp)) != -1)
+			vector_del_slot(vecs->pathvec, i);
+		free_path(pp);
 	}
-
 out:
-	if ((i = find_slot(vecs->pathvec, (void *)pp)) != -1)
-		vector_del_slot(vecs->pathvec, i);
-
-	free_path(pp);
-
 	return retval;
 
 fail:
-- 
2.26.2

[PATCH 63/74] multipathd: deal with INIT_REMOVED during path addition

[mwilck]

From: Martin Wilck <mwilck@suse.com>

With the introduction of INIT_REMOVED, we have to deal with the situation
when a path is re-added in this state. This enables us to detect the
situation where a path is added while still part of a map after a failed
removal, which we couldn't before. Dealing with this correctly requires
some additional logic. There's a good case (re-added path is still mapped
by a map with matching WWID) and a bad case (non-matching WWID).

The logic is very similar in uev_add_path() and cli_add_path().

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/cli_handlers.c | 49 +++++++++++++++++++++++++++++++++--
 multipathd/main.c         | 54 ++++++++++++++++++++++++++++++++++++---
 2 files changed, 97 insertions(+), 6 deletions(-)

diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 782bb00..679fd57 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -713,11 +713,56 @@ cli_add_path (void * v, char ** reply, int * len, void * data)
 		goto blacklisted;
 
 	pp = find_path_by_dev(vecs->pathvec, param);
-	if (pp) {
+	if (pp && pp->initialized != INIT_REMOVED) {
 		condlog(2, "%s: path already in pathvec", param);
 		if (pp->mpp)
 			return 0;
-	} else {
+	} else if (pp) {
+		/* Trying to add a path in INIT_REMOVED state */
+		struct multipath *prev_mpp;
+
+		prev_mpp = pp->mpp;
+		pp->mpp = NULL;
+		pp->initialized = INIT_NEW;
+		pp->wwid[0] = '\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 (prev_mpp) {
+			/* Similar logic as in uev_add_path() */
+			pp->mpp = prev_mpp;
+			if (r == PATHINFO_OK &&
+			    !strncmp(prev_mpp->wwid, pp->wwid, WWID_SIZE)) {
+				condlog(2, "%s: path re-added to %s", pp->dev,
+					pp->mpp->alias);
+				return 0;
+			} else if (!ev_remove_path(pp, vecs, true))
+				/* Path removed in ev_remove_path() */
+				pp = NULL;
+			else {
+				/* Init state is now INIT_REMOVED again */
+				pp->dmstate = PSTATE_FAILED;
+				dm_fail_path(pp->mpp->alias, pp->dev_t);
+				condlog(1, "%s: failed to re-add path still mapped in %s",
+					pp->dev, pp->mpp->alias);
+				return 1;
+			}
+		} else {
+			switch (r) {
+			case PATHINFO_SKIPPED:
+				goto blacklisted;
+			case PATHINFO_OK:
+				break;
+			default:
+				condlog(0, "%s: failed to get pathinfo", param);
+				return 1;
+			}
+		}
+	}
+
+	if (!pp) {
 		struct udev_device *udevice;
 
 		udevice = udev_device_new_from_subsystem_sysname(udev,
diff --git a/multipathd/main.c b/multipathd/main.c
index 545eb6d..7b2d320 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -842,9 +842,21 @@ uev_add_path (struct uevent *uev, struct vectors * vecs, int need_do_map)
 	pp = find_path_by_dev(vecs->pathvec, uev->kernel);
 	if (pp) {
 		int r;
+		struct multipath *prev_mpp = NULL;
+		bool was_removed = pp->initialized == INIT_REMOVED;
+
+		if (was_removed) {
+			condlog(3, "%s: re-adding removed path", pp->dev);
+			pp->initialized = INIT_NEW;
+			prev_mpp = pp->mpp;
+			pp->mpp = NULL;
+			/* make sure get_uid() is called */
+			pp->wwid[0] = '\0';
+		} else
+			condlog(3,
+				"%s: spurious uevent, path already in pathvec",
+				uev->kernel);
 
-		condlog(3, "%s: spurious uevent, path already in pathvec",
-			uev->kernel);
 		if (!pp->mpp && !strlen(pp->wwid)) {
 			condlog(3, "%s: reinitialize path", uev->kernel);
 			udev_device_unref(pp->udev);
@@ -854,9 +866,43 @@ uev_add_path (struct uevent *uev, struct vectors * vecs, int need_do_map)
 			r = pathinfo(pp, conf,
 				     DI_ALL | DI_BLACKLIST);
 			pthread_cleanup_pop(1);
-			if (r == PATHINFO_OK)
+			if (r == PATHINFO_OK && !prev_mpp)
 				ret = ev_add_path(pp, vecs, need_do_map);
-			else if (r == PATHINFO_SKIPPED) {
+			else if (r == PATHINFO_OK &&
+				 !strncmp(pp->wwid, prev_mpp->wwid, WWID_SIZE)) {
+				/*
+				 * Path was unsuccessfully removed, but now
+				 * re-added, and still belongs to the right map
+				 * - all fine.
+				 */
+				pp->mpp = prev_mpp;
+				ret = 0;
+			} else if (prev_mpp) {
+				/*
+				 * Bad: re-added path still hangs in wrong map
+				 * Make another attempt to remove the path
+				 */
+				pp->mpp = prev_mpp;
+				ret = ev_remove_path(pp, vecs, true);
+				if (r == PATHINFO_OK && !ret)
+					/*
+					 * Path successfully freed, move on to
+					 * "new path" code path below
+					 */
+					pp = NULL;
+				else {
+					/*
+					 * Failure in ev_remove_path will keep
+					 * path in pathvec in INIT_REMOVED state
+					 * Fail the path to make sure it isn't
+					 * used any more.
+					 */
+					pp->dmstate = PSTATE_FAILED;
+					dm_fail_path(pp->mpp->alias, pp->dev_t);
+					condlog(1, "%s: failed to re-add path still mapped in %s",
+						pp->dev, pp->mpp->alias);
+				}
+			} else if (r == PATHINFO_SKIPPED) {
 				condlog(3, "%s: remove blacklisted path",
 					uev->kernel);
 				i = find_slot(vecs->pathvec, (void *)pp);
-- 
2.26.2

Re: [PATCH 63/74] multipathd: deal with INIT_REMOVED during path addition

[Benjamin Marzinski]

On Thu, Jul 09, 2020 at 12:51:34PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> With the introduction of INIT_REMOVED, we have to deal with the situation
> when a path is re-added in this state. This enables us to detect the
> situation where a path is added while still part of a map after a failed
> removal, which we couldn't before. Dealing with this correctly requires
> some additional logic. There's a good case (re-added path is still mapped
> by a map with matching WWID) and a bad case (non-matching WWID).
> 
> The logic is very similar in uev_add_path() and cli_add_path().
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipathd/cli_handlers.c | 49 +++++++++++++++++++++++++++++++++--
>  multipathd/main.c         | 54 ++++++++++++++++++++++++++++++++++++---
>  2 files changed, 97 insertions(+), 6 deletions(-)
> 
> diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> index 782bb00..679fd57 100644
> --- a/multipathd/cli_handlers.c
> +++ b/multipathd/cli_handlers.c
> @@ -713,11 +713,56 @@ cli_add_path (void * v, char ** reply, int * len, void * data)
>  		goto blacklisted;
>  
>  	pp = find_path_by_dev(vecs->pathvec, param);
> -	if (pp) {
> +	if (pp && pp->initialized != INIT_REMOVED) {
>  		condlog(2, "%s: path already in pathvec", param);
>  		if (pp->mpp)
>  			return 0;
> -	} else {
> +	} else if (pp) {
> +		/* Trying to add a path in INIT_REMOVED state */
> +		struct multipath *prev_mpp;
> +
> +		prev_mpp = pp->mpp;

O.k. I might be missing something here. How is it possible for a
path to be in the INIT_REMOVED state, and not have pp->mpp set?
Is this just a safety check?  Is so, should we print a message,
since this is an unexpected state.

> +		pp->mpp = NULL;
> +		pp->initialized = INIT_NEW;
> +		pp->wwid[0] = '\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 (prev_mpp) {
> +			/* Similar logic as in uev_add_path() */
> +			pp->mpp = prev_mpp;
> +			if (r == PATHINFO_OK &&
> +			    !strncmp(prev_mpp->wwid, pp->wwid, WWID_SIZE)) {
> +				condlog(2, "%s: path re-added to %s", pp->dev,
> +					pp->mpp->alias);

Should multipath be reinstating the path with dm here? I realize that it
will get resinstated when it's checked next. At the very least,
multipathd should be making sure it get checked the next time it's
examined in checkerloop.

> +				return 0;
> +			} else if (!ev_remove_path(pp, vecs, true))
> +				/* Path removed in ev_remove_path() */
> +				pp = NULL;
> +			else {
> +				/* Init state is now INIT_REMOVED again */
> +				pp->dmstate = PSTATE_FAILED;
> +				dm_fail_path(pp->mpp->alias, pp->dev_t);
> +				condlog(1, "%s: failed to re-add path still mapped in %s",
> +					pp->dev, pp->mpp->alias);
> +				return 1;
> +			}
> +		} else {
> +			switch (r) {
> +			case PATHINFO_SKIPPED:
> +				goto blacklisted;
> +			case PATHINFO_OK:
> +				break;
> +			default:
> +				condlog(0, "%s: failed to get pathinfo", param);
> +				return 1;
> +			}
> +		}
> +	}
> +
> +	if (!pp) {
>  		struct udev_device *udevice;
>  
>  		udevice = udev_device_new_from_subsystem_sysname(udev,
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 545eb6d..7b2d320 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -842,9 +842,21 @@ uev_add_path (struct uevent *uev, struct vectors * vecs, int need_do_map)
>  	pp = find_path_by_dev(vecs->pathvec, uev->kernel);
>  	if (pp) {
>  		int r;
> +		struct multipath *prev_mpp = NULL;
> +		bool was_removed = pp->initialized == INIT_REMOVED;

was_removed is only used once, so it's not necessary.

> +
> +		if (was_removed) {
> +			condlog(3, "%s: re-adding removed path", pp->dev);
> +			pp->initialized = INIT_NEW;
> +			prev_mpp = pp->mpp;
> +			pp->mpp = NULL;
> +			/* make sure get_uid() is called */
> +			pp->wwid[0] = '\0';
> +		} else
> +			condlog(3,
> +				"%s: spurious uevent, path already in pathvec",
> +				uev->kernel);
>  
> -		condlog(3, "%s: spurious uevent, path already in pathvec",
> -			uev->kernel);
>  		if (!pp->mpp && !strlen(pp->wwid)) {
>  			condlog(3, "%s: reinitialize path", uev->kernel);
>  			udev_device_unref(pp->udev);
> @@ -854,9 +866,43 @@ uev_add_path (struct uevent *uev, struct vectors * vecs, int need_do_map)
>  			r = pathinfo(pp, conf,
>  				     DI_ALL | DI_BLACKLIST);
>  			pthread_cleanup_pop(1);
> -			if (r == PATHINFO_OK)
> +			if (r == PATHINFO_OK && !prev_mpp)
>  				ret = ev_add_path(pp, vecs, need_do_map);
> -			else if (r == PATHINFO_SKIPPED) {
> +			else if (r == PATHINFO_OK &&

again, should we reinstate this path?

> +				 !strncmp(pp->wwid, prev_mpp->wwid, WWID_SIZE)) {
> +				/*
> +				 * Path was unsuccessfully removed, but now
> +				 * re-added, and still belongs to the right map
> +				 * - all fine.
> +				 */
> +				pp->mpp = prev_mpp;
> +				ret = 0;
> +			} else if (prev_mpp) {
> +				/*
> +				 * Bad: re-added path still hangs in wrong map
> +				 * Make another attempt to remove the path
> +				 */
> +				pp->mpp = prev_mpp;
> +				ret = ev_remove_path(pp, vecs, true);
> +				if (r == PATHINFO_OK && !ret)
> +					/*
> +					 * Path successfully freed, move on to
> +					 * "new path" code path below
> +					 */
> +					pp = NULL;
> +				else {
> +					/*
> +					 * Failure in ev_remove_path will keep
> +					 * path in pathvec in INIT_REMOVED state
> +					 * Fail the path to make sure it isn't
> +					 * used any more.
> +					 */
> +					pp->dmstate = PSTATE_FAILED;
> +					dm_fail_path(pp->mpp->alias, pp->dev_t);
> +					condlog(1, "%s: failed to re-add path still mapped in %s",
> +						pp->dev, pp->mpp->alias);
> +				}
> +			} else if (r == PATHINFO_SKIPPED) {
>  				condlog(3, "%s: remove blacklisted path",
>  					uev->kernel);
>  				i = find_slot(vecs->pathvec, (void *)pp);
> -- 
> 2.26.2

[PATCH 64/74] multipathd: check_path(): set retrigger_delay if necessary

[mwilck]

From: Martin Wilck <mwilck@suse.com>

In a follow up patch, we will set INIT_MISSING_UDEV and set tick=1
(minimal) at the same time. In this case, which is new, check_path()
must reset the delay when it first triggers an uevent.

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

diff --git a/multipathd/main.c b/multipathd/main.c
index 7b2d320..0cd0ee6 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2019,6 +2019,7 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 	int disable_reinstate = 0;
 	int oldchkrstate = pp->chkrstate;
 	int retrigger_tries, verbosity;
+	unsigned int retrigger_delay;
 	unsigned int checkint, max_checkint;
 	struct config *conf;
 	int marginal_pathgroups, marginal_changed = 0;
@@ -2036,6 +2037,7 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 
 	conf = get_multipath_config();
 	retrigger_tries = conf->retrigger_tries;
+	retrigger_delay = conf->retrigger_delay;
 	checkint = conf->checkint;
 	max_checkint = conf->max_checkint;
 	verbosity = conf->verbosity;
@@ -2048,6 +2050,8 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 	};
 
 	if (!pp->mpp && pp->initialized == INIT_MISSING_UDEV) {
+		if (pp->tick != retrigger_delay)
+			pp->tick = conf->retrigger_delay;
 		if (pp->retriggers < retrigger_tries) {
 			condlog(2, "%s: triggering change event to reinitialize",
 				pp->dev);
-- 
2.26.2

Re: [PATCH 64/74] multipathd: check_path(): set retrigger_delay if necessary

[Benjamin Marzinski]

On Thu, Jul 09, 2020 at 12:51:35PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> In a follow up patch, we will set INIT_MISSING_UDEV and set tick=1
> (minimal) at the same time. In this case, which is new, check_path()
> must reset the delay when it first triggers an uevent.

Maybe I'm just being obtuse here, but I don't get what this does.
pp->tick will always be 0 for any path that makes it to the check

if (!pp->mpp && pp->initialized == INIT_MISSING_UDEV) {

And then if it's not out of retries, the path will get set to
INIT_REQUESTED_UDEV, and pathinfo() will take care of resetting pp->tick
when it gets called by the new uevent.

If it is out of retries, the path won't get pp->tick reset, which seems
wrong, but it that case it should probably be set to max_checkint, like
happens when the "add missing paths" code fails.

Or like I said, maybe I'm just missing something.
-Ben

> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipathd/main.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 7b2d320..0cd0ee6 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -2019,6 +2019,7 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
>  	int disable_reinstate = 0;
>  	int oldchkrstate = pp->chkrstate;
>  	int retrigger_tries, verbosity;
> +	unsigned int retrigger_delay;
>  	unsigned int checkint, max_checkint;
>  	struct config *conf;
>  	int marginal_pathgroups, marginal_changed = 0;
> @@ -2036,6 +2037,7 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
>  
>  	conf = get_multipath_config();
>  	retrigger_tries = conf->retrigger_tries;
> +	retrigger_delay = conf->retrigger_delay;
>  	checkint = conf->checkint;
>  	max_checkint = conf->max_checkint;
>  	verbosity = conf->verbosity;
> @@ -2048,6 +2050,8 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
>  	};
>  
>  	if (!pp->mpp && pp->initialized == INIT_MISSING_UDEV) {
> +		if (pp->tick != retrigger_delay)
> +			pp->tick = conf->retrigger_delay;
>  		if (pp->retriggers < retrigger_tries) {
>  			condlog(2, "%s: triggering change event to reinitialize",
>  				pp->dev);
> -- 
> 2.26.2

Re: [PATCH 64/74] multipathd: check_path(): set retrigger_delay if necessary

[Martin Wilck]

On Sun, 2020-07-19 at 00:07 -0500, Benjamin Marzinski wrote:
> On Thu, Jul 09, 2020 at 12:51:35PM +0200, mwilck@suse.com wrote:
> > From: Martin Wilck <mwilck@suse.com>
> > 
> > In a follow up patch, we will set INIT_MISSING_UDEV and set tick=1
> > (minimal) at the same time. In this case, which is new,
> > check_path()
> > must reset the delay when it first triggers an uevent.
> 
> Maybe I'm just being obtuse here, but I don't get what this does.
> pp->tick will always be 0 for any path that makes it to the check
> 
> if (!pp->mpp && pp->initialized == INIT_MISSING_UDEV) {
> 
> And then if it's not out of retries, the path will get set to
> INIT_REQUESTED_UDEV, and pathinfo() will take care of resetting pp-
> >tick
> when it gets called by the new uevent.
> 
> If it is out of retries, the path won't get pp->tick reset, which
> seems
> wrong, but it that case it should probably be set to max_checkint,
> like
> happens when the "add missing paths" code fails.
> 
> Or like I said, maybe I'm just missing something.

You're not. This was just plain stupid.

Thanks
Martin

[PATCH 65/74] libmultipath: add update_pathvec_from_dm()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

It can happen in particular during boot or startup that we encounter
paths as map members which haven't been discovered or fully initialized
yet, and are thus not in the pathvec. These paths need special treatment
in various ways. Currently this is dealt with in disassemble_map(). That's
a layer violation, and the way it is done is suboptimal in various ways.

As a preparation to change that, this patch introduces a new function,
update_pathvec_from_dm(), that is supposed to deal with newly discovered
paths from disassemble_map(). It has to be called after disassemble_map()
has finished.

The logic of the new function is similar but not identical to what
disassemble_map() was doing before. Firstly, the function will simply
remove path devices that don't exist - there's no point in carrying these
around. Map reloads can't be called from this code for reasons of the
overall program logic. But it's prepared to signal to the caller that
a map reload is in order. Using this return value will be future work.

Second, pathinfo() is now called on new paths rather than just setting
pp->dev. The pathinfo flags can be passed as argument to make the function
flexible for different callers.

Finally, treatment of WWIDs is different now. There'll be only one attempt
at guessing the map WWID if it's not set yet. If a non-matching path WWID
is found, the path is removed, and a new uevent triggered (this is the
most important change wrt the dangerous previous code that would simply
overwrite the path WWID). If the path WWID is empty, it will still be
set from the map WWID, which I consider not perfect, but no worse
than what we did before.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/structs_vec.c | 134 +++++++++++++++++++++++++++++++++++++
 libmultipath/structs_vec.h |   2 +
 2 files changed, 136 insertions(+)

diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 1e0f109..5dd37d5 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -59,6 +59,140 @@ int update_mpp_paths(struct multipath *mpp, vector pathvec)
 	return store_failure;
 }
 
+static bool guess_mpp_wwid(struct multipath *mpp)
+{
+	int i, j;
+	struct pathgroup *pgp;
+	struct path *pp;
+
+	if (strlen(mpp->wwid) || !mpp->pg)
+		return true;
+
+	vector_foreach_slot(mpp->pg, pgp, i) {
+		if (!pgp->paths)
+			continue;
+		vector_foreach_slot(pgp->paths, pp, j) {
+			if (pp->initialized == INIT_OK && strlen(pp->wwid)) {
+				strlcpy(mpp->wwid, pp->wwid, sizeof(mpp->wwid));
+				condlog(2, "%s: guessed WWID %s from path %s",
+					mpp->alias, mpp->wwid, pp->dev);
+				return true;
+			}
+		}
+	}
+	condlog(1, "%s: unable to guess WWID", mpp->alias);
+	return false;
+}
+
+/*
+ * update_pathvec_from_dm() - update pathvec after disassemble_map()
+ *
+ * disassemble_map() may return block devices that are members in
+ * multipath maps but haven't been discovered. Check whether they
+ * need to be added to pathvec or discarded.
+ *
+ * Returns: true if immediate map reload is desirable
+ *
+ * Side effects:
+ * - may delete non-existing paths and empty pathgroups from mpp
+ * - may set pp->wwid and / or mpp->wwid
+ * - calls pathinfo() on existing paths is pathinfo_flags is not 0
+ */
+bool update_pathvec_from_dm(vector pathvec, struct multipath *mpp,
+	int pathinfo_flags)
+{
+	int i, j;
+	struct pathgroup *pgp;
+	struct path *pp;
+	struct config *conf;
+	bool mpp_has_wwid;
+	bool must_reload = false;
+
+	if (!mpp->pg)
+		return false;
+
+	mpp_has_wwid = guess_mpp_wwid(mpp);
+
+	vector_foreach_slot(mpp->pg, pgp, i) {
+		if (!pgp->paths)
+			goto delete_pg;
+
+		vector_foreach_slot(pgp->paths, pp, j) {
+			pp->mpp = mpp;
+
+			if (pp->udev) {
+				if (pathinfo_flags) {
+					conf = get_multipath_config();
+					pthread_cleanup_push(put_multipath_config,
+							     conf);
+					pathinfo(pp, conf, pathinfo_flags);
+					pthread_cleanup_pop(1);
+				}
+				continue;
+			}
+
+			/* If this fails, the device is not in sysfs */
+			pp->udev = get_udev_device(pp->dev_t, DEV_DEVT);
+			if (!pp->udev) {
+				condlog(2, "%s: discarding non-existing path %s",
+					mpp->alias, pp->dev_t);
+				vector_del_slot(pgp->paths, j--);
+				free_path(pp);
+				must_reload = true;
+			} else {
+
+				devt2devname(pp->dev, sizeof(pp->dev),
+					     pp->dev_t);
+				conf = get_multipath_config();
+				pthread_cleanup_push(put_multipath_config,
+						     conf);
+				pp->checkint = conf->checkint;
+				if (pathinfo(pp, conf,
+					     DI_SYSFS|DI_WWID|pathinfo_flags)
+				    != PATHINFO_OK)
+					condlog(1, "%s: error in pathinfo",
+						pp->dev);
+				pthread_cleanup_pop(1);
+				if (mpp_has_wwid && !strlen(pp->wwid)) {
+					condlog(3, "%s: setting wwid from map: %s",
+						pp->dev, mpp->wwid);
+					strlcpy(pp->wwid, mpp->wwid,
+						sizeof(pp->wwid));
+				} else if (mpp_has_wwid &&
+					   strcmp(mpp->wwid, pp->wwid)) {
+
+					condlog(0, "%s: path %s WWID %s doesn't match, removing from map",
+						mpp->wwid, pp->dev_t, pp->wwid);
+					/*
+					 * This path exists, but in the wong map.
+					 * We can't reload the map from here.
+					 * Instead, treat this path like "missing udev",
+					 * which it probably is.
+					 * check_path() will trigger an uevent
+					 * and reset pp->tick.
+					 */
+					must_reload = true;
+					pp->mpp = NULL;
+					dm_fail_path(mpp->alias, pp->dev_t);
+					vector_del_slot(pgp->paths, j--);
+					pp->initialized = INIT_MISSING_UDEV;
+					pp->tick = 1;
+				}
+				condlog(2, "%s: adding new path %s",
+					mpp->alias, pp->dev);
+				store_path(pathvec, pp);
+			}
+		}
+		if (VECTOR_SIZE(pgp->paths) != 0)
+			continue;
+	delete_pg:
+		condlog(2, "%s: removing empty pathgroup %d", mpp->alias, i);
+		vector_del_slot(mpp->pg, i--);
+		free_pathgroup(pgp, KEEP_PATHS);
+	}
+	return must_reload;
+}
+
 int adopt_paths(vector pathvec, struct multipath *mpp)
 {
 	int i, ret;
diff --git a/libmultipath/structs_vec.h b/libmultipath/structs_vec.h
index cd3ef76..4c28148 100644
--- a/libmultipath/structs_vec.h
+++ b/libmultipath/structs_vec.h
@@ -21,6 +21,8 @@ void orphan_path (struct path * pp, const char *reason);
 void set_path_removed(struct path *pp);
 
 int verify_paths(struct multipath *mpp);
+bool update_pathvec_from_dm(vector pathvec, struct multipath *mpp,
+			    int pathinfo_flags);
 int update_mpp_paths(struct multipath * mpp, vector pathvec);
 int update_multipath_strings (struct multipath *mpp, vector pathvec,
 			      int is_daemon);
-- 
2.26.2

Re: [PATCH 65/74] libmultipath: add update_pathvec_from_dm()

[Benjamin Marzinski]

On Thu, Jul 09, 2020 at 12:51:36PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> It can happen in particular during boot or startup that we encounter
> paths as map members which haven't been discovered or fully initialized
> yet, and are thus not in the pathvec. These paths need special treatment
> in various ways. Currently this is dealt with in disassemble_map(). That's
> a layer violation, and the way it is done is suboptimal in various ways.
> 
> As a preparation to change that, this patch introduces a new function,
> update_pathvec_from_dm(), that is supposed to deal with newly discovered
> paths from disassemble_map(). It has to be called after disassemble_map()
> has finished.

I don't really understand the reason for adding paths to pathvec, just
because they appear in disassemble_map(). If multipathd sees a map with
a path that it doesn't have in pathvec, then that path should either
sortly appear, or multipathd likely doesn't have it for a reason.  I
agree that it's possible that uevent got missed, but it seems more
likely that the user updated multipath.conf and hasn't reconfigured
multipathd. 

Maybe when I read future patches this will make more sense, but can't
multipathd add paths to the pathvec that it is configured to blacklist.
I would like to think that multipathd is definitive for the multipath
devices that it controls.  If multipathd doesn't think a path belongs in
a multipath device, then the path doesn't belong in the multipath
device. There are corner cases where a path can appear in a multipath
device before multipathd gets notified about the path, but they all seem
pretty rare. Someone running "multipath" before multipathd has seen the
path addition uevent, for instance. Perhaps if pathinfo() should always
set DI_BLACKLIST, and if the path is skipped, flag the device for
reloading.
 
> The logic of the new function is similar but not identical to what
> disassemble_map() was doing before. Firstly, the function will simply
> remove path devices that don't exist - there's no point in carrying these
> around. Map reloads can't be called from this code for reasons of the
> overall program logic. But it's prepared to signal to the caller that
> a map reload is in order. Using this return value will be future work.
> 
> Second, pathinfo() is now called on new paths rather than just setting
> pp->dev. The pathinfo flags can be passed as argument to make the function
> flexible for different callers.
> 
> Finally, treatment of WWIDs is different now. There'll be only one attempt
> at guessing the map WWID if it's not set yet. If a non-matching path WWID
> is found, the path is removed, and a new uevent triggered (this is the
> most important change wrt the dangerous previous code that would simply
> overwrite the path WWID). If the path WWID is empty, it will still be
> set from the map WWID, which I consider not perfect, but no worse
> than what we did before.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/structs_vec.c | 134 +++++++++++++++++++++++++++++++++++++
>  libmultipath/structs_vec.h |   2 +
>  2 files changed, 136 insertions(+)
> 
> diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
> index 1e0f109..5dd37d5 100644
> --- a/libmultipath/structs_vec.c
> +++ b/libmultipath/structs_vec.c
> @@ -59,6 +59,140 @@ int update_mpp_paths(struct multipath *mpp, vector pathvec)
>  	return store_failure;
>  }
>  
> +static bool guess_mpp_wwid(struct multipath *mpp)
> +{
> +	int i, j;
> +	struct pathgroup *pgp;
> +	struct path *pp;
> +
> +	if (strlen(mpp->wwid) || !mpp->pg)
> +		return true;
> +
> +	vector_foreach_slot(mpp->pg, pgp, i) {
> +		if (!pgp->paths)
> +			continue;
> +		vector_foreach_slot(pgp->paths, pp, j) {
> +			if (pp->initialized == INIT_OK && strlen(pp->wwid)) {
> +				strlcpy(mpp->wwid, pp->wwid, sizeof(mpp->wwid));
> +				condlog(2, "%s: guessed WWID %s from path %s",
> +					mpp->alias, mpp->wwid, pp->dev);
> +				return true;
> +			}
> +		}
> +	}
> +	condlog(1, "%s: unable to guess WWID", mpp->alias);
> +	return false;
> +}
> +
> +/*
> + * update_pathvec_from_dm() - update pathvec after disassemble_map()
> + *
> + * disassemble_map() may return block devices that are members in
> + * multipath maps but haven't been discovered. Check whether they
> + * need to be added to pathvec or discarded.
> + *
> + * Returns: true if immediate map reload is desirable
> + *
> + * Side effects:
> + * - may delete non-existing paths and empty pathgroups from mpp
> + * - may set pp->wwid and / or mpp->wwid
> + * - calls pathinfo() on existing paths is pathinfo_flags is not 0
> + */
> +bool update_pathvec_from_dm(vector pathvec, struct multipath *mpp,
> +	int pathinfo_flags)
> +{
> +	int i, j;
> +	struct pathgroup *pgp;
> +	struct path *pp;
> +	struct config *conf;
> +	bool mpp_has_wwid;
> +	bool must_reload = false;
> +
> +	if (!mpp->pg)
> +		return false;
> +
> +	mpp_has_wwid = guess_mpp_wwid(mpp);
> +
> +	vector_foreach_slot(mpp->pg, pgp, i) {
> +		if (!pgp->paths)
> +			goto delete_pg;
> +
> +		vector_foreach_slot(pgp->paths, pp, j) {
> +			pp->mpp = mpp;
> +
> +			if (pp->udev) {
> +				if (pathinfo_flags) {
> +					conf = get_multipath_config();
> +					pthread_cleanup_push(put_multipath_config,
> +							     conf);
> +					pathinfo(pp, conf, pathinfo_flags);
> +					pthread_cleanup_pop(1);
> +				}
> +				continue;

I suppose that if pp->udev is set then the path is in pathvec, but the
lack of check seemed odd to me. 

> +			}
> +
> +			/* If this fails, the device is not in sysfs */
> +			pp->udev = get_udev_device(pp->dev_t, DEV_DEVT);
> +			if (!pp->udev) {
> +				condlog(2, "%s: discarding non-existing path %s",
> +					mpp->alias, pp->dev_t);
> +				vector_del_slot(pgp->paths, j--);
> +				free_path(pp);
> +				must_reload = true;
> +			} else {
> +
> +				devt2devname(pp->dev, sizeof(pp->dev),
> +					     pp->dev_t);
> +				conf = get_multipath_config();
> +				pthread_cleanup_push(put_multipath_config,
> +						     conf);
> +				pp->checkint = conf->checkint;
> +				if (pathinfo(pp, conf,
> +					     DI_SYSFS|DI_WWID|pathinfo_flags)
> +				    != PATHINFO_OK)
> +					condlog(1, "%s: error in pathinfo",
> +						pp->dev);

This seems wrong to me.  A blacklisted path shouldn't be added to
pathvec.

> +				pthread_cleanup_pop(1);
> +				if (mpp_has_wwid && !strlen(pp->wwid)) {
> +					condlog(3, "%s: setting wwid from map: %s",
> +						pp->dev, mpp->wwid);
> +					strlcpy(pp->wwid, mpp->wwid,
> +						sizeof(pp->wwid));
> +				} else if (mpp_has_wwid &&
> +					   strcmp(mpp->wwid, pp->wwid)) {
> +
> +					condlog(0, "%s: path %s WWID %s doesn't match, removing from map",
> +						mpp->wwid, pp->dev_t, pp->wwid);
> +					/*
> +					 * This path exists, but in the wong map.
> +					 * We can't reload the map from here.
> +					 * Instead, treat this path like "missing udev",
> +					 * which it probably is.
> +					 * check_path() will trigger an uevent
> +					 * and reset pp->tick.
> +					 */
> +					must_reload = true;
> +					pp->mpp = NULL;
> +					dm_fail_path(mpp->alias, pp->dev_t);
> +					vector_del_slot(pgp->paths, j--);
> +					pp->initialized = INIT_MISSING_UDEV;
> +					pp->tick = 1;
> +				}
> +				condlog(2, "%s: adding new path %s",
> +					mpp->alias, pp->dev);
> +				store_path(pathvec, pp);
> +			}
> +		}
> +		if (VECTOR_SIZE(pgp->paths) != 0)
> +			continue;
> +	delete_pg:
> +		condlog(2, "%s: removing empty pathgroup %d", mpp->alias, i);
> +		vector_del_slot(mpp->pg, i--);
> +		free_pathgroup(pgp, KEEP_PATHS);

Should this flag the device for reloading, to remove the useless path
group?

> +	}
> +	return must_reload;
> +}
> +
>  int adopt_paths(vector pathvec, struct multipath *mpp)
>  {
>  	int i, ret;
> diff --git a/libmultipath/structs_vec.h b/libmultipath/structs_vec.h
> index cd3ef76..4c28148 100644
> --- a/libmultipath/structs_vec.h
> +++ b/libmultipath/structs_vec.h
> @@ -21,6 +21,8 @@ void orphan_path (struct path * pp, const char *reason);
>  void set_path_removed(struct path *pp);
>  
>  int verify_paths(struct multipath *mpp);
> +bool update_pathvec_from_dm(vector pathvec, struct multipath *mpp,
> +			    int pathinfo_flags);
>  int update_mpp_paths(struct multipath * mpp, vector pathvec);
>  int update_multipath_strings (struct multipath *mpp, vector pathvec,
>  			      int is_daemon);
> -- 
> 2.26.2

Re: [PATCH 65/74] libmultipath: add update_pathvec_from_dm()

[Martin Wilck]

On Sat, 2020-07-18 at 23:46 -0500, Benjamin Marzinski wrote:
> On Thu, Jul 09, 2020 at 12:51:36PM +0200, mwilck@suse.com wrote:
> > From: Martin Wilck <mwilck@suse.com>
> > 
> > It can happen in particular during boot or startup that we
> > encounter
> > paths as map members which haven't been discovered or fully
> > initialized
> > yet, and are thus not in the pathvec. These paths need special
> > treatment
> > in various ways. Currently this is dealt with in disassemble_map().
> > That's
> > a layer violation, and the way it is done is suboptimal in various
> > ways.
> > 
> > As a preparation to change that, this patch introduces a new
> > function,
> > update_pathvec_from_dm(), that is supposed to deal with newly
> > discovered
> > paths from disassemble_map(). It has to be called after
> > disassemble_map()
> > has finished.
> 
> I don't really understand the reason for adding paths to pathvec,
> just
> because they appear in disassemble_map(). If multipathd sees a map
> with
> a path that it doesn't have in pathvec, then that path should either
> sortly appear, or multipathd likely doesn't have it for a reason.  I
> agree that it's possible that uevent got missed, but it seems more
> likely that the user updated multipath.conf and hasn't reconfigured
> multipathd. 
> 
> Maybe when I read future patches this will make more sense, but can't
> multipathd add paths to the pathvec that it is configured to
> blacklist.
> I would like to think that multipathd is definitive for the multipath
> devices that it controls.  If multipathd doesn't think a path belongs
> in
> a multipath device, then the path doesn't belong in the multipath
> device. There are corner cases where a path can appear in a multipath
> device before multipathd gets notified about the path, but they all
> seem
> pretty rare. Someone running "multipath" before multipathd has seen
> the
> path addition uevent, for instance. Perhaps if pathinfo() should
> always
> set DI_BLACKLIST, and if the path is skipped, flag the device for
> reloading.

I agree wrt blacklisting. But for non-blacklisted paths that look like
valid map members, I think adding them to the pathvec makes sense.

The current upstream code doesn't add the code to pathvec; instead it
keeps paths in multipath maps that are not in pathvec. I tend to find
that inconsistent. I believe that pathvec should contain all paths that
multipathd is supposed to deal with in some manner (at least that's one
possible definition of it's semantics, see my reply to 69/74).

Note that multipathd used to add these paths to pathvec in the past,
but later stopped doing that to avoid removed paths being re-added (see
also my commit message for 'libmultipath: disassemble_map(): get rid of
"is_daemon" argument'). I've introduced INIT_REMOVED to deal with
that latter issue; thus we can now add all paths to pathvec
which appear to be valid multipath map members, even if they're not
fully initialized (yet).

update_pathvec_from_dm() is introduced as a single function to
incorporate all the related logic in one place, making it easy to
track.

To my understanding, the two main reasons why paths can be found in
maps that are not in pathvec (besides what you mentioned already) are:

 1) paths having been multipathed during initrd processing, but being 
    not fully initalized after switching root yet, because udev
    coldplug processing is delayed for some reason,
 2) paths that are removed but couldn't be deleted from maps because
    the respective device-mapper RELOAD calls failed.

2) should be mostly dealt with by the earlier INIT_REMOVED patches. In
general, the issues with path removal are mitigated a lot by your
kernel patch 86f1152b ("dm: allow active and inactive tables to share
dm_devs"), which eliminated the main reason for RELOAD ioctls to fail.
But multipath-tools supports older kernels that don't have this patch
yet (at least we never made a statement to the contrary).

Anyway, issue 1) remains, and cannot easily be excluded. Simply
removing such partially-initialized paths from maps is highly dangerous
IMO; maps might degrade and fail for no good reason, in particular
during startup / boot.

 
> > The logic of the new function is similar but not identical to what
> > disassemble_map() was doing before. Firstly, the function will
> > simply
> > remove path devices that don't exist - there's no point in carrying
> > these
> > around. Map reloads can't be called from this code for reasons of
> > the
> > overall program logic. But it's prepared to signal to the caller
> > that
> > a map reload is in order. Using this return value will be future
> > work.
> > 
> > Second, pathinfo() is now called on new paths rather than just
> > setting
> > pp->dev. The pathinfo flags can be passed as argument to make the
> > function
> > flexible for different callers.
> > 
> > Finally, treatment of WWIDs is different now. There'll be only one
> > attempt
> > at guessing the map WWID if it's not set yet. If a non-matching
> > path WWID
> > is found, the path is removed, and a new uevent triggered (this is
> > the
> > most important change wrt the dangerous previous code that would
> > simply
> > overwrite the path WWID). If the path WWID is empty, it will still
> > be
> > set from the map WWID, which I consider not perfect, but no worse
> > than what we did before.
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  libmultipath/structs_vec.c | 134
> > +++++++++++++++++++++++++++++++++++++
> >  libmultipath/structs_vec.h |   2 +
> >  2 files changed, 136 insertions(+)
> > 
> > diff --git a/libmultipath/structs_vec.c
> > b/libmultipath/structs_vec.c
> > index 1e0f109..5dd37d5 100644
> > --- a/libmultipath/structs_vec.c
> > +++ b/libmultipath/structs_vec.c
> > @@ -59,6 +59,140 @@ int update_mpp_paths(struct multipath *mpp,
> > vector pathvec)
> >  	return store_failure;
> >  }
> >  
> > +static bool guess_mpp_wwid(struct multipath *mpp)
> > +{
> > +	int i, j;
> > +	struct pathgroup *pgp;
> > +	struct path *pp;
> > +
> > +	if (strlen(mpp->wwid) || !mpp->pg)
> > +		return true;
> > +
> > +	vector_foreach_slot(mpp->pg, pgp, i) {
> > +		if (!pgp->paths)
> > +			continue;
> > +		vector_foreach_slot(pgp->paths, pp, j) {
> > +			if (pp->initialized == INIT_OK && strlen(pp-
> > >wwid)) {
> > +				strlcpy(mpp->wwid, pp->wwid,
> > sizeof(mpp->wwid));
> > +				condlog(2, "%s: guessed WWID %s from
> > path %s",
> > +					mpp->alias, mpp->wwid, pp-
> > >dev);
> > +				return true;
> > +			}
> > +		}
> > +	}
> > +	condlog(1, "%s: unable to guess WWID", mpp->alias);
> > +	return false;
> > +}
> > +
> > +/*
> > + * update_pathvec_from_dm() - update pathvec after
> > disassemble_map()
> > + *
> > + * disassemble_map() may return block devices that are members in
> > + * multipath maps but haven't been discovered. Check whether they
> > + * need to be added to pathvec or discarded.
> > + *
> > + * Returns: true if immediate map reload is desirable
> > + *
> > + * Side effects:
> > + * - may delete non-existing paths and empty pathgroups from mpp
> > + * - may set pp->wwid and / or mpp->wwid
> > + * - calls pathinfo() on existing paths is pathinfo_flags is not 0
> > + */
> > +bool update_pathvec_from_dm(vector pathvec, struct multipath *mpp,
> > +	int pathinfo_flags)
> > +{
> > +	int i, j;
> > +	struct pathgroup *pgp;
> > +	struct path *pp;
> > +	struct config *conf;
> > +	bool mpp_has_wwid;
> > +	bool must_reload = false;
> > +
> > +	if (!mpp->pg)
> > +		return false;
> > +
> > +	mpp_has_wwid = guess_mpp_wwid(mpp);
> > +
> > +	vector_foreach_slot(mpp->pg, pgp, i) {
> > +		if (!pgp->paths)
> > +			goto delete_pg;
> > +
> > +		vector_foreach_slot(pgp->paths, pp, j) {
> > +			pp->mpp = mpp;
> > +
> > +			if (pp->udev) {
> > +				if (pathinfo_flags) {
> > +					conf = get_multipath_config();
> > +					pthread_cleanup_push(put_multip
> > ath_config,
> > +							     conf);
> > +					pathinfo(pp, conf,
> > pathinfo_flags);
> > +					pthread_cleanup_pop(1);
> > +				}
> > +				continue;
> 
> I suppose that if pp->udev is set then the path is in pathvec, but
> the
> lack of check seemed odd to me. 

disassemble_map() looks up paths in pathvec before adding them to the
pg. If it does not find them in pathvec, it stores an empty path with
only pp->dev_t initialized in pgp->paths. Thus pp->udev can only be set
if the path had been found in pathvec.

I'll add a comment explaining that.

> 
> > +			}
> > +
> > +			/* If this fails, the device is not in sysfs */
> > +			pp->udev = get_udev_device(pp->dev_t,
> > DEV_DEVT);
> > +			if (!pp->udev) {
> > +				condlog(2, "%s: discarding non-existing 
> > path %s",
> > +					mpp->alias, pp->dev_t);
> > +				vector_del_slot(pgp->paths, j--);
> > +				free_path(pp);
> > +				must_reload = true;
> > +			} else {
> > +
> > +				devt2devname(pp->dev, sizeof(pp->dev),
> > +					     pp->dev_t);
> > +				conf = get_multipath_config();
> > +				pthread_cleanup_push(put_multipath_conf
> > ig,
> > +						     conf);
> > +				pp->checkint = conf->checkint;
> > +				if (pathinfo(pp, conf,
> > +					     DI_SYSFS|DI_WWID|pathinfo_
> > flags)
> > +				    != PATHINFO_OK)
> > +					condlog(1, "%s: error in
> > pathinfo",
> > +						pp->dev);
> 
> This seems wrong to me.  A blacklisted path shouldn't be added to
> pathvec.

Right, I should handle the error condition differently here, and I
should be using DI_BLACKLIST, too.

> > +				pthread_cleanup_pop(1);
> > +				if (mpp_has_wwid && !strlen(pp->wwid))
> > {
> > +					condlog(3, "%s: setting wwid
> > from map: %s",
> > +						pp->dev, mpp->wwid);
> > +					strlcpy(pp->wwid, mpp->wwid,
> > +						sizeof(pp->wwid));
> > +				} else if (mpp_has_wwid &&
> > +					   strcmp(mpp->wwid, pp->wwid)) 
> > {
> > +
> > +					condlog(0, "%s: path %s WWID %s
> > doesn't match, removing from map",
> > +						mpp->wwid, pp->dev_t,
> > pp->wwid);
> > +					/*
> > +					 * This path exists, but in the
> > wong map.
> > +					 * We can't reload the map from
> > here.
> > +					 * Instead, treat this path
> > like "missing udev",
> > +					 * which it probably is.
> > +					 * check_path() will trigger an
> > uevent
> > +					 * and reset pp->tick.
> > +					 */
> > +					must_reload = true;
> > +					pp->mpp = NULL;
> > +					dm_fail_path(mpp->alias, pp-
> > >dev_t);
> > +					vector_del_slot(pgp->paths, j
> > --);
> > +					pp->initialized =
> > INIT_MISSING_UDEV;
> > +					pp->tick = 1;
> > +				}
> > +				condlog(2, "%s: adding new path %s",
> > +					mpp->alias, pp->dev);
> > +				store_path(pathvec, pp);
> > +			}
> > +		}
> > +		if (VECTOR_SIZE(pgp->paths) != 0)
> > +			continue;
> > +	delete_pg:
> > +		condlog(2, "%s: removing empty pathgroup %d", mpp-
> > >alias, i);
> > +		vector_del_slot(mpp->pg, i--);
> > +		free_pathgroup(pgp, KEEP_PATHS);
> 
> Should this flag the device for reloading, to remove the useless path
> group?

Good point, thanks.

> 
> > +	}
> > +	return must_reload;
> > +}
> > +
> >  int adopt_paths(vector pathvec, struct multipath *mpp)
> >  {
> >  	int i, ret;
> > diff --git a/libmultipath/structs_vec.h
> > b/libmultipath/structs_vec.h
> > index cd3ef76..4c28148 100644
> > --- a/libmultipath/structs_vec.h
> > +++ b/libmultipath/structs_vec.h
> > @@ -21,6 +21,8 @@ void orphan_path (struct path * pp, const char
> > *reason);
> >  void set_path_removed(struct path *pp);
> >  
> >  int verify_paths(struct multipath *mpp);
> > +bool update_pathvec_from_dm(vector pathvec, struct multipath *mpp,
> > +			    int pathinfo_flags);
> >  int update_mpp_paths(struct multipath * mpp, vector pathvec);
> >  int update_multipath_strings (struct multipath *mpp, vector
> > pathvec,
> >  			      int is_daemon);
> > -- 
> > 2.26.2

[PATCH 66/74] libmultipath: update_pathvec_from_dm: handle pp->mpp mismatch

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Treat this like a WWID mismatch.

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

diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 5dd37d5..8651b98 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -118,6 +118,12 @@ bool update_pathvec_from_dm(vector pathvec, struct multipath *mpp,
 			goto delete_pg;
 
 		vector_foreach_slot(pgp->paths, pp, j) {
+
+			if (pp->mpp && pp->mpp != mpp) {
+				condlog(0, "BUG: %s: found path %s which is already in %s",
+					mpp->alias, pp->dev, pp->mpp->alias);
+				goto bad_path;
+			}
 			pp->mpp = mpp;
 
 			if (pp->udev) {
@@ -163,25 +169,28 @@ bool update_pathvec_from_dm(vector pathvec, struct multipath *mpp,
 
 					condlog(0, "%s: path %s WWID %s doesn't match, removing from map",
 						mpp->wwid, pp->dev_t, pp->wwid);
-					/*
-					 * This path exists, but in the wong map.
-					 * We can't reload the map from here.
-					 * Instead, treat this path like "missing udev",
-					 * which it probably is.
-					 * check_path() will trigger an uevent
-					 * and reset pp->tick.
-					 */
-					must_reload = true;
-					pp->mpp = NULL;
-					dm_fail_path(mpp->alias, pp->dev_t);
-					vector_del_slot(pgp->paths, j--);
-					pp->initialized = INIT_MISSING_UDEV;
-					pp->tick = 1;
+					goto bad_path;
 				}
 				condlog(2, "%s: adding new path %s",
 					mpp->alias, pp->dev);
 				store_path(pathvec, pp);
+
 			}
+			continue;
+
+		bad_path:
+			/*
+			 * This path exists, but in the wrong map.
+			 * We can't reload the map from here.
+			 * Instead, treat this path like "missing udev".
+			 * check_path() will trigger an uevent and reset pp->tick.
+			 */
+			must_reload = true;
+			pp->mpp = NULL;
+			dm_fail_path(mpp->alias, pp->dev_t);
+			vector_del_slot(pgp->paths, j--);
+			pp->initialized = INIT_MISSING_UDEV;
+			pp->tick = 1;
 		}
 		if (VECTOR_SIZE(pgp->paths) != 0)
 			continue;
-- 
2.26.2

Re: [PATCH 66/74] libmultipath: update_pathvec_from_dm: handle pp->mpp mismatch

[Benjamin Marzinski]

On Thu, Jul 09, 2020 at 12:51:37PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Treat this like a WWID mismatch.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/structs_vec.c | 37 +++++++++++++++++++++++--------------
>  1 file changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
> index 5dd37d5..8651b98 100644
> --- a/libmultipath/structs_vec.c
> +++ b/libmultipath/structs_vec.c
> @@ -118,6 +118,12 @@ bool update_pathvec_from_dm(vector pathvec, struct multipath *mpp,
>  			goto delete_pg;
>  
>  		vector_foreach_slot(pgp->paths, pp, j) {
> +
> +			if (pp->mpp && pp->mpp != mpp) {
> +				condlog(0, "BUG: %s: found path %s which is already in %s",
> +					mpp->alias, pp->dev, pp->mpp->alias);
> +				goto bad_path;
> +			}
>  			pp->mpp = mpp;
>  
>  			if (pp->udev) {
> @@ -163,25 +169,28 @@ bool update_pathvec_from_dm(vector pathvec, struct multipath *mpp,
>  
>  					condlog(0, "%s: path %s WWID %s doesn't match, removing from map",
>  						mpp->wwid, pp->dev_t, pp->wwid);
> -					/*
> -					 * This path exists, but in the wong map.
> -					 * We can't reload the map from here.
> -					 * Instead, treat this path like "missing udev",
> -					 * which it probably is.
> -					 * check_path() will trigger an uevent
> -					 * and reset pp->tick.
> -					 */
> -					must_reload = true;
> -					pp->mpp = NULL;
> -					dm_fail_path(mpp->alias, pp->dev_t);
> -					vector_del_slot(pgp->paths, j--);
> -					pp->initialized = INIT_MISSING_UDEV;
> -					pp->tick = 1;
> +					goto bad_path;
>  				}
>  				condlog(2, "%s: adding new path %s",
>  					mpp->alias, pp->dev);
>  				store_path(pathvec, pp);
> +
>  			}
> +			continue;
> +
> +		bad_path:
> +			/*
> +			 * This path exists, but in the wrong map.
> +			 * We can't reload the map from here.
> +			 * Instead, treat this path like "missing udev".
> +			 * check_path() will trigger an uevent and reset pp->tick.
> +			 */
> +			must_reload = true;
> +			pp->mpp = NULL;
> +			dm_fail_path(mpp->alias, pp->dev_t);
> +			vector_del_slot(pgp->paths, j--);
> +			pp->initialized = INIT_MISSING_UDEV;
> +			pp->tick = 1;

Is there a reason not to call orphan_path() to clean up things like any
open fd, until we figure out what to do with the path.

>  		}
>  		if (VECTOR_SIZE(pgp->paths) != 0)
>  			continue;
> -- 
> 2.26.2

Re: [PATCH 66/74] libmultipath: update_pathvec_from_dm: handle pp->mpp mismatch

[Martin Wilck]

On Sun, 2020-07-19 at 00:12 -0500, Benjamin Marzinski wrote:
> On Thu, Jul 09, 2020 at 12:51:37PM +0200, mwilck@suse.com wrote:
> > From: Martin Wilck <mwilck@suse.com>
> > 
> > Treat this like a WWID mismatch.
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  libmultipath/structs_vec.c | 37 +++++++++++++++++++++++-----------
> > ---
> >  1 file changed, 23 insertions(+), 14 deletions(-)
> > 
> > diff --git a/libmultipath/structs_vec.c
> > b/libmultipath/structs_vec.c
> > index 5dd37d5..8651b98 100644
> > --- a/libmultipath/structs_vec.c
> > +++ b/libmultipath/structs_vec.c
> > 
> > [...]
> > +		bad_path:
> > +			/*
> > +			 * This path exists, but in the wrong map.
> > +			 * We can't reload the map from here.
> > +			 * Instead, treat this path like "missing
> > udev".
> > +			 * check_path() will trigger an uevent and
> > reset pp->tick.
> > +			 */
> > +			must_reload = true;
> > +			pp->mpp = NULL;
> > +			dm_fail_path(mpp->alias, pp->dev_t);
> > +			vector_del_slot(pgp->paths, j--);
> > +			pp->initialized = INIT_MISSING_UDEV;
> > +			pp->tick = 1;
> 
> Is there a reason not to call orphan_path() to clean up things like
> any
> open fd, until we figure out what to do with the path.

Thanks for the suggestion. It makes sense for the 2nd "bad_path"
condition but not for the first. I'll treat the two cases differently.

Regards
Martin

[PATCH 67/74] libmultipath: disassemble_map(): always search paths by dev_t

[mwilck]

From: Martin Wilck <mwilck@suse.com>

There's no point in searching for the devname first. dev_t is the primary
device property for both device mapper and udev.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/dmparser.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
index 27581cd..7fcd76e 100644
--- a/libmultipath/dmparser.c
+++ b/libmultipath/dmparser.c
@@ -295,12 +295,8 @@ int disassemble_map(vector pathvec, char *params, struct multipath *mpp,
 				devname[0] = '\0';
 			}
 
-			if (pathvec) {
-				if (strlen(devname))
-					pp = find_path_by_dev(pathvec, devname);
-				else
-					pp = find_path_by_devt(pathvec, word);
-			}
+			if (pathvec)
+				pp = find_path_by_devt(pathvec, word);
 
 			if (!pp) {
 				pp = alloc_path();
-- 
2.26.2

[PATCH 68/74] libmultipath: disassemble_map(): require non-NULL pathvec

[mwilck]

From: Martin Wilck <mwilck@suse.com>

We would fail in store_path() at the latest if pathvec was NULL.
And all callers set pathvec anyway.

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

diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
index 7fcd76e..e6f2cbe 100644
--- a/libmultipath/dmparser.c
+++ b/libmultipath/dmparser.c
@@ -6,6 +6,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <assert.h>
 
 #include "checkers.h"
 #include "vector.h"
@@ -137,6 +138,7 @@ int disassemble_map(vector pathvec, char *params, struct multipath *mpp,
 	struct path * pp;
 	struct pathgroup * pgp;
 
+	assert(pathvec != NULL);
 	p = params;
 
 	condlog(4, "%s: disassemble map [%s]", mpp->alias, params);
@@ -295,8 +297,7 @@ int disassemble_map(vector pathvec, char *params, struct multipath *mpp,
 				devname[0] = '\0';
 			}
 
-			if (pathvec)
-				pp = find_path_by_devt(pathvec, word);
+			pp = find_path_by_devt(pathvec, word);
 
 			if (!pp) {
 				pp = alloc_path();
-- 
2.26.2

[PATCH 69/74] libmultipath: disassemble_map(): get rid of "is_daemon" argument

[mwilck]

From: Martin Wilck <mwilck@suse.com>

The reason for the is_daemon parameter in disassemble_map() lies
deep in multipath-tools' past, in b96dead ("[multipathd] remove the
retry login in uev_remove_path()"): By not adding paths from
disassembled maps to the pathvec, we avoided to re-add removed paths on
future map reloads (logic in update_mpp_paths()). As we can handle this with
INIT_REMOVED now, we don't need to distinguish daemon and non-daemon
mode any more. This fixes a memory leak, because only paths which are in
pathvec will be freed on program exit.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmpathpersist/mpath_persist.c | 2 +-
 libmultipath/dmparser.c         | 6 ++----
 libmultipath/dmparser.h         | 2 +-
 libmultipath/structs_vec.c      | 9 ++++-----
 libmultipath/structs_vec.h      | 6 ++----
 multipath/main.c                | 4 ++--
 multipathd/main.c               | 8 ++++----
 7 files changed, 16 insertions(+), 21 deletions(-)

diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c
index 3da7a6c..cb3182f 100644
--- a/libmpathpersist/mpath_persist.c
+++ b/libmpathpersist/mpath_persist.c
@@ -391,7 +391,7 @@ get_mpvec (vector curmp, vector pathvec, char * refwwid)
 		condlog(3, "params = %s", params);
 		dm_get_status(mpp->alias, status);
 		condlog(3, "status = %s", status);
-		disassemble_map (pathvec, params, mpp, 0);
+		disassemble_map (pathvec, params, mpp);
 
 		/*
 		 * disassemble_map() can add new paths to pathvec.
diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
index e6f2cbe..233a1b8 100644
--- a/libmultipath/dmparser.c
+++ b/libmultipath/dmparser.c
@@ -122,8 +122,7 @@ err:
 
 #undef APPEND
 
-int disassemble_map(vector pathvec, char *params, struct multipath *mpp,
-		    int is_daemon)
+int disassemble_map(vector pathvec, char *params, struct multipath *mpp)
 {
 	char * word;
 	char * p;
@@ -311,8 +310,7 @@ int disassemble_map(vector pathvec, char *params, struct multipath *mpp,
 					strlcpy(pp->wwid, mpp->wwid,
 						WWID_SIZE);
 				}
-				/* Only call this in multipath client mode */
-				if (!is_daemon && store_path(pathvec, pp))
+				if (store_path(pathvec, pp))
 					goto out1;
 			} else {
 				if (!strlen(pp->wwid) &&
diff --git a/libmultipath/dmparser.h b/libmultipath/dmparser.h
index e1badb0..1b45df0 100644
--- a/libmultipath/dmparser.h
+++ b/libmultipath/dmparser.h
@@ -1,3 +1,3 @@
 int assemble_map (struct multipath *, char *, int);
-int disassemble_map (vector, char *, struct multipath *, int);
+int disassemble_map (vector, char *, struct multipath *);
 int disassemble_status (char *, struct multipath *);
diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 8651b98..73a7221 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -370,7 +370,7 @@ extract_hwe_from_path(struct multipath * mpp)
 }
 
 int
-update_multipath_table (struct multipath *mpp, vector pathvec, int is_daemon)
+update_multipath_table (struct multipath *mpp, vector pathvec)
 {
 	int r = DMP_ERR;
 	char params[PARAMS_SIZE] = {0};
@@ -384,7 +384,7 @@ update_multipath_table (struct multipath *mpp, vector pathvec, int is_daemon)
 		return r;
 	}
 
-	if (disassemble_map(pathvec, params, mpp, is_daemon)) {
+	if (disassemble_map(pathvec, params, mpp)) {
 		condlog(3, "%s: cannot disassemble map", mpp->alias);
 		return DMP_ERR;
 	}
@@ -474,7 +474,7 @@ void sync_paths(struct multipath *mpp, vector pathvec)
 }
 
 int
-update_multipath_strings(struct multipath *mpp, vector pathvec, int is_daemon)
+update_multipath_strings(struct multipath *mpp, vector pathvec)
 {
 	struct pathgroup *pgp;
 	int i, r = DMP_ERR;
@@ -489,10 +489,9 @@ update_multipath_strings(struct multipath *mpp, vector pathvec, int is_daemon)
 	free_pgvec(mpp->pg, KEEP_PATHS);
 	mpp->pg = NULL;
 
-	r = update_multipath_table(mpp, pathvec, is_daemon);
+	r = update_multipath_table(mpp, pathvec);
 	if (r != DMP_OK)
 		return r;
-
 	sync_paths(mpp, pathvec);
 
 	r = update_multipath_status(mpp);
diff --git a/libmultipath/structs_vec.h b/libmultipath/structs_vec.h
index 4c28148..32cad60 100644
--- a/libmultipath/structs_vec.h
+++ b/libmultipath/structs_vec.h
@@ -24,8 +24,7 @@ int verify_paths(struct multipath *mpp);
 bool update_pathvec_from_dm(vector pathvec, struct multipath *mpp,
 			    int pathinfo_flags);
 int update_mpp_paths(struct multipath * mpp, vector pathvec);
-int update_multipath_strings (struct multipath *mpp, vector pathvec,
-			      int is_daemon);
+int update_multipath_strings (struct multipath *mpp, vector pathvec);
 void extract_hwe_from_path(struct multipath * mpp);
 
 #define PURGE_VEC 1
@@ -41,8 +40,7 @@ struct multipath * add_map_with_path (struct vectors * vecs,
 				struct path * pp, int add_vec);
 void update_queue_mode_del_path(struct multipath *mpp);
 void update_queue_mode_add_path(struct multipath *mpp);
-int update_multipath_table (struct multipath *mpp, vector pathvec,
-			    int is_daemon);
+int update_multipath_table (struct multipath *mpp, vector pathvec);
 int update_multipath_status (struct multipath *mpp);
 vector get_used_hwes(const struct _vector *pathvec);
 
diff --git a/multipath/main.c b/multipath/main.c
index cfb85dc..8a2a6f7 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -272,7 +272,7 @@ get_dm_mpvec (enum mpath_cmds cmd, vector curmp, vector pathvec, char * refwwid)
 		dm_get_status(mpp->alias, status);
 		condlog(3, "status = %s", status);
 
-		disassemble_map(pathvec, params, mpp, 0);
+		disassemble_map(pathvec, params, mpp);
 
 		/*
 		 * disassemble_map() can add new paths to pathvec.
@@ -352,7 +352,7 @@ static int check_usable_paths(struct config *conf,
 
 	dm_get_map(mpp->alias, &mpp->size, params);
 	dm_get_status(mpp->alias, status);
-	disassemble_map(pathvec, params, mpp, 0);
+	disassemble_map(pathvec, params, mpp);
 	disassemble_status(status, mpp);
 
 	vector_foreach_slot (mpp->pg, pg, i) {
diff --git a/multipathd/main.c b/multipathd/main.c
index 0cd0ee6..66ca4e3 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -409,7 +409,7 @@ int __setup_multipath(struct vectors *vecs, struct multipath *mpp,
 		goto out;
 	}
 
-	if (update_multipath_strings(mpp, vecs->pathvec, 1) != DMP_OK) {
+	if (update_multipath_strings(mpp, vecs->pathvec) != DMP_OK) {
 		condlog(0, "%s: failed to setup multipath", mpp->alias);
 		goto out;
 	}
@@ -551,7 +551,7 @@ add_map_without_path (struct vectors *vecs, const char *alias)
 	mpp->mpe = find_mpe(conf->mptable, mpp->wwid);
 	put_multipath_config(conf);
 
-	if (update_multipath_table(mpp, vecs->pathvec, 1) != DMP_OK)
+	if (update_multipath_table(mpp, vecs->pathvec) != DMP_OK)
 		goto out;
 	if (update_multipath_status(mpp) != DMP_OK)
 		goto out;
@@ -1410,7 +1410,7 @@ map_discovery (struct vectors * vecs)
 		return 1;
 
 	vector_foreach_slot (vecs->mpvec, mpp, i)
-		if (update_multipath_table(mpp, vecs->pathvec, 1) != DMP_OK ||
+		if (update_multipath_table(mpp, vecs->pathvec) != DMP_OK ||
 		    update_multipath_status(mpp) != DMP_OK) {
 			remove_map(mpp, vecs, 1);
 			i--;
@@ -2153,7 +2153,7 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 	/*
 	 * Synchronize with kernel state
 	 */
-	ret = update_multipath_strings(pp->mpp, vecs->pathvec, 1);
+	ret = update_multipath_strings(pp->mpp, vecs->pathvec);
 	if (ret != DMP_OK) {
 		if (ret == DMP_NOT_FOUND) {
 			/* multipath device missing. Likely removed */
-- 
2.26.2

Re: [PATCH 69/74] libmultipath: disassemble_map(): get rid of "is_daemon" argument

[Benjamin Marzinski]

On Thu, Jul 09, 2020 at 12:51:40PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> The reason for the is_daemon parameter in disassemble_map() lies
> deep in multipath-tools' past, in b96dead ("[multipathd] remove the
> retry login in uev_remove_path()"): By not adding paths from
> disassembled maps to the pathvec, we avoided to re-add removed paths on
> future map reloads (logic in update_mpp_paths()). As we can handle this with
> INIT_REMOVED now, we don't need to distinguish daemon and non-daemon
> mode any more. This fixes a memory leak, because only paths which are in
> pathvec will be freed on program exit.

I don't have any problems with the code in this patch. I just want to
reiterate that I don't think that multipathd should automatically be
adding paths, just because they were in a multipath device.  In my
opinion, multipathd should have the final decision as to what a
multipath device should look like.  If it sees an unexpected path, and
runs pathinfo on it, and finds that that path does belong, it's fine to
add it. But if pathinfo says that the path doesn't belong, multipathd
shouldn't add it to the pathvec. It should instead trigger a reload of
the device to remove path.

-Ben

 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmpathpersist/mpath_persist.c | 2 +-
>  libmultipath/dmparser.c         | 6 ++----
>  libmultipath/dmparser.h         | 2 +-
>  libmultipath/structs_vec.c      | 9 ++++-----
>  libmultipath/structs_vec.h      | 6 ++----
>  multipath/main.c                | 4 ++--
>  multipathd/main.c               | 8 ++++----
>  7 files changed, 16 insertions(+), 21 deletions(-)
> 
> diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c
> index 3da7a6c..cb3182f 100644
> --- a/libmpathpersist/mpath_persist.c
> +++ b/libmpathpersist/mpath_persist.c
> @@ -391,7 +391,7 @@ get_mpvec (vector curmp, vector pathvec, char * refwwid)
>  		condlog(3, "params = %s", params);
>  		dm_get_status(mpp->alias, status);
>  		condlog(3, "status = %s", status);
> -		disassemble_map (pathvec, params, mpp, 0);
> +		disassemble_map (pathvec, params, mpp);
>  
>  		/*
>  		 * disassemble_map() can add new paths to pathvec.
> diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
> index e6f2cbe..233a1b8 100644
> --- a/libmultipath/dmparser.c
> +++ b/libmultipath/dmparser.c
> @@ -122,8 +122,7 @@ err:
>  
>  #undef APPEND
>  
> -int disassemble_map(vector pathvec, char *params, struct multipath *mpp,
> -		    int is_daemon)
> +int disassemble_map(vector pathvec, char *params, struct multipath *mpp)
>  {
>  	char * word;
>  	char * p;
> @@ -311,8 +310,7 @@ int disassemble_map(vector pathvec, char *params, struct multipath *mpp,
>  					strlcpy(pp->wwid, mpp->wwid,
>  						WWID_SIZE);
>  				}
> -				/* Only call this in multipath client mode */
> -				if (!is_daemon && store_path(pathvec, pp))
> +				if (store_path(pathvec, pp))
>  					goto out1;
>  			} else {
>  				if (!strlen(pp->wwid) &&
> diff --git a/libmultipath/dmparser.h b/libmultipath/dmparser.h
> index e1badb0..1b45df0 100644
> --- a/libmultipath/dmparser.h
> +++ b/libmultipath/dmparser.h
> @@ -1,3 +1,3 @@
>  int assemble_map (struct multipath *, char *, int);
> -int disassemble_map (vector, char *, struct multipath *, int);
> +int disassemble_map (vector, char *, struct multipath *);
>  int disassemble_status (char *, struct multipath *);
> diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
> index 8651b98..73a7221 100644
> --- a/libmultipath/structs_vec.c
> +++ b/libmultipath/structs_vec.c
> @@ -370,7 +370,7 @@ extract_hwe_from_path(struct multipath * mpp)
>  }
>  
>  int
> -update_multipath_table (struct multipath *mpp, vector pathvec, int is_daemon)
> +update_multipath_table (struct multipath *mpp, vector pathvec)
>  {
>  	int r = DMP_ERR;
>  	char params[PARAMS_SIZE] = {0};
> @@ -384,7 +384,7 @@ update_multipath_table (struct multipath *mpp, vector pathvec, int is_daemon)
>  		return r;
>  	}
>  
> -	if (disassemble_map(pathvec, params, mpp, is_daemon)) {
> +	if (disassemble_map(pathvec, params, mpp)) {
>  		condlog(3, "%s: cannot disassemble map", mpp->alias);
>  		return DMP_ERR;
>  	}
> @@ -474,7 +474,7 @@ void sync_paths(struct multipath *mpp, vector pathvec)
>  }
>  
>  int
> -update_multipath_strings(struct multipath *mpp, vector pathvec, int is_daemon)
> +update_multipath_strings(struct multipath *mpp, vector pathvec)
>  {
>  	struct pathgroup *pgp;
>  	int i, r = DMP_ERR;
> @@ -489,10 +489,9 @@ update_multipath_strings(struct multipath *mpp, vector pathvec, int is_daemon)
>  	free_pgvec(mpp->pg, KEEP_PATHS);
>  	mpp->pg = NULL;
>  
> -	r = update_multipath_table(mpp, pathvec, is_daemon);
> +	r = update_multipath_table(mpp, pathvec);
>  	if (r != DMP_OK)
>  		return r;
> -
>  	sync_paths(mpp, pathvec);
>  
>  	r = update_multipath_status(mpp);
> diff --git a/libmultipath/structs_vec.h b/libmultipath/structs_vec.h
> index 4c28148..32cad60 100644
> --- a/libmultipath/structs_vec.h
> +++ b/libmultipath/structs_vec.h
> @@ -24,8 +24,7 @@ int verify_paths(struct multipath *mpp);
>  bool update_pathvec_from_dm(vector pathvec, struct multipath *mpp,
>  			    int pathinfo_flags);
>  int update_mpp_paths(struct multipath * mpp, vector pathvec);
> -int update_multipath_strings (struct multipath *mpp, vector pathvec,
> -			      int is_daemon);
> +int update_multipath_strings (struct multipath *mpp, vector pathvec);
>  void extract_hwe_from_path(struct multipath * mpp);
>  
>  #define PURGE_VEC 1
> @@ -41,8 +40,7 @@ struct multipath * add_map_with_path (struct vectors * vecs,
>  				struct path * pp, int add_vec);
>  void update_queue_mode_del_path(struct multipath *mpp);
>  void update_queue_mode_add_path(struct multipath *mpp);
> -int update_multipath_table (struct multipath *mpp, vector pathvec,
> -			    int is_daemon);
> +int update_multipath_table (struct multipath *mpp, vector pathvec);
>  int update_multipath_status (struct multipath *mpp);
>  vector get_used_hwes(const struct _vector *pathvec);
>  
> diff --git a/multipath/main.c b/multipath/main.c
> index cfb85dc..8a2a6f7 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -272,7 +272,7 @@ get_dm_mpvec (enum mpath_cmds cmd, vector curmp, vector pathvec, char * refwwid)
>  		dm_get_status(mpp->alias, status);
>  		condlog(3, "status = %s", status);
>  
> -		disassemble_map(pathvec, params, mpp, 0);
> +		disassemble_map(pathvec, params, mpp);
>  
>  		/*
>  		 * disassemble_map() can add new paths to pathvec.
> @@ -352,7 +352,7 @@ static int check_usable_paths(struct config *conf,
>  
>  	dm_get_map(mpp->alias, &mpp->size, params);
>  	dm_get_status(mpp->alias, status);
> -	disassemble_map(pathvec, params, mpp, 0);
> +	disassemble_map(pathvec, params, mpp);
>  	disassemble_status(status, mpp);
>  
>  	vector_foreach_slot (mpp->pg, pg, i) {
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 0cd0ee6..66ca4e3 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -409,7 +409,7 @@ int __setup_multipath(struct vectors *vecs, struct multipath *mpp,
>  		goto out;
>  	}
>  
> -	if (update_multipath_strings(mpp, vecs->pathvec, 1) != DMP_OK) {
> +	if (update_multipath_strings(mpp, vecs->pathvec) != DMP_OK) {
>  		condlog(0, "%s: failed to setup multipath", mpp->alias);
>  		goto out;
>  	}
> @@ -551,7 +551,7 @@ add_map_without_path (struct vectors *vecs, const char *alias)
>  	mpp->mpe = find_mpe(conf->mptable, mpp->wwid);
>  	put_multipath_config(conf);
>  
> -	if (update_multipath_table(mpp, vecs->pathvec, 1) != DMP_OK)
> +	if (update_multipath_table(mpp, vecs->pathvec) != DMP_OK)
>  		goto out;
>  	if (update_multipath_status(mpp) != DMP_OK)
>  		goto out;
> @@ -1410,7 +1410,7 @@ map_discovery (struct vectors * vecs)
>  		return 1;
>  
>  	vector_foreach_slot (vecs->mpvec, mpp, i)
> -		if (update_multipath_table(mpp, vecs->pathvec, 1) != DMP_OK ||
> +		if (update_multipath_table(mpp, vecs->pathvec) != DMP_OK ||
>  		    update_multipath_status(mpp) != DMP_OK) {
>  			remove_map(mpp, vecs, 1);
>  			i--;
> @@ -2153,7 +2153,7 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
>  	/*
>  	 * Synchronize with kernel state
>  	 */
> -	ret = update_multipath_strings(pp->mpp, vecs->pathvec, 1);
> +	ret = update_multipath_strings(pp->mpp, vecs->pathvec);
>  	if (ret != DMP_OK) {
>  		if (ret == DMP_NOT_FOUND) {
>  			/* multipath device missing. Likely removed */
> -- 
> 2.26.2

Re: [PATCH 69/74] libmultipath: disassemble_map(): get rid of "is_daemon" argument

[Martin Wilck]

On Sun, 2020-07-19 at 00:26 -0500, Benjamin Marzinski wrote:
> On Thu, Jul 09, 2020 at 12:51:40PM +0200, mwilck@suse.com wrote:
> > From: Martin Wilck <mwilck@suse.com>
> > 
> > The reason for the is_daemon parameter in disassemble_map() lies
> > deep in multipath-tools' past, in b96dead ("[multipathd] remove the
> > retry login in uev_remove_path()"): By not adding paths from
> > disassembled maps to the pathvec, we avoided to re-add removed
> > paths on
> > future map reloads (logic in update_mpp_paths()). As we can handle
> > this with
> > INIT_REMOVED now, we don't need to distinguish daemon and non-
> > daemon
> > mode any more. This fixes a memory leak, because only paths which
> > are in
> > pathvec will be freed on program exit.
> 
> I don't have any problems with the code in this patch. I just want to
> reiterate that I don't think that multipathd should automatically be
> adding paths, just because they were in a multipath device.  In my
> opinion, multipathd should have the final decision as to what a
> multipath device should look like.  If it sees an unexpected path,
> and
> runs pathinfo on it, and finds that that path does belong, it's fine
> to
> add it. But if pathinfo says that the path doesn't belong, multipathd
> shouldn't add it to the pathvec. It should instead trigger a reload
> of
> the device to remove path.

Got it. I commented in my reply to 65/74. I'll repost this part with
the minor issues you raised fixed (hopefully). Then let's review /
discuss this again. If we agree on your PoV, we can probably ditch the
whole INIT_REMOVED part of my series.

I hope you agree that "if (!is_daemon)" so deep in libmultipath
is ugly and should be replaced by something cleaner.

We should take the opportunity to agree on a definition on the exact
semantics of pathvec, i.e. which devices should be members of pathvec,
and which ones shouldn't. I don't see a clear, consequent definition
currently.

Martin

Re: [PATCH 69/74] libmultipath: disassemble_map(): get rid of "is_daemon" argument

[Benjamin Marzinski]

On Wed, Aug 05, 2020 at 10:05:19PM +0200, Martin Wilck wrote:
> On Sun, 2020-07-19 at 00:26 -0500, Benjamin Marzinski wrote:
> > On Thu, Jul 09, 2020 at 12:51:40PM +0200, mwilck@suse.com wrote:
> > > From: Martin Wilck <mwilck@suse.com>
> > > 
> > > The reason for the is_daemon parameter in disassemble_map() lies
> > > deep in multipath-tools' past, in b96dead ("[multipathd] remove the
> > > retry login in uev_remove_path()"): By not adding paths from
> > > disassembled maps to the pathvec, we avoided to re-add removed
> > > paths on
> > > future map reloads (logic in update_mpp_paths()). As we can handle
> > > this with
> > > INIT_REMOVED now, we don't need to distinguish daemon and non-
> > > daemon
> > > mode any more. This fixes a memory leak, because only paths which
> > > are in
> > > pathvec will be freed on program exit.
> > 
> > I don't have any problems with the code in this patch. I just want to
> > reiterate that I don't think that multipathd should automatically be
> > adding paths, just because they were in a multipath device.  In my
> > opinion, multipathd should have the final decision as to what a
> > multipath device should look like.  If it sees an unexpected path,
> > and
> > runs pathinfo on it, and finds that that path does belong, it's fine
> > to
> > add it. But if pathinfo says that the path doesn't belong, multipathd
> > shouldn't add it to the pathvec. It should instead trigger a reload
> > of
> > the device to remove path.
> 
> Got it. I commented in my reply to 65/74. I'll repost this part with
> the minor issues you raised fixed (hopefully). Then let's review /
> discuss this again. If we agree on your PoV, we can probably ditch the
> whole INIT_REMOVED part of my series.

Sure.

> I hope you agree that "if (!is_daemon)" so deep in libmultipath
> is ugly and should be replaced by something cleaner.

That really never bothered me, but I see your point.
 
> We should take the opportunity to agree on a definition on the exact
> semantics of pathvec, i.e. which devices should be members of pathvec,
> and which ones shouldn't. I don't see a clear, consequent definition
> currently.

I think that you're correct that all paths that multipathd is dealing
with should be on the pathvec. Obviously paths that are only accessable
via a local variable are fine, but code shouldn't be dropping the vecs
lock with a path that is accessable globally (for instance from
mpp->paths) but not on the pathvec. What disassemble_map() has been
doing is definitely wrong.

-Ben

> Martin
> 

[PATCH 70/74] libmultipath: disassemble_map(): do not change pathvec and WWIDs

[mwilck]

From: Martin Wilck <mwilck@suse.com>

After introducing update_pathvec_from_dm() in a predecessor patch,
the "layer violations" in disassemble_map() can now be removed.
I hope this clarifies program logic a little bit.

Callers need to call update_pathvec_from_dm() after disassemble_map().

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/dmparser.c    | 48 +++++---------------------------------
 libmultipath/structs_vec.c |  3 +++
 2 files changed, 9 insertions(+), 42 deletions(-)

diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
index 233a1b8..e629a89 100644
--- a/libmultipath/dmparser.c
+++ b/libmultipath/dmparser.c
@@ -122,6 +122,12 @@ err:
 
 #undef APPEND
 
+/*
+ * Caution callers: If this function encounters yet unkown path devices, it
+ * adds them uninitialized to the mpp.
+ * Call update_pathvec_from_dm() after this function to make sure
+ * all data structures are in a sane state.
+ */
 int disassemble_map(vector pathvec, char *params, struct multipath *mpp)
 {
 	char * word;
@@ -282,20 +288,12 @@ int disassemble_map(vector pathvec, char *params, struct multipath *mpp)
 		FREE(word);
 
 		for (j = 0; j < num_paths; j++) {
-			char devname[FILE_NAME_SIZE];
-
 			pp = NULL;
 			p += get_word(p, &word);
 
 			if (!word)
 				goto out;
 
-			if (devt2devname(devname, FILE_NAME_SIZE, word)) {
-				condlog(2, "%s: cannot find block device",
-					word);
-				devname[0] = '\0';
-			}
-
 			pp = find_path_by_devt(pathvec, word);
 
 			if (!pp) {
@@ -305,46 +303,12 @@ int disassemble_map(vector pathvec, char *params, struct multipath *mpp)
 					goto out1;
 
 				strlcpy(pp->dev_t, word, BLK_DEV_SIZE);
-				strlcpy(pp->dev, devname, FILE_NAME_SIZE);
-				if (strlen(mpp->wwid)) {
-					strlcpy(pp->wwid, mpp->wwid,
-						WWID_SIZE);
-				}
-				if (store_path(pathvec, pp))
-					goto out1;
-			} else {
-				if (!strlen(pp->wwid) &&
-				    strlen(mpp->wwid))
-					strlcpy(pp->wwid, mpp->wwid,
-						WWID_SIZE);
 			}
 			FREE(word);
 
 			if (store_path(pgp->paths, pp))
 				goto out;
 
-			/*
-			 * Update wwid for multipaths which are not setup
-			 * in the get_dm_mpvec() code path
-			 */
-			if (!strlen(mpp->wwid))
-				strlcpy(mpp->wwid, pp->wwid, WWID_SIZE);
-
-			/*
-			 * Update wwid for paths which may not have been
-			 * active at the time the getuid callout was run
-			 */
-			else if (!strlen(pp->wwid))
-				strlcpy(pp->wwid, mpp->wwid, WWID_SIZE);
-
-			/*
-			 * Do not allow in-use patch to change wwid
-			 */
-			else if (strcmp(pp->wwid, mpp->wwid) != 0) {
-				condlog(0, "%s: path wwid appears to have changed. Using map wwid.\n", pp->dev_t);
-				strlcpy(pp->wwid, mpp->wwid, WWID_SIZE);
-			}
-
 			pgp->id ^= (long)pp;
 			pp->pgindex = i + 1;
 
diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 73a7221..2ddb8cd 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -389,6 +389,9 @@ update_multipath_table (struct multipath *mpp, vector pathvec)
 		return DMP_ERR;
 	}
 
+	/* FIXME: we should deal with the return value here */
+	update_pathvec_from_dm(pathvec, mpp, 0);
+
 	return DMP_OK;
 }
 
-- 
2.26.2

[PATCH 71/74] multipath: use update_pathvec_from_dm()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

The multipath-specific function update_paths() can now be replaced with
a call to update_pathvec_from_dm().

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipath/main.c | 67 +++---------------------------------------------
 1 file changed, 3 insertions(+), 64 deletions(-)

diff --git a/multipath/main.c b/multipath/main.c
index 8a2a6f7..435c5d5 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -187,63 +187,6 @@ usage (char * progname)
 
 }
 
-static int
-update_paths (struct multipath * mpp, int quick)
-{
-	int i, j;
-	struct pathgroup * pgp;
-	struct path * pp;
-	struct config *conf;
-
-	if (!mpp->pg)
-		return 0;
-
-	vector_foreach_slot (mpp->pg, pgp, i) {
-		if (!pgp->paths)
-			continue;
-
-		vector_foreach_slot (pgp->paths, pp, j) {
-			if (!strlen(pp->dev)) {
-				if (devt2devname(pp->dev, FILE_NAME_SIZE,
-						 pp->dev_t)) {
-					/*
-					 * path is not in sysfs anymore
-					 */
-					pp->chkrstate = pp->state = PATH_DOWN;
-					pp->offline = 1;
-					continue;
-				}
-				pp->mpp = mpp;
-				if (quick)
-					continue;
-				conf = get_multipath_config();
-				if (pathinfo(pp, conf, DI_ALL))
-					pp->state = PATH_UNCHECKED;
-				put_multipath_config(conf);
-				continue;
-			}
-			pp->mpp = mpp;
-			if (quick)
-				continue;
-			if (pp->state == PATH_UNCHECKED ||
-			    pp->state == PATH_WILD) {
-				conf = get_multipath_config();
-				if (pathinfo(pp, conf, DI_CHECKER))
-					pp->state = PATH_UNCHECKED;
-				put_multipath_config(conf);
-			}
-
-			if (pp->priority == PRIO_UNDEF) {
-				conf = get_multipath_config();
-				if (pathinfo(pp, conf, DI_PRIO))
-					pp->priority = PRIO_UNDEF;
-				put_multipath_config(conf);
-			}
-		}
-	}
-	return 0;
-}
-
 static int
 get_dm_mpvec (enum mpath_cmds cmd, vector curmp, vector pathvec, char * refwwid)
 {
@@ -273,13 +216,8 @@ get_dm_mpvec (enum mpath_cmds cmd, vector curmp, vector pathvec, char * refwwid)
 		condlog(3, "status = %s", status);
 
 		disassemble_map(pathvec, params, mpp);
-
-		/*
-		 * disassemble_map() can add new paths to pathvec.
-		 * If not in "fast list mode", we need to fetch information
-		 * about them
-		 */
-		update_paths(mpp, (cmd == CMD_LIST_SHORT));
+		update_pathvec_from_dm(pathvec, mpp,
+				       (cmd == CMD_LIST_SHORT ? 0 : DI_ALL));
 
 		if (cmd == CMD_LIST_LONG)
 			mpp->bestpg = select_path_group(mpp);
@@ -353,6 +291,7 @@ static int check_usable_paths(struct config *conf,
 	dm_get_map(mpp->alias, &mpp->size, params);
 	dm_get_status(mpp->alias, status);
 	disassemble_map(pathvec, params, mpp);
+	update_pathvec_from_dm(pathvec, mpp, 0);
 	disassemble_status(status, mpp);
 
 	vector_foreach_slot (mpp->pg, pg, i) {
-- 
2.26.2

Re: [PATCH 71/74] multipath: use update_pathvec_from_dm()

[Benjamin Marzinski]

On Thu, Jul 09, 2020 at 12:51:42PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> The multipath-specific function update_paths() can now be replaced with
> a call to update_pathvec_from_dm().
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipath/main.c | 67 +++---------------------------------------------
>  1 file changed, 3 insertions(+), 64 deletions(-)
> 
> diff --git a/multipath/main.c b/multipath/main.c
> index 8a2a6f7..435c5d5 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -187,63 +187,6 @@ usage (char * progname)
>  
>  }
>  
> -static int
> -update_paths (struct multipath * mpp, int quick)
> -{
> -	int i, j;
> -	struct pathgroup * pgp;
> -	struct path * pp;
> -	struct config *conf;
> -
> -	if (!mpp->pg)
> -		return 0;
> -
> -	vector_foreach_slot (mpp->pg, pgp, i) {
> -		if (!pgp->paths)
> -			continue;
> -
> -		vector_foreach_slot (pgp->paths, pp, j) {
> -			if (!strlen(pp->dev)) {
> -				if (devt2devname(pp->dev, FILE_NAME_SIZE,
> -						 pp->dev_t)) {
> -					/*
> -					 * path is not in sysfs anymore
> -					 */
> -					pp->chkrstate = pp->state = PATH_DOWN;
> -					pp->offline = 1;
> -					continue;
> -				}
> -				pp->mpp = mpp;
> -				if (quick)
> -					continue;
> -				conf = get_multipath_config();
> -				if (pathinfo(pp, conf, DI_ALL))
> -					pp->state = PATH_UNCHECKED;
> -				put_multipath_config(conf);
> -				continue;
> -			}
> -			pp->mpp = mpp;
> -			if (quick)
> -				continue;
> -			if (pp->state == PATH_UNCHECKED ||
> -			    pp->state == PATH_WILD) {
> -				conf = get_multipath_config();
> -				if (pathinfo(pp, conf, DI_CHECKER))
> -					pp->state = PATH_UNCHECKED;
> -				put_multipath_config(conf);
> -			}
> -
> -			if (pp->priority == PRIO_UNDEF) {
> -				conf = get_multipath_config();
> -				if (pathinfo(pp, conf, DI_PRIO))
> -					pp->priority = PRIO_UNDEF;
> -				put_multipath_config(conf);
> -			}
> -		}
> -	}
> -	return 0;
> -}
> -
>  static int
>  get_dm_mpvec (enum mpath_cmds cmd, vector curmp, vector pathvec, char * refwwid)
>  {
> @@ -273,13 +216,8 @@ get_dm_mpvec (enum mpath_cmds cmd, vector curmp, vector pathvec, char * refwwid)
>  		condlog(3, "status = %s", status);
>  
>  		disassemble_map(pathvec, params, mpp);
> -
> -		/*
> -		 * disassemble_map() can add new paths to pathvec.
> -		 * If not in "fast list mode", we need to fetch information
> -		 * about them
> -		 */
> -		update_paths(mpp, (cmd == CMD_LIST_SHORT));
> +		update_pathvec_from_dm(pathvec, mpp,
> +				       (cmd == CMD_LIST_SHORT ? 0 : DI_ALL));

I personally don't think that "multipath -l" should be opening the path
fd. Checking sysfs seems like the limit of what we want to do for fast
listing. So, I would prefer

update_pathvec_from_dm(pathvec, mpp,
		       (cmd == CMD_LIST_SHORT ? DI_NOIO : DI_ALL));

That will make pathinfo exit early.

-Ben

>  
>  		if (cmd == CMD_LIST_LONG)
>  			mpp->bestpg = select_path_group(mpp);
> @@ -353,6 +291,7 @@ static int check_usable_paths(struct config *conf,
>  	dm_get_map(mpp->alias, &mpp->size, params);
>  	dm_get_status(mpp->alias, status);
>  	disassemble_map(pathvec, params, mpp);
> +	update_pathvec_from_dm(pathvec, mpp, 0);
>  	disassemble_status(status, mpp);
>  
>  	vector_foreach_slot (mpp->pg, pg, i) {
> -- 
> 2.26.2

Re: [PATCH 71/74] multipath: use update_pathvec_from_dm()

[Martin Wilck]

On Sun, 2020-07-19 at 21:57 -0500, Benjamin Marzinski wrote:
> On Thu, Jul 09, 2020 at 12:51:42PM +0200, mwilck@suse.com wrote:
> > From: Martin Wilck <mwilck@suse.com>
> > 
> > The multipath-specific function update_paths() can now be replaced
> > with
> > a call to update_pathvec_from_dm().
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  multipath/main.c | 67 +++-----------------------------------------
> > ----
> >  1 file changed, 3 insertions(+), 64 deletions(-)
> > 
> > diff --git a/multipath/main.c b/multipath/main.c
> > index 8a2a6f7..435c5d5 100644
> > --- a/multipath/main.c
> > +++ b/multipath/main.c
> > 
> >  static int
> >  get_dm_mpvec (enum mpath_cmds cmd, vector curmp, vector pathvec,
> > char * refwwid)
> >  {
> > @@ -273,13 +216,8 @@ get_dm_mpvec (enum mpath_cmds cmd, vector
> > curmp, vector pathvec, char * refwwid)
> >  		condlog(3, "status = %s", status);
> >  
> >  		disassemble_map(pathvec, params, mpp);
> > -
> > -		/*
> > -		 * disassemble_map() can add new paths to pathvec.
> > -		 * If not in "fast list mode", we need to fetch
> > information
> > -		 * about them
> > -		 */
> > -		update_paths(mpp, (cmd == CMD_LIST_SHORT));
> > +		update_pathvec_from_dm(pathvec, mpp,
> > +				       (cmd == CMD_LIST_SHORT ? 0 :
> > DI_ALL));
> 
> I personally don't think that "multipath -l" should be opening the
> path
> fd. Checking sysfs seems like the limit of what we want to do for
> fast
> listing. So, I would prefer
> 
> update_pathvec_from_dm(pathvec, mpp,
> 		       (cmd == CMD_LIST_SHORT ? DI_NOIO : DI_ALL));
> 
> That will make pathinfo exit early.

Good suggestion. But then I need to avoid rerunning pathinfo() for
already scanned paths in update_pathvec_from_dm(). Will do.

Regards
Martin

[PATCH 72/74] libmpathpersist: use update_pathvec_from_dm()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

The libmpathpersist-specific function updatepaths() can be replaced
with the generic function.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmpathpersist/mpath_persist.c | 54 +--------------------------------
 1 file changed, 1 insertion(+), 53 deletions(-)

diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c
index cb3182f..0c66ff3 100644
--- a/libmpathpersist/mpath_persist.c
+++ b/libmpathpersist/mpath_persist.c
@@ -65,52 +65,6 @@ mpath_lib_exit (struct config *conf)
 	return 0;
 }
 
-static int
-updatepaths (struct multipath * mpp)
-{
-	int i, j;
-	struct pathgroup * pgp;
-	struct path * pp;
-	struct config *conf;
-
-	if (!mpp->pg)
-		return 0;
-
-	vector_foreach_slot (mpp->pg, pgp, i){
-		if (!pgp->paths)
-			continue;
-
-		vector_foreach_slot (pgp->paths, pp, j){
-			if (!strlen(pp->dev)){
-				/*
-				 * path is not in sysfs anymore
-				 */
-				pp->state = PATH_DOWN;
-				continue;
-			}
-			pp->mpp = mpp;
-			if (pp->udev == NULL) {
-				pp->udev = get_udev_device(pp->dev_t, DEV_DEVT);
-				if (pp->udev == NULL) {
-					pp->state = PATH_DOWN;
-					continue;
-				}
-				conf = get_multipath_config();
-				pathinfo(pp, conf, DI_SYSFS|DI_CHECKER);
-				put_multipath_config(conf);
-				continue;
-			}
-			if (pp->state == PATH_UNCHECKED ||
-					pp->state == PATH_WILD) {
-				conf = get_multipath_config();
-				pathinfo(pp, conf, DI_CHECKER);
-				put_multipath_config(conf);
-			}
-		}
-	}
-	return 0;
-}
-
 int
 mpath_prin_activepath (struct multipath *mpp, int rq_servact,
 	struct prin_resp * resp, int noisy)
@@ -392,13 +346,7 @@ get_mpvec (vector curmp, vector pathvec, char * refwwid)
 		dm_get_status(mpp->alias, status);
 		condlog(3, "status = %s", status);
 		disassemble_map (pathvec, params, mpp);
-
-		/*
-		 * disassemble_map() can add new paths to pathvec.
-		 * If not in "fast list mode", we need to fetch information
-		 * about them
-		 */
-		updatepaths(mpp);
+		update_pathvec_from_dm(pathvec, mpp, DI_CHECKER);
 		disassemble_status (status, mpp);
 
 	}
-- 
2.26.2

[PATCH 73/74] libmultipath: decrease loglevel in store_path()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

"Empty device name" in store_path() can happen regularly and
shouldn't be logged at -v2.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/structs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libmultipath/structs.c b/libmultipath/structs.c
index 056a205..f4de542 100644
--- a/libmultipath/structs.c
+++ b/libmultipath/structs.c
@@ -322,7 +322,7 @@ store_path (vector pathvec, struct path * pp)
 		err++;
 	}
 	if (!strlen(pp->dev)) {
-		condlog(2, "%s: Empty device name", pp->dev_t);
+		condlog(3, "%s: Empty device name", pp->dev_t);
 		err++;
 	}
 
-- 
2.26.2

[PATCH 74/74] libmultipath: dmparser: constify function arguments

[mwilck]

From: Martin Wilck <mwilck@suse.com>

With the previous change that avoids additions to pathvec,
the pathvec argument to disassemble_map() is const now.
Also use const for the string arguments where possible.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/dmparser.c | 11 ++++++-----
 libmultipath/dmparser.h |  4 ++--
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
index e629a89..6d9488f 100644
--- a/libmultipath/dmparser.c
+++ b/libmultipath/dmparser.c
@@ -128,10 +128,11 @@ err:
  * Call update_pathvec_from_dm() after this function to make sure
  * all data structures are in a sane state.
  */
-int disassemble_map(vector pathvec, char *params, struct multipath *mpp)
+int disassemble_map(const struct _vector *pathvec,
+		    const char *params, struct multipath *mpp)
 {
 	char * word;
-	char * p;
+	const char *p;
 	int i, j, k;
 	int num_features = 0;
 	int num_hwhandler = 0;
@@ -344,10 +345,10 @@ out:
 	return 1;
 }
 
-int disassemble_status(char *params, struct multipath *mpp)
+int disassemble_status(const char *params, struct multipath *mpp)
 {
-	char * word;
-	char * p;
+	char *word;
+	const char *p;
 	int i, j, k;
 	int num_feature_args;
 	int num_hwhandler_args;
diff --git a/libmultipath/dmparser.h b/libmultipath/dmparser.h
index 1b45df0..212fee5 100644
--- a/libmultipath/dmparser.h
+++ b/libmultipath/dmparser.h
@@ -1,3 +1,3 @@
 int assemble_map (struct multipath *, char *, int);
-int disassemble_map (vector, char *, struct multipath *);
-int disassemble_status (char *, struct multipath *);
+int disassemble_map (const struct _vector *, const char *, struct multipath *);
+int disassemble_status (const char *, struct multipath *);
-- 
2.26.2

Re: [PATCH 00/74] multipath-tools series part V: removed path handling

[Benjamin Marzinski]

On Thu, Jul 09, 2020 at 12:51:24PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Hi Christophe, hi Ben,
> 
> This is part V of a larger patch series for multipath-tools I've been preparing.
> It's based on the previously submitted part IV.
> 
> 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-5".


For the part, with the exception of patches 61,63,64,65,66 & 71
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

> 
> The series addresses several issues with missing, unintialized, and removed
> paths. The motivation was that I strongly dislike the side effects of
> disassemble_map(), adding paths and setting WWIDs while parsing map
> parameters. IMO this has always been a layering violation. This patch set adds
> a new function dedicated to handling paths which have not been detected via
> udev but are present as members of dm maps. That makes it much easier to
> track and improve the logic applied when such devices are detected.
> 
> I believe that the new logic will also fix the issue recently reported by
> Chongyun ("libmultipath/dmparser: add missing path with good status when sync
> state with dm kernel"). At least, the approach is very similar, although
> the call to pathinfo() for new devices now happens in update_pathvec_from_dm()
> rather than in disassemble_map().
> 
> The patch set also gets rid of the "is_daemon" argument for disassemble_map(),
> causing semantics in daemon and non-daemon mode. The reason for this parameter
> goes way back into history; it was avoiding to re-add manually removed paths
> because they were still showing up in maps. But OTOH we must add missing
> devices which we've failed to detect. The patch set handles this by tracking
> the state of "being removed" as a new init state, and only actually removing
> the paths from pathvec when they don't show up in maps any more.
> 
> Regards
> Martin
> 
> Martin Wilck (21):
>   libmultipath: protect use of pp->udev
>   libmultipath: add uninitialize_path()
>   multipath-tools: introduce INIT_REMOVED state
>   libmultipath: update_mpp_paths(): handle INIT_REMOVED
>   libmultipath: verify_paths(): don't delete paths from pathvec
>   libmultipath: sync_paths(): handle INIT_REMOVED
>   libmultipath: orphan_paths(): delete paths in INIT_REMOVED state
>   libmultipath: adopt_paths(): skip removed paths
>   multipathd: ev_remove_path(): use INIT_REMOVED
>   multipathd: deal with INIT_REMOVED during path addition
>   multipathd: check_path(): set retrigger_delay if necessary
>   libmultipath: add update_pathvec_from_dm()
>   libmultipath: update_pathvec_from_dm: handle pp->mpp mismatch
>   libmultipath: disassemble_map(): always search paths by dev_t
>   libmultipath: disassemble_map(): require non-NULL pathvec
>   libmultipath: disassemble_map(): get rid of "is_daemon" argument
>   libmultipath: disassemble_map(): do not change pathvec and WWIDs
>   multipath: use update_pathvec_from_dm()
>   libmpathpersist: use update_pathvec_from_dm()
>   libmultipath: decrease loglevel in store_path()
>   libmultipath: dmparser: constify function arguments
> 
>  libmpathpersist/mpath_persist.c       |  56 +-----
>  libmultipath/configure.c              |   2 +-
>  libmultipath/discovery.c              |  15 +-
>  libmultipath/dmparser.c               |  70 ++-----
>  libmultipath/dmparser.h               |   4 +-
>  libmultipath/prioritizers/alua_rtpg.c |   6 +-
>  libmultipath/structs.c                |  21 +-
>  libmultipath/structs.h                |   6 +
>  libmultipath/structs_vec.c            | 263 +++++++++++++++++++++++---
>  libmultipath/structs_vec.h            |  11 +-
>  multipath/main.c                      |  71 +------
>  multipathd/cli_handlers.c             |  49 ++++-
>  multipathd/main.c                     | 113 ++++++++---
>  13 files changed, 445 insertions(+), 242 deletions(-)
> 
> -- 
> 2.26.2