DM-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: mwilck@suse.com
To: Christophe Varoqui <christophe.varoqui@opensvc.com>,
	Benjamin Marzinski <bmarzins@redhat.com>
Cc: dm-devel@redhat.com, Martin Wilck <mwilck@suse.com>
Subject: [dm-devel] [PATCH v2 02/12] libmultipath: copy mpp->hwe from pp->hwe
Date: Fri, 16 Oct 2020 12:42:29 +0200
Message-ID: <20201016104239.8217-3-mwilck@suse.com> (raw)
In-Reply-To: <20201016104239.8217-1-mwilck@suse.com>

From: Martin Wilck <mwilck@suse.com>

Since f0462f0 ("libmultipath: use vector for for pp->hwe and mp->hwe"),
we've been trying to fix issues caused by paths getting freed and mpp->hwe
dangling. This approach couldn't work because we need mpp->hwe to persist,
even if all paths are removed from the map. Before f0462f0, a simple
assignment worked, because the lifetime of the hwe wasn't bound to the
path. But now, we need to copy the vector. It turns out that we need to set
mpp->hwe only in two places, add_map_with_path() and setup_map(), and
that the code is simplified overall.

Even now, it can happen that a map is added with add_map_without_paths(),
and has no paths. In that case, calling do_set_from_hwe() with a NULL
pointer is not a bug, so remove the message.

Fixes: f0462f0 ("libmultipath: use vector for for pp->hwe and mp->hwe")
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c   |  7 +++++
 libmultipath/propsel.c     |  4 +--
 libmultipath/structs.c     | 15 +++++++++++
 libmultipath/structs.h     |  1 +
 libmultipath/structs_vec.c | 54 ++++++++++++++++----------------------
 multipathd/main.c          | 10 -------
 6 files changed, 46 insertions(+), 45 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 6fb477f..d7afc91 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -311,6 +311,13 @@ int setup_map(struct multipath *mpp, char *params, int params_size,
 	if (mpp->disable_queueing && VECTOR_SIZE(mpp->paths) != 0)
 		mpp->disable_queueing = 0;
 
+	/*
+	 * If this map was created with add_map_without_path(),
+	 * mpp->hwe might not be set yet.
+	 */
+	if (!mpp->hwe)
+		extract_hwe_from_path(mpp);
+
 	/*
 	 * properties selectors
 	 *
diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index 7e6e0d6..4020134 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -65,9 +65,7 @@ do {									\
 	__do_set_from_vec(struct hwentry, var, (src)->hwe, dest)
 
 #define do_set_from_hwe(var, src, dest, msg)				\
-	if (!src->hwe) {						\
-		condlog(0, "BUG: do_set_from_hwe called with hwe == NULL"); \
-	} else if (__do_set_from_hwe(var, src, dest)) {			\
+	if (src->hwe && __do_set_from_hwe(var, src, dest)) {		\
 		origin = msg;						\
 		goto out;						\
 	}
diff --git a/libmultipath/structs.c b/libmultipath/structs.c
index 464596f..2efad6f 100644
--- a/libmultipath/structs.c
+++ b/libmultipath/structs.c
@@ -234,6 +234,17 @@ alloc_multipath (void)
 	return mpp;
 }
 
+void *set_mpp_hwe(struct multipath *mpp, const struct path *pp)
+{
+	if (!mpp || !pp || !pp->hwe)
+		return NULL;
+	if (mpp->hwe)
+		return mpp->hwe;
+	mpp->hwe = vector_convert(NULL, pp->hwe,
+				  struct hwentry, identity);
+	return mpp->hwe;
+}
+
 void free_multipath_attributes(struct multipath *mpp)
 {
 	if (!mpp)
@@ -290,6 +301,10 @@ free_multipath (struct multipath * mpp, enum free_path_mode free_paths)
 
 	free_pathvec(mpp->paths, free_paths);
 	free_pgvec(mpp->pg, free_paths);
+	if (mpp->hwe) {
+		vector_free(mpp->hwe);
+		mpp->hwe = NULL;
+	}
 	FREE_PTR(mpp->mpcontext);
 	FREE(mpp);
 }
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 7de93d6..4ce3055 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -422,6 +422,7 @@ struct host_group {
 struct path * alloc_path (void);
 struct pathgroup * alloc_pathgroup (void);
 struct multipath * alloc_multipath (void);
+void *set_mpp_hwe(struct multipath *mpp, const struct path *pp);
 void uninitialize_path(struct path *pp);
 void free_path (struct path *);
 void free_pathvec (vector vec, enum free_path_mode free_paths);
diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 8895fa7..f7f45f1 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -294,11 +294,6 @@ err:
 void orphan_path(struct path *pp, const char *reason)
 {
 	condlog(3, "%s: orphan path, %s", pp->dev, reason);
-	if (pp->mpp && pp->hwe && pp->mpp->hwe == pp->hwe) {
-		condlog(0, "BUG: orphaning path %s that holds hwe of %s",
-			pp->dev, pp->mpp->alias);
-		pp->mpp->hwe = NULL;
-	}
 	pp->mpp = NULL;
 	uninitialize_path(pp);
 }
@@ -308,8 +303,6 @@ void orphan_paths(vector pathvec, struct multipath *mpp, const char *reason)
 	int i;
 	struct path * pp;
 
-	/* Avoid BUG message from orphan_path() */
-	mpp->hwe = NULL;
 	vector_foreach_slot (pathvec, pp, i) {
 		if (pp->mpp == mpp) {
 			if (pp->initialized == INIT_REMOVED) {
@@ -397,24 +390,26 @@ extract_hwe_from_path(struct multipath * mpp)
 	if (mpp->hwe || !mpp->paths)
 		return;
 
-	condlog(3, "%s: searching paths for valid hwe", mpp->alias);
+	condlog(4, "%s: searching paths for valid hwe", mpp->alias);
 	/* doing this in two passes seems like paranoia to me */
 	vector_foreach_slot(mpp->paths, pp, i) {
-		if (pp->state != PATH_UP)
-			continue;
-		if (pp->hwe) {
-			mpp->hwe = pp->hwe;
-			return;
-		}
+		if (pp->state == PATH_UP &&
+		    pp->initialized != INIT_REMOVED && pp->hwe)
+			goto done;
 	}
 	vector_foreach_slot(mpp->paths, pp, i) {
-		if (pp->state == PATH_UP)
-			continue;
-		if (pp->hwe) {
-			mpp->hwe = pp->hwe;
-			return;
-		}
+		if (pp->state != PATH_UP &&
+		    pp->initialized != INIT_REMOVED && pp->hwe)
+			goto done;
 	}
+done:
+	if (i < VECTOR_SIZE(mpp->paths))
+		(void)set_mpp_hwe(mpp, pp);
+
+	if (mpp->hwe)
+		condlog(3, "%s: got hwe from path %s", mpp->alias, pp->dev);
+	else
+		condlog(2, "%s: no hwe found", mpp->alias);
 }
 
 int
@@ -514,8 +509,6 @@ void sync_paths(struct multipath *mpp, vector pathvec)
 		}
 		if (!found) {
 			condlog(3, "%s dropped path %s", mpp->alias, pp->dev);
-			if (mpp->hwe == pp->hwe)
-				mpp->hwe = NULL;
 			vector_del_slot(mpp->paths, i--);
 			orphan_path(pp, "path removed externally");
 		}
@@ -524,8 +517,6 @@ void sync_paths(struct multipath *mpp, vector pathvec)
 	update_mpp_paths(mpp, pathvec);
 	vector_foreach_slot (mpp->paths, pp, i)
 		pp->mpp = mpp;
-	if (mpp->hwe == NULL)
-		extract_hwe_from_path(mpp);
 }
 
 int
@@ -701,9 +692,15 @@ struct multipath *add_map_with_path(struct vectors *vecs, struct path *pp,
 
 	conf = get_multipath_config();
 	mpp->mpe = find_mpe(conf->mptable, pp->wwid);
-	mpp->hwe = pp->hwe;
 	put_multipath_config(conf);
 
+	/*
+	 * We need to call this before select_alias(),
+	 * because that accesses hwe properties.
+	 */
+	if (pp->hwe && !set_mpp_hwe(mpp, pp))
+		goto out;
+
 	strcpy(mpp->wwid, pp->wwid);
 	find_existing_alias(mpp, vecs);
 	if (select_alias(conf, mpp))
@@ -754,12 +751,6 @@ int verify_paths(struct multipath *mpp)
 			vector_del_slot(mpp->paths, i);
 			i--;
 
-			/* Make sure mpp->hwe doesn't point to freed memory.
-			 * We call extract_hwe_from_path() below to restore
-			 * mpp->hwe
-			 */
-			if (mpp->hwe == pp->hwe)
-				mpp->hwe = NULL;
 			/*
 			 * Don't delete path from pathvec yet. We'll do this
 			 * after the path has been removed from the map, in
@@ -771,7 +762,6 @@ int verify_paths(struct multipath *mpp)
 				mpp->alias, pp->dev, pp->dev_t);
 		}
 	}
-	extract_hwe_from_path(mpp);
 	return count;
 }
 
diff --git a/multipathd/main.c b/multipathd/main.c
index a4abbb2..eedc6c1 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1153,13 +1153,6 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
 		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
-		 */
-		if (mpp->hwe == pp->hwe)
-			mpp->hwe = NULL;
-
 		/*
 		 * remove the map IF removing the last path
 		 */
@@ -1191,9 +1184,6 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
 			 */
 		}
 
-		if (mpp->hwe == NULL)
-			extract_hwe_from_path(mpp);
-
 		if (setup_map(mpp, params, PARAMS_SIZE, vecs)) {
 			condlog(0, "%s: failed to setup map for"
 				" removal of path %s", mpp->alias, pp->dev);
-- 
2.28.0


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


  parent reply index

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-16 10:42 [dm-devel] [PATCH v2 00/12] multipath-tools: add linker version scripts mwilck
2020-10-16 10:42 ` [dm-devel] [PATCH v2 01/12] libmultipath: find_mpe(): don't match with empty WWID mwilck
2020-10-16 10:42 ` mwilck [this message]
2020-10-16 10:42 ` [dm-devel] [PATCH v2 03/12] libmultipath: dm_map_present_by_uuid(): fix dm_task_create() call mwilck
2020-10-16 10:42 ` [dm-devel] [PATCH v2 04/12] libdmmp tests: fix compilation mwilck
2020-10-16 10:42 ` [dm-devel] [PATCH v2 05/12] libmultipath: prio: constify some function parameters mwilck
2020-10-16 10:42 ` [dm-devel] [PATCH v2 06/12] libmultipath: checkers/prio: allow non-lazy .so loading mwilck
2020-10-16 10:42 ` [dm-devel] [PATCH v2 07/12] multipath-tools Makefiles: separate rules for .so and man pages mwilck
2020-10-16 10:42 ` [dm-devel] [PATCH v2 08/12] libmultipath: create separate .so for unit tests mwilck
2020-10-16 21:04   ` Benjamin Marzinski
2020-10-16 10:42 ` [dm-devel] [PATCH v2 09/12] libmultipath: add linker version script mwilck
2020-10-16 10:42 ` [dm-devel] [PATCH v2 10/12] libmpathpersist: " mwilck
2020-10-16 10:42 ` [dm-devel] [PATCH v2 11/12] libmpathcmd: " mwilck
2020-10-16 10:42 ` [dm-devel] [PATCH v2 12/12] libmpathpersist: initialize mpp->hwe in get_mpvec() mwilck
2020-10-16 22:13   ` Benjamin Marzinski

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201016104239.8217-3-mwilck@suse.com \
    --to=mwilck@suse.com \
    --cc=bmarzins@redhat.com \
    --cc=christophe.varoqui@opensvc.com \
    --cc=dm-devel@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DM-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dm-devel/0 dm-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dm-devel dm-devel/ https://lore.kernel.org/dm-devel \
		dm-devel@redhat.com
	public-inbox-index dm-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/com.redhat.dm-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git