[PATCH v2 0/8] multipath-tools fixes

[PATCH v2 0/8] multipath-tools fixes

[Martin Wilck]

The first patch is an attempt to fix
https://github.com/opensvc/multipath-tools/issues/105.

Patch 2-5 provide a re-implementation of pgcmp() without relying on
pgp->id, as discussed during the review of v1 of this series.

Patch 6 and 7 change the way multipathd triggers uevents when
it detects configuration changes, motivated by
https://github.com/opensvc/multipath-tools/issues/105.

Up to now, multipathd had done this basically only if the "failed" state of a
map had changed. This would miss cases where e.g. with find_multipaths strict,
a path had been added to the WWIDs file and "multipathd reconfigure" had been
run. In such cases, the paths would be added to the map, but udev would still
not see them.  The same holds for changes of the blacklist.

With this set, when a map is created, or when map creation fails, multipathd
checks whether udev already sees the correct properties for the device, and
triggers an uevent otherwise.

When maps are to be removed because of configuration changes in
coalesce_maps() (for example if a WWID has been removed from the WWIDs file),
multipathd now also triggers uevents for the respective path devices. This
is only done in coalesce_maps(), where current configuration changes are
reflected. We can't do it after a "remove map" CLI command, for example,
because that is by definition a temporary change. If we triggered an uevent
for that, multipathd would recreate the map it just removed as soone as it
receives the uevents.

Patch 8 is a minor log message change.

Martin Wilck (8):
  libmultipath: fix handling of pp->pgindex
  libmpathutil: change STATIC_BITFIELD to BITFIELD
  libmpathutil: add cleanup_bitfield()
  libmultipath: move pathcmp() to configure.c
  libmultipath: re-implement pgcmp without the pathgroup id
  libmultipath: trigger uevents upon map creation in domap()
  multipathd: trigger uevents upon map removal in coalesce_maps()
  multipathd: improve a log message in coalesce_maps()

 libmpathutil/libmpathutil.version |   1 +
 libmpathutil/util.c               |   5 ++
 libmpathutil/util.h               |  10 +--
 libmultipath/configure.c          | 124 ++++++++++++++++++++----------
 libmultipath/devmapper.c          |   9 +--
 libmultipath/discovery.c          |   2 +-
 libmultipath/dmparser.c           |   1 -
 libmultipath/pgpolicies.c         |   6 ++
 libmultipath/structs.c            |  31 +++-----
 libmultipath/structs.h            |   3 -
 libmultipath/structs_vec.c        |  15 ++++
 multipathd/main.c                 |   6 +-
 12 files changed, 135 insertions(+), 78 deletions(-)

-- 
2.47.0

[PATCH v2 1/8] libmultipath: fix handling of pp->pgindex

[Martin Wilck]

pp->pgindex is set in disassemble_map() when a map is parsed.
There are various possiblities for this index to become invalid.
pp->pgindex is only used in enable_group() and followover_should_fallback(),
and both callers take no action if it is 0, which is the right
thing to do if we don't know the path's pathgroup.

Make sure pp->pgindex is reset to 0 in various places:
- when it's orphaned,
- before (re)grouping paths,
- when we detect a bad mpp assignment in update_pathvec_from_dm().
- when a pathgroup is deleted in update_pathvec_from_dm(). In this
  case, pgindex needs to be invalidated for all paths in all pathgroups
  after the one that was deleted.

The hunk in group_paths is mostly redundant with the hunk in free_pgvec(), but
because we're looping over pg->paths in the former and over pg->pgp in
the latter, I think it's better too play safe.

Fixes: 99db1bd ("[multipathd] re-enable disabled PG when at least one path is up")
Fixes: https://github.com/opensvc/multipath-tools/issues/105
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/pgpolicies.c  |  6 ++++++
 libmultipath/structs.c     | 12 +++++++++++-
 libmultipath/structs_vec.c | 15 +++++++++++++++
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/libmultipath/pgpolicies.c b/libmultipath/pgpolicies.c
index edc3c61..23ef2bd 100644
--- a/libmultipath/pgpolicies.c
+++ b/libmultipath/pgpolicies.c
@@ -127,6 +127,8 @@ fail:
 int group_paths(struct multipath *mp, int marginal_pathgroups)
 {
 	vector normal, marginal;
+	struct path *pp;
+	int i;
 
 	if (!mp->pg)
 		mp->pg = vector_alloc();
@@ -138,6 +140,10 @@ int group_paths(struct multipath *mp, int marginal_pathgroups)
 	if (!mp->pgpolicyfn)
 		goto fail;
 
+	/* Reset pgindex, we're going to invalidate it */
+	vector_foreach_slot(mp->paths, pp, i)
+		pp->pgindex = 0;
+
 	if (!marginal_pathgroups ||
 	    split_marginal_paths(mp->paths, &normal, &marginal) != 0) {
 		if (mp->pgpolicyfn(mp, mp->paths) != 0)
diff --git a/libmultipath/structs.c b/libmultipath/structs.c
index 61c8f32..4851725 100644
--- a/libmultipath/structs.c
+++ b/libmultipath/structs.c
@@ -239,8 +239,18 @@ free_pgvec (vector pgvec, enum free_path_mode free_paths)
 	if (!pgvec)
 		return;
 
-	vector_foreach_slot(pgvec, pgp, i)
+	vector_foreach_slot(pgvec, pgp, i) {
+
+		/* paths are going to be re-grouped, reset pgindex */
+		if (free_paths != FREE_PATHS) {
+			struct path *pp;
+			int j;
+
+			vector_foreach_slot(pgp->paths, pp, j)
+				pp->pgindex = 0;
+		}
 		free_pathgroup(pgp, free_paths);
+	}
 
 	vector_free(pgvec);
 }
diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index d22056c..35883fc 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -108,6 +108,7 @@ static bool update_pathvec_from_dm(vector pathvec, struct multipath *mpp,
 	struct config *conf;
 	bool mpp_has_wwid;
 	bool must_reload = false;
+	bool pg_deleted = false;
 
 	if (!mpp->pg)
 		return false;
@@ -125,6 +126,10 @@ static bool update_pathvec_from_dm(vector pathvec, struct multipath *mpp,
 
 		vector_foreach_slot(pgp->paths, pp, j) {
 
+			/* A pathgroup has been deleted before. Invalidate pgindex */
+			if (pg_deleted)
+				pp->pgindex = 0;
+
 			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);
@@ -139,6 +144,13 @@ static bool update_pathvec_from_dm(vector pathvec, struct multipath *mpp,
 				must_reload = true;
 				dm_fail_path(mpp->alias, pp->dev_t);
 				vector_del_slot(pgp->paths, j--);
+				/*
+				 * pp->pgindex has been set in disassemble_map(),
+				 * which has probably been called just before for
+				 * mpp. So he pgindex relates to mpp and may be
+				 * wrong for pp->mpp. Invalidate it.
+				 */
+				pp->pgindex = 0;
 				continue;
 			}
 			pp->mpp = mpp;
@@ -237,6 +249,8 @@ static bool update_pathvec_from_dm(vector pathvec, struct multipath *mpp,
 		vector_del_slot(mpp->pg, i--);
 		free_pathgroup(pgp, KEEP_PATHS);
 		must_reload = true;
+		/* Invalidate pgindex for all other pathgroups */
+		pg_deleted = true;
 	}
 	mpp->need_reload = mpp->need_reload || must_reload;
 	return must_reload;
@@ -354,6 +368,7 @@ void orphan_path(struct path *pp, const char *reason)
 {
 	condlog(3, "%s: orphan path, %s", pp->dev, reason);
 	pp->mpp = NULL;
+	pp->pgindex = 0;
 	uninitialize_path(pp);
 }
 
-- 
2.47.0

[PATCH v2 2/8] libmpathutil: change STATIC_BITFIELD to BITFIELD

[Martin Wilck]

A non-dynamically allocated bitfield doesn't have to have static
scope, it can also be a local variable on the stack. Change the macro such
that it can also be used for the latter case.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmpathutil/util.h      | 8 ++++----
 libmultipath/discovery.c | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/libmpathutil/util.h b/libmpathutil/util.h
index b1772e3..df7f91e 100644
--- a/libmpathutil/util.h
+++ b/libmpathutil/util.h
@@ -91,15 +91,15 @@ struct bitfield {
 	bitfield_t bits[];
 };
 
-#define STATIC_BITFIELD(name, length)					\
-	static struct {							\
+#define BITFIELD(name, length)					\
+	struct {							\
 		unsigned int len;					\
 		bitfield_t bits[((length) - 1) / bits_per_slot + 1];	\
-	} __static__ ## name = {					\
+	} __storage_for__ ## name = {					\
 		.len = (length),					\
 		.bits = { 0, },						\
 	}; \
-	struct bitfield *name = (struct bitfield *)& __static__ ## name
+	struct bitfield *name = (struct bitfield *)& __storage_for__ ## name
 
 struct bitfield *alloc_bitfield(unsigned int maxbit);
 
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index b585156..2be1f5a 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -875,7 +875,7 @@ sysfs_set_nexus_loss_tmo(struct path *pp)
 static void
 scsi_tmo_error_msg(struct path *pp)
 {
-	STATIC_BITFIELD(bf, LAST_BUS_PROTOCOL_ID + 1);
+	static BITFIELD(bf, LAST_BUS_PROTOCOL_ID + 1);
 	STRBUF_ON_STACK(proto_buf);
 	unsigned int proto_id = bus_protocol_id(pp);
 
-- 
2.47.0

[PATCH v2 3/8] libmpathutil: add cleanup_bitfield()

[Martin Wilck]

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmpathutil/libmpathutil.version | 1 +
 libmpathutil/util.c               | 5 +++++
 libmpathutil/util.h               | 2 +-
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/libmpathutil/libmpathutil.version b/libmpathutil/libmpathutil.version
index fb92f72..14aa083 100644
--- a/libmpathutil/libmpathutil.version
+++ b/libmpathutil/libmpathutil.version
@@ -51,6 +51,7 @@ global:
 	append_strbuf_str__;
 	append_strbuf_quoted;
 	basenamecpy;
+	cleanup_bitfield;
 	cleanup_charp;
 	cleanup_fclose;
 	cleanup_fd_ptr;
diff --git a/libmpathutil/util.c b/libmpathutil/util.c
index 1255974..38e984f 100644
--- a/libmpathutil/util.c
+++ b/libmpathutil/util.c
@@ -337,6 +337,11 @@ void cleanup_fclose(void *p)
 		fclose(p);
 }
 
+void cleanup_bitfield(struct bitfield **p)
+{
+	free(*p);
+}
+
 struct bitfield *alloc_bitfield(unsigned int maxbit)
 {
 	unsigned int n;
diff --git a/libmpathutil/util.h b/libmpathutil/util.h
index df7f91e..3799fd4 100644
--- a/libmpathutil/util.h
+++ b/libmpathutil/util.h
@@ -146,5 +146,5 @@ static inline void clear_bit_in_bitfield(unsigned int bit, struct bitfield *bf)
 void cleanup_charp(char **p);
 void cleanup_ucharp(unsigned char **p);
 void cleanup_udev_device(struct udev_device **udd);
-
+void cleanup_bitfield(struct bitfield **p);
 #endif /* UTIL_H_INCLUDED */
-- 
2.47.0

[PATCH v2 4/8] libmultipath: move pathcmp() to configure.c

[Martin Wilck]

... as it has only one caller, and make it static. No functional changes.

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

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index d0e9c95..9ab84d5 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -426,6 +426,25 @@ compute_pgid(struct pathgroup * pgp)
 		pgp->id ^= (long)pp;
 }
 
+static int pathcmp(const struct pathgroup *pgp, const struct pathgroup *cpgp)
+{
+	int i, j;
+	struct path *pp, *cpp;
+	int pnum = 0, found = 0;
+
+	vector_foreach_slot(pgp->paths, pp, i) {
+		pnum++;
+		vector_foreach_slot(cpgp->paths, cpp, j) {
+			if ((long)pp == (long)cpp) {
+				found++;
+				break;
+			}
+		}
+	}
+
+	return pnum - found;
+}
+
 static int
 pgcmp (struct multipath * mpp, struct multipath * cmpp)
 {
diff --git a/libmultipath/structs.c b/libmultipath/structs.c
index 4851725..dfa547b 100644
--- a/libmultipath/structs.c
+++ b/libmultipath/structs.c
@@ -627,25 +627,6 @@ int count_active_pending_paths(const struct multipath *mpp)
 	return do_pathcount(mpp, states, 3);
 }
 
-int pathcmp(const struct pathgroup *pgp, const struct pathgroup *cpgp)
-{
-	int i, j;
-	struct path *pp, *cpp;
-	int pnum = 0, found = 0;
-
-	vector_foreach_slot(pgp->paths, pp, i) {
-		pnum++;
-		vector_foreach_slot(cpgp->paths, cpp, j) {
-			if ((long)pp == (long)cpp) {
-				found++;
-				break;
-			}
-		}
-	}
-
-	return pnum - found;
-}
-
 struct path *
 first_path (const struct multipath * mpp)
 {
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 4821f19..49d9a2f 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -598,7 +598,6 @@ struct path *mp_find_path_by_devt(const struct multipath *mpp, const char *devt)
 int pathcount (const struct multipath *, int);
 int count_active_paths(const struct multipath *);
 int count_active_pending_paths(const struct multipath *);
-int pathcmp (const struct pathgroup *, const struct pathgroup *);
 int add_feature (char **, const char *);
 int remove_feature (char **, const char *);
 
-- 
2.47.0

[PATCH v2 5/8] libmultipath: re-implement pgcmp without the pathgroup id

[Martin Wilck]

pgp->id is not only calculated with a weak XOR hash, it can also
get wrong if any path regrouping occurs. As it's not a big gain
performance-wise, just drop pgp->id and compare path groups
directly.

The previous algorithm didn't detect the case case where cpgp
contained a path that was not contained in pgp. Fix this, too,
by comparing the number of paths in the path groups and making
sure that each pg of mpp is matched by exactly one pg of cmpp.

Fixes: 90773ba ("libmultipath: resolve hash collisions in pgcmp()")
Suggested-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c | 94 ++++++++++++++++++++++++++--------------
 libmultipath/dmparser.c  |  1 -
 libmultipath/structs.h   |  1 -
 3 files changed, 61 insertions(+), 35 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 9ab84d5..bd71e68 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -416,16 +416,6 @@ int setup_map(struct multipath *mpp, char **params, struct vectors *vecs)
 	return 0;
 }
 
-static void
-compute_pgid(struct pathgroup * pgp)
-{
-	struct path * pp;
-	int i;
-
-	vector_foreach_slot (pgp->paths, pp, i)
-		pgp->id ^= (long)pp;
-}
-
 static int pathcmp(const struct pathgroup *pgp, const struct pathgroup *cpgp)
 {
 	int i, j;
@@ -445,32 +435,74 @@ static int pathcmp(const struct pathgroup *pgp, const struct pathgroup *cpgp)
 	return pnum - found;
 }
 
-static int
-pgcmp (struct multipath * mpp, struct multipath * cmpp)
+static int pgcmp(struct multipath *mpp, struct multipath *cmpp)
 {
 	int i, j;
-	struct pathgroup * pgp;
-	struct pathgroup * cpgp;
-	int r = 0;
+	struct pathgroup *pgp, *cpgp;
+	BITFIELD(bf, bits_per_slot);
+	struct bitfield *bf__  __attribute__((cleanup(cleanup_bitfield))) = NULL;
 
 	if (!mpp)
 		return 0;
 
-	vector_foreach_slot (mpp->pg, pgp, i) {
-		compute_pgid(pgp);
+	if (VECTOR_SIZE(mpp->pg) != VECTOR_SIZE(cmpp->pg))
+		return 1;
 
-		vector_foreach_slot (cmpp->pg, cpgp, j) {
-			if (pgp->id == cpgp->id &&
-			    !pathcmp(pgp, cpgp)) {
-				r = 0;
-				break;
-			}
-			r++;
-		}
-		if (r)
-			return r;
+	/* handle the unlikely case of more than 64 pgs */
+	if ((unsigned int)VECTOR_SIZE(cmpp->pg) > bits_per_slot) {
+		bf__ = alloc_bitfield(VECTOR_SIZE(cmpp->pg));
+		if (bf__ == NULL)
+			/* error - if in doubt, assume not equal */
+			return 1;
+		bf = bf__;
 	}
-	return r;
+
+	vector_foreach_slot (mpp->pg, pgp, i) {
+
+		if (VECTOR_SIZE(pgp->paths) == 0) {
+			bool found = false;
+
+			vector_foreach_slot (cmpp->pg, cpgp, j) {
+				if (!is_bit_set_in_bitfield(j, bf) &&
+				    VECTOR_SIZE(cpgp->paths) == 0) {
+					set_bit_in_bitfield(j, bf);
+					found = true;
+					break;
+				}
+			}
+			if (!found)
+				return 1;
+		} else {
+			bool found = false;
+			struct path *pp = VECTOR_SLOT(pgp->paths, 0);
+
+			/* search for a pg in cmpp that contains pp */
+			vector_foreach_slot (cmpp->pg, cpgp, j) {
+				if (!is_bit_set_in_bitfield(j, bf) &&
+				    VECTOR_SIZE(cpgp->paths) == VECTOR_SIZE(pgp->paths)) {
+					int k;
+					struct path *cpp;
+
+					vector_foreach_slot(cpgp->paths, cpp, k) {
+						if (cpp != pp)
+							continue;
+						/*
+						 * cpgp contains pp.
+						 * See if it's identical.
+						 */
+						set_bit_in_bitfield(j, bf);
+						if (pathcmp(pgp, cpgp))
+							return 1;
+						found = true;
+						break;
+					}
+				}
+			}
+			if (!found)
+				return 1;
+		}
+	}
+	return 0;
 }
 
 void trigger_partitions_udev_change(struct udev_device *dev,
@@ -763,11 +795,7 @@ void select_action (struct multipath *mpp, const struct vector_s *curmp,
 		select_reload_action(mpp, "minio change");
 		return;
 	}
-	if (!cmpp->pg || VECTOR_SIZE(cmpp->pg) != VECTOR_SIZE(mpp->pg)) {
-		select_reload_action(mpp, "path group number change");
-		return;
-	}
-	if (pgcmp(mpp, cmpp)) {
+	if (!cmpp->pg || pgcmp(mpp, cmpp)) {
 		select_reload_action(mpp, "path group topology change");
 		return;
 	}
diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
index 1d0506d..c8c47e0 100644
--- a/libmultipath/dmparser.c
+++ b/libmultipath/dmparser.c
@@ -307,7 +307,6 @@ int disassemble_map(const struct vector_s *pathvec,
 
 			free(word);
 
-			pgp->id ^= (long)pp;
 			pp->pgindex = i + 1;
 
 			for (k = 0; k < num_paths_args; k++)
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 49d9a2f..2159cb3 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -531,7 +531,6 @@ static inline int san_path_check_enabled(const struct multipath *mpp)
 }
 
 struct pathgroup {
-	long id;
 	int status;
 	int priority;
 	int enabled_paths;
-- 
2.47.0

Re: [PATCH v2 5/8] libmultipath: re-implement pgcmp without the pathgroup id

[Benjamin Marzinski]

On Thu, Nov 28, 2024 at 12:04:27AM +0100, Martin Wilck wrote:
> pgp->id is not only calculated with a weak XOR hash, it can also
> get wrong if any path regrouping occurs. As it's not a big gain
> performance-wise, just drop pgp->id and compare path groups
> directly.
> 
> The previous algorithm didn't detect the case case where cpgp
> contained a path that was not contained in pgp. Fix this, too,
> by comparing the number of paths in the path groups and making
> sure that each pg of mpp is matched by exactly one pg of cmpp.
> 
> Fixes: 90773ba ("libmultipath: resolve hash collisions in pgcmp()")
> Suggested-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/configure.c | 94 ++++++++++++++++++++++++++--------------
>  libmultipath/dmparser.c  |  1 -
>  libmultipath/structs.h   |  1 -
>  3 files changed, 61 insertions(+), 35 deletions(-)
> 
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index 9ab84d5..bd71e68 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -416,16 +416,6 @@ int setup_map(struct multipath *mpp, char **params, struct vectors *vecs)
>  	return 0;
>  }
>  
> -static void
> -compute_pgid(struct pathgroup * pgp)
> -{
> -	struct path * pp;
> -	int i;
> -
> -	vector_foreach_slot (pgp->paths, pp, i)
> -		pgp->id ^= (long)pp;
> -}
> -
>  static int pathcmp(const struct pathgroup *pgp, const struct pathgroup *cpgp)
>  {
>  	int i, j;
> @@ -445,32 +435,74 @@ static int pathcmp(const struct pathgroup *pgp, const struct pathgroup *cpgp)
>  	return pnum - found;
>  }
>  
> -static int
> -pgcmp (struct multipath * mpp, struct multipath * cmpp)
> +static int pgcmp(struct multipath *mpp, struct multipath *cmpp)
>  {
>  	int i, j;
> -	struct pathgroup * pgp;
> -	struct pathgroup * cpgp;
> -	int r = 0;
> +	struct pathgroup *pgp, *cpgp;
> +	BITFIELD(bf, bits_per_slot);
> +	struct bitfield *bf__  __attribute__((cleanup(cleanup_bitfield))) = NULL;

Nitpick. I'm pretty sure we can't call pgcmp() unless mpp and cmpp are
set. It seems like we should either trust the parameters (this is a
static function with one caller) or check both of them.

>  	if (!mpp)
>  		return 0;
>  
> -	vector_foreach_slot (mpp->pg, pgp, i) {
> -		compute_pgid(pgp);
> +	if (VECTOR_SIZE(mpp->pg) != VECTOR_SIZE(cmpp->pg))
> +		return 1;
>  
> -		vector_foreach_slot (cmpp->pg, cpgp, j) {
> -			if (pgp->id == cpgp->id &&
> -			    !pathcmp(pgp, cpgp)) {
> -				r = 0;
> -				break;
> -			}
> -			r++;
> -		}
> -		if (r)
> -			return r;
> +	/* handle the unlikely case of more than 64 pgs */
> +	if ((unsigned int)VECTOR_SIZE(cmpp->pg) > bits_per_slot) {
> +		bf__ = alloc_bitfield(VECTOR_SIZE(cmpp->pg));
> +		if (bf__ == NULL)
> +			/* error - if in doubt, assume not equal */
> +			return 1;
> +		bf = bf__;
>  	}
> -	return r;
> +
> +	vector_foreach_slot (mpp->pg, pgp, i) {
> +
> +		if (VECTOR_SIZE(pgp->paths) == 0) {
> +			bool found = false;
> +
> +			vector_foreach_slot (cmpp->pg, cpgp, j) {
> +				if (!is_bit_set_in_bitfield(j, bf) &&
> +				    VECTOR_SIZE(cpgp->paths) == 0) {
> +					set_bit_in_bitfield(j, bf);
> +					found = true;
> +					break;
> +				}
> +			}
> +			if (!found)
> +				return 1;
> +		} else {
> +			bool found = false;
> +			struct path *pp = VECTOR_SLOT(pgp->paths, 0);
> +
> +			/* search for a pg in cmpp that contains pp */
> +			vector_foreach_slot (cmpp->pg, cpgp, j) {
> +				if (!is_bit_set_in_bitfield(j, bf) &&
> +				    VECTOR_SIZE(cpgp->paths) == VECTOR_SIZE(pgp->paths)) {
> +					int k;
> +					struct path *cpp;
> +
> +					vector_foreach_slot(cpgp->paths, cpp, k) {
> +						if (cpp != pp)
> +							continue;
> +						/*
> +						 * cpgp contains pp.
> +						 * See if it's identical.
> +						 */
> +						set_bit_in_bitfield(j, bf);
> +						if (pathcmp(pgp, cpgp))
> +							return 1;
> +						found = true;
> +						break;
> +					}
> +				}
> +			}
> +			if (!found)
> +				return 1;
> +		}
> +	}
> +	return 0;
>  }
>  
>  void trigger_partitions_udev_change(struct udev_device *dev,
> @@ -763,11 +795,7 @@ void select_action (struct multipath *mpp, const struct vector_s *curmp,
>  		select_reload_action(mpp, "minio change");
>  		return;
>  	}
> -	if (!cmpp->pg || VECTOR_SIZE(cmpp->pg) != VECTOR_SIZE(mpp->pg)) {
> -		select_reload_action(mpp, "path group number change");
> -		return;
> -	}
> -	if (pgcmp(mpp, cmpp)) {
> +	if (!cmpp->pg || pgcmp(mpp, cmpp)) {

I know that the code did this before, but is there a reason why we
always reload if cmpp->pg is NULL? I'm pretty sure it's possible to call
select action when neither mpp nor cmpp have any paths. (cli_reload on a
pathless device should do that). In this case, the topology isn't
changing, and AFAICS pgcmp() should work with no pathgroups.

-Ben

>  		select_reload_action(mpp, "path group topology change");
>  		return;
>  	}
> diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
> index 1d0506d..c8c47e0 100644
> --- a/libmultipath/dmparser.c
> +++ b/libmultipath/dmparser.c
> @@ -307,7 +307,6 @@ int disassemble_map(const struct vector_s *pathvec,
>  
>  			free(word);
>  
> -			pgp->id ^= (long)pp;
>  			pp->pgindex = i + 1;
>  
>  			for (k = 0; k < num_paths_args; k++)
> diff --git a/libmultipath/structs.h b/libmultipath/structs.h
> index 49d9a2f..2159cb3 100644
> --- a/libmultipath/structs.h
> +++ b/libmultipath/structs.h
> @@ -531,7 +531,6 @@ static inline int san_path_check_enabled(const struct multipath *mpp)
>  }
>  
>  struct pathgroup {
> -	long id;
>  	int status;
>  	int priority;
>  	int enabled_paths;
> -- 
> 2.47.0

Re: [PATCH v2 5/8] libmultipath: re-implement pgcmp without the pathgroup id

[Martin Wilck]

On Mon, 2024-12-02 at 19:17 -0500, Benjamin Marzinski wrote:
> On Thu, Nov 28, 2024 at 12:04:27AM +0100, Martin Wilck wrote:
> > 
> > @@ -763,11 +795,7 @@ void select_action (struct multipath *mpp,
> > const struct vector_s *curmp,
> >  		select_reload_action(mpp, "minio change");
> >  		return;
> >  	}
> > -	if (!cmpp->pg || VECTOR_SIZE(cmpp->pg) != VECTOR_SIZE(mpp-
> > >pg)) {
> > -		select_reload_action(mpp, "path group number
> > change");
> > -		return;
> > -	}
> > -	if (pgcmp(mpp, cmpp)) {
> > +	if (!cmpp->pg || pgcmp(mpp, cmpp)) {
> 
> I know that the code did this before, but is there a reason why we
> always reload if cmpp->pg is NULL? I'm pretty sure it's possible to
> call
> select action when neither mpp nor cmpp have any paths. (cli_reload
> on a
> pathless device should do that). In this case, the topology isn't
> changing, and AFAICS pgcmp() should work with no pathgroups.


It dates back to b0991a8 ("Allow zero paths for multipath map"),
multipath-tools 0.4.9.

I guess back then Hannes either encountered in crash in some situation,
or he was just being cautious.

I can remove this check.

Martin

Re: [PATCH v2 5/8] libmultipath: re-implement pgcmp without the pathgroup id

[Martin Wilck]

On Tue, 2024-12-03 at 13:40 +0100, Martin Wilck wrote:
> On Mon, 2024-12-02 at 19:17 -0500, Benjamin Marzinski wrote:
> > On Thu, Nov 28, 2024 at 12:04:27AM +0100, Martin Wilck wrote:
> > > 
> > > @@ -763,11 +795,7 @@ void select_action (struct multipath *mpp,
> > > const struct vector_s *curmp,
> > >  		select_reload_action(mpp, "minio change");
> > >  		return;
> > >  	}
> > > -	if (!cmpp->pg || VECTOR_SIZE(cmpp->pg) !=
> > > VECTOR_SIZE(mpp-
> > > > pg)) {
> > > -		select_reload_action(mpp, "path group number
> > > change");
> > > -		return;
> > > -	}
> > > -	if (pgcmp(mpp, cmpp)) {
> > > +	if (!cmpp->pg || pgcmp(mpp, cmpp)) {
> > 
> > I know that the code did this before, but is there a reason why we
> > always reload if cmpp->pg is NULL? I'm pretty sure it's possible to
> > call
> > select action when neither mpp nor cmpp have any paths. (cli_reload
> > on a
> > pathless device should do that). In this case, the topology isn't
> > changing, and AFAICS pgcmp() should work with no pathgroups.
> 
> 
> It dates back to b0991a8 ("Allow zero paths for multipath map"),
> multipath-tools 0.4.9.
> 
> I guess back then Hannes either encountered in crash in some
> situation,
> or he was just being cautious.
> 
> I can remove this check.

As all our vector macros handle the (V==NULL) case, we can rely on them
doing the "right thing" if we just check VECTOR_SIZE(). While I kind of
dislike these hidden checks (other code bases like systemd use assert()
instead, which probably helps finding bugs), we rely on this behavior
basically everywhere in our code, so why not here.

Regards
Martin

[PATCH v2 6/8] libmultipath: trigger uevents upon map creation in domap()

[Martin Wilck]

If map creation succeeds, previously not multipathed devices are
now multipathed. udev may not have noticed this yet, thus trigger
path uevents to make it aware of the situation. Likewise, if
creating a map fails, the paths in question were likely considered
multipath members by udev, too. They will now be marked as failed,
so trigger an event in this situation as well.

Fixes: https://github.com/opensvc/multipath-tools/issues/103
Suggested-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c | 11 ++++-------
 libmultipath/devmapper.c |  9 +++------
 libmultipath/structs.h   |  1 -
 3 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index bd71e68..2b1dfd2 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -614,8 +614,6 @@ trigger_paths_udev_change(struct multipath *mpp, bool is_mpath)
 		vector_foreach_slot(pgp->paths, pp, j)
 			trigger_path_udev_change(pp, is_mpath);
 	}
-
-	mpp->needs_paths_uevent = 0;
 }
 
 static int sysfs_set_max_sectors_kb(struct multipath *mpp)
@@ -1002,10 +1000,10 @@ int domap(struct multipath *mpp, char *params, int is_daemon)
 		 * succeeded
 		 */
 		mpp->force_udev_reload = 0;
-		if (mpp->action == ACT_CREATE &&
-		    (remember_wwid(mpp->wwid) == 1 ||
-		     mpp->needs_paths_uevent))
+		if (mpp->action == ACT_CREATE) {
+			remember_wwid(mpp->wwid);
 			trigger_paths_udev_change(mpp, true);
+		}
 		if (!is_daemon) {
 			/* multipath client mode */
 			dm_switchgroup(mpp->alias, mpp->bestpg);
@@ -1030,8 +1028,7 @@ int domap(struct multipath *mpp, char *params, int is_daemon)
 		}
 		dm_setgeometry(mpp);
 		return DOMAP_OK;
-	} else if (r == DOMAP_FAIL && mpp->action == ACT_CREATE &&
-		   mpp->needs_paths_uevent)
+	} else if (r == DOMAP_FAIL && mpp->action == ACT_CREATE)
 		trigger_paths_udev_change(mpp, false);
 
 	return DOMAP_FAIL;
diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index d4dd954..55052cf 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -537,7 +537,7 @@ static uint16_t build_udev_flags(const struct multipath *mpp, int reload)
 		 MPATH_UDEV_RELOAD_FLAG : 0);
 }
 
-int dm_addmap_create (struct multipath *mpp, char * params)
+int dm_addmap_create (struct multipath *mpp, char *params)
 {
 	int ro;
 	uint16_t udev_flags = build_udev_flags(mpp, 0);
@@ -547,9 +547,7 @@ int dm_addmap_create (struct multipath *mpp, char * params)
 
 		if (dm_addmap(DM_DEVICE_CREATE, TGT_MPATH, mpp, params, ro,
 			      udev_flags)) {
-			if (unmark_failed_wwid(mpp->wwid) ==
-			    WWID_FAILED_CHANGED)
-				mpp->needs_paths_uevent = 1;
+			unmark_failed_wwid(mpp->wwid);
 			return 1;
 		}
 		/*
@@ -570,8 +568,7 @@ int dm_addmap_create (struct multipath *mpp, char * params)
 			break;
 		}
 	}
-	if (mark_failed_wwid(mpp->wwid) == WWID_FAILED_CHANGED)
-		mpp->needs_paths_uevent = 1;
+	mark_failed_wwid(mpp->wwid);
 	return 0;
 }
 
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 2159cb3..6a30c59 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -467,7 +467,6 @@ struct multipath {
 	int max_sectors_kb;
 	int force_readonly;
 	int force_udev_reload;
-	int needs_paths_uevent;
 	int ghost_delay;
 	int ghost_delay_tick;
 	int queue_mode;
-- 
2.47.0

[PATCH v2 7/8] multipathd: trigger uevents upon map removal in coalesce_maps()

[Martin Wilck]

... if a map has been flushed. In this case, we know that the
the paths haven't been multipathed by coalesce_paths() because of the current
configuration (failure to create the map can't be the reason if the map
exists in coalesce_maps()). Make sure udev sees the paths which have
been released from the map as non-multipath.

Note that this is the only case where maps are flushed where it is correct
to trigger paths uevents. In other cases, e.g. after a "remove map" CLI
command, the configuration is unchanged and if we triggered an uevent,
the map would be re-created by multipathd when the uevent arrived.

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

diff --git a/multipathd/main.c b/multipathd/main.c
index fcab1ed..9ed27da 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -794,8 +794,10 @@ coalesce_maps(struct vectors *vecs, vector nmpv)
 				vector_del_slot(ompv, i);
 				i--;
 			}
-			else
+			else {
 				condlog(2, "%s devmap removed", ompp->alias);
+				trigger_paths_udev_change(ompp, false);
+			}
 		} else if (reassign_maps) {
 			condlog(3, "%s: Reassign existing device-mapper"
 				" devices", ompp->alias);
-- 
2.47.0

[PATCH v2 8/8] multipathd: improve a log message in coalesce_maps()

[Martin Wilck]

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

diff --git a/multipathd/main.c b/multipathd/main.c
index 9ed27da..e9d9848 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -795,7 +795,7 @@ coalesce_maps(struct vectors *vecs, vector nmpv)
 				i--;
 			}
 			else {
-				condlog(2, "%s devmap removed", ompp->alias);
+				condlog(2, "%s: multipath map removed", ompp->alias);
 				trigger_paths_udev_change(ompp, false);
 			}
 		} else if (reassign_maps) {
-- 
2.47.0

Re: [PATCH v2 0/8] multipath-tools fixes

[Benjamin Marzinski]

On Thu, Nov 28, 2024 at 12:04:22AM +0100, Martin Wilck wrote:

For all but patch 5/8:
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

> The first patch is an attempt to fix
> https://github.com/opensvc/multipath-tools/issues/105.
> 
> Patch 2-5 provide a re-implementation of pgcmp() without relying on
> pgp->id, as discussed during the review of v1 of this series.
> 
> Patch 6 and 7 change the way multipathd triggers uevents when
> it detects configuration changes, motivated by
> https://github.com/opensvc/multipath-tools/issues/105.
> 
> Up to now, multipathd had done this basically only if the "failed" state of a
> map had changed. This would miss cases where e.g. with find_multipaths strict,
> a path had been added to the WWIDs file and "multipathd reconfigure" had been
> run. In such cases, the paths would be added to the map, but udev would still
> not see them.  The same holds for changes of the blacklist.
> 
> With this set, when a map is created, or when map creation fails, multipathd
> checks whether udev already sees the correct properties for the device, and
> triggers an uevent otherwise.
> 
> When maps are to be removed because of configuration changes in
> coalesce_maps() (for example if a WWID has been removed from the WWIDs file),
> multipathd now also triggers uevents for the respective path devices. This
> is only done in coalesce_maps(), where current configuration changes are
> reflected. We can't do it after a "remove map" CLI command, for example,
> because that is by definition a temporary change. If we triggered an uevent
> for that, multipathd would recreate the map it just removed as soone as it
> receives the uevents.
> 
> Patch 8 is a minor log message change.
> 
> Martin Wilck (8):
>   libmultipath: fix handling of pp->pgindex
>   libmpathutil: change STATIC_BITFIELD to BITFIELD
>   libmpathutil: add cleanup_bitfield()
>   libmultipath: move pathcmp() to configure.c
>   libmultipath: re-implement pgcmp without the pathgroup id
>   libmultipath: trigger uevents upon map creation in domap()
>   multipathd: trigger uevents upon map removal in coalesce_maps()
>   multipathd: improve a log message in coalesce_maps()
> 
>  libmpathutil/libmpathutil.version |   1 +
>  libmpathutil/util.c               |   5 ++
>  libmpathutil/util.h               |  10 +--
>  libmultipath/configure.c          | 124 ++++++++++++++++++++----------
>  libmultipath/devmapper.c          |   9 +--
>  libmultipath/discovery.c          |   2 +-
>  libmultipath/dmparser.c           |   1 -
>  libmultipath/pgpolicies.c         |   6 ++
>  libmultipath/structs.c            |  31 +++-----
>  libmultipath/structs.h            |   3 -
>  libmultipath/structs_vec.c        |  15 ++++
>  multipathd/main.c                 |   6 +-
>  12 files changed, 135 insertions(+), 78 deletions(-)
> 
> -- 
> 2.47.0