diff mbox series

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

Message ID 20241127230430.139639-2-mwilck@suse.com (mailing list archive)
State Not Applicable, archived
Delegated to: Benjamin Marzinski
Headers show
Series multipath-tools fixes | expand

Commit Message

Martin Wilck Nov. 27, 2024, 11:04 p.m. UTC
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 mbox series

Patch

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);
 }