dm-devel.redhat.com archive mirror
 help / color / mirror / Atom feed
* [dm-devel] [PATCH 0/6] multipath: persistent reservation fixes
@ 2022-12-20 23:41 Benjamin Marzinski
  2022-12-20 23:41 ` [dm-devel] [PATCH 1/6] multipathd: make pr registration consistent Benjamin Marzinski
                   ` (6 more replies)
  0 siblings, 7 replies; 9+ messages in thread
From: Benjamin Marzinski @ 2022-12-20 23:41 UTC (permalink / raw)
  To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck

The first three patches fix issues with multipathd's persistent
reservation handling. As long as mpathpersist is run on a multipath
device while multipathd is montioring it, everything works fine. But if
new paths to that device appear or first become active while multipathd
isn't running or the multipath device doesn't exist, multipathd might
not register keys to those paths when it starts up or adds the multipath
device.

These patches also fix an issue where, if there are no active paths for
multipathd to use for checking registered keys on device creation, it
will treat the device as if there are no registered keys.

The other three patches fix issues I found while looking into the
persistent reservation problems.

Benjamin Marzinski (6):
  multipathd: make pr registration consistent
  libmultipath: make prflag an enum
  multipathd: handle no active paths in update_map_pr
  multipathd: add missing newline to cli_del_map reply
  libmultipath: skip extra vector work in remove_maps
  libmultipath: orphan paths if coalesce_paths frees newmp

 libmpathpersist/mpath_persist_int.c | 10 ++++-
 libmultipath/configure.c            |  7 +++-
 libmultipath/structs.h              |  9 ++++-
 libmultipath/structs_vec.c          |  6 +--
 multipathd/cli_handlers.c           | 20 +++++-----
 multipathd/main.c                   | 60 ++++++++++++++++++++---------
 6 files changed, 77 insertions(+), 35 deletions(-)

-- 
2.17.2

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [dm-devel] [PATCH 1/6] multipathd: make pr registration consistent
  2022-12-20 23:41 [dm-devel] [PATCH 0/6] multipath: persistent reservation fixes Benjamin Marzinski
@ 2022-12-20 23:41 ` Benjamin Marzinski
  2022-12-20 23:41 ` [dm-devel] [PATCH 2/6] libmultipath: make prflag an enum Benjamin Marzinski
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 9+ messages in thread
From: Benjamin Marzinski @ 2022-12-20 23:41 UTC (permalink / raw)
  To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck

multipathd was inconsistent on what it did with persistent reservations
when a multipath device was created. If a multipath device with a
configured reservation key was created during configure(), multipathd
would try to read the registered keys using an active path. If it saw a
matching key, it would set the prflag, but not attempt to register the
key on any of the other paths.  This means that if a new path had
appeared while multipathd was not running, it wouldn't register the key
on this path.

If the multipath device was created during ev_add_path(), multipathd
would used the added path to check if there was a matching key and if
there was, register the key only on the added path and then set the
prflag. This could be problematic if the device was created with
multiple paths, for instance because find_mutipaths was set to "yes" and
a second path just appeared. In this case, if the device happened to be
only registered on the second path, it would not get registered on the
first path.

If the multipath device was added to multipathd during a call to
ev_add_map(), multipathd wouldn't set the prflag or register the key on
any paths.

After a device was created with the prflag set, if a new path appeared
before the creation uevent, and multipathd was forced to delay adding
it, when it finally updated the multipath device, the key would be
registered on all paths, fixing any paths missed during creation.
However, if a new path appeared after the creation uevent, the key would
only be registered on that new path. Any paths that were missed on
creation would stay missed.

persistent key registration needs to be handled consistently.  This
patch does so by making sure that however a multipath device is added to
multipathd, it will check to see if the configured key is registered. If
it is, multipathd will set the prflag and register the key on all the
currently active paths.

When a new path is added, multipathd will use it to check for active
keys, as before. But if it finds a matching key and prflag isn't
currently set, it will register the key on all paths.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 43 +++++++++++++++++++++++++++++--------------
 1 file changed, 29 insertions(+), 14 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 1e1b254f..f7212d7b 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -586,13 +586,26 @@ flush_map_nopaths(struct multipath *mpp, struct vectors *vecs) {
 	return false;
 }
 
+static void
+pr_register_active_paths(struct multipath *mpp)
+{
+	unsigned int i, j;
+	struct path *pp;
+	struct pathgroup *pgp;
+
+	vector_foreach_slot (mpp->pg, pgp, i) {
+		vector_foreach_slot (pgp->paths, pp, j) {
+			if ((pp->state == PATH_UP) || (pp->state == PATH_GHOST))
+				mpath_pr_event_handle(pp);
+		}
+	}
+}
+
 static int
 update_map (struct multipath *mpp, struct vectors *vecs, int new_map)
 {
 	int retries = 3;
 	char *params __attribute__((cleanup(cleanup_charp))) = NULL;
-	struct path *pp;
-	int i;
 
 retry:
 	condlog(4, "%s: updating new map", mpp->alias);
@@ -609,15 +622,6 @@ retry:
 
 	mpp->action = ACT_RELOAD;
 
-	if (mpp->prflag) {
-		vector_foreach_slot(mpp->paths, pp, i) {
-			if ((pp->state == PATH_UP)  || (pp->state == PATH_GHOST)) {
-				/* persistent reservation check*/
-				mpath_pr_event_handle(pp);
-			}
-		}
-	}
-
 	if (setup_map(mpp, &params, vecs)) {
 		condlog(0, "%s: failed to setup new map in update", mpp->alias);
 		retries = -1;
@@ -643,6 +647,11 @@ fail:
 
 	sync_map_state(mpp);
 
+	if (!mpp->prflag)
+		update_map_pr(mpp);
+	if (mpp->prflag)
+		pr_register_active_paths(mpp);
+
 	if (retries < 0)
 		condlog(0, "%s: failed reload in new map update", mpp->alias);
 	return 0;
@@ -1191,6 +1200,7 @@ ev_add_path (struct path * pp, struct vectors * vecs, int need_do_map)
 	int start_waiter = 0;
 	int ret;
 	int ro;
+	unsigned char prflag = 0;
 
 	/*
 	 * need path UID to go any further
@@ -1234,6 +1244,8 @@ rescan:
 
 		verify_paths(mpp);
 		mpp->action = ACT_RELOAD;
+		prflag = mpp->prflag;
+		mpath_pr_event_handle(pp);
 	} else {
 		if (!should_multipath(pp, vecs->pathvec, vecs->mpvec)) {
 			orphan_path(pp, "only one path");
@@ -1252,9 +1264,6 @@ rescan:
 			goto fail; /* leave path added to pathvec */
 	}
 
-	/* persistent reservation check*/
-	mpath_pr_event_handle(pp);
-
 	/* ro check - if new path is ro, force map to be ro as well */
 	ro = sysfs_get_ro(pp);
 	if (ro == 1)
@@ -1319,6 +1328,10 @@ rescan:
 	sync_map_state(mpp);
 
 	if (retries >= 0) {
+		if (start_waiter)
+			update_map_pr(mpp);
+		if (mpp->prflag && !prflag)
+				pr_register_active_paths(mpp);
 		condlog(2, "%s [%s]: path added to devmap %s",
 			pp->dev, pp->dev_t, mpp->alias);
 		return 0;
@@ -2852,6 +2865,8 @@ configure (struct vectors * vecs, enum force_reload_types reload_type)
 		if (remember_wwid(mpp->wwid) == 1)
 			trigger_paths_udev_change(mpp, true);
 		update_map_pr(mpp);
+		if (mpp->prflag)
+			pr_register_active_paths(mpp);
 	}
 
 	/*
-- 
2.17.2

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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [dm-devel] [PATCH 2/6] libmultipath: make prflag an enum
  2022-12-20 23:41 [dm-devel] [PATCH 0/6] multipath: persistent reservation fixes Benjamin Marzinski
  2022-12-20 23:41 ` [dm-devel] [PATCH 1/6] multipathd: make pr registration consistent Benjamin Marzinski
@ 2022-12-20 23:41 ` Benjamin Marzinski
  2023-01-25 10:40   ` Martin Wilck
  2022-12-20 23:41 ` [dm-devel] [PATCH 3/6] multipathd: handle no active paths in update_map_pr Benjamin Marzinski
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 9+ messages in thread
From: Benjamin Marzinski @ 2022-12-20 23:41 UTC (permalink / raw)
  To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck

In preparation for a future patch, make prflag an enum, and change the
reply of cli_getprstatus() to a string.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmpathpersist/mpath_persist_int.c |  2 +-
 libmultipath/structs.h              |  8 +++++++-
 multipathd/cli_handlers.c           | 17 +++++++++--------
 multipathd/main.c                   | 14 +++++++-------
 4 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/libmpathpersist/mpath_persist_int.c b/libmpathpersist/mpath_persist_int.c
index 6924b379..a84d9474 100644
--- a/libmpathpersist/mpath_persist_int.c
+++ b/libmpathpersist/mpath_persist_int.c
@@ -783,7 +783,7 @@ int update_map_pr(struct multipath *mpp)
 
 	if (isFound)
 	{
-		mpp->prflag = 1;
+		mpp->prflag = PRFLAG_SET;
 		condlog(2, "%s: prflag flag set.", mpp->alias );
 	}
 
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 9e2c1ab0..f2265300 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -375,6 +375,12 @@ struct path {
 
 typedef int (pgpolicyfn) (struct multipath *, vector);
 
+
+enum prflag_value {
+	PRFLAG_UNSET,
+	PRFLAG_SET,
+};
+
 struct multipath {
 	char wwid[WWID_SIZE];
 	char alias_old[WWID_SIZE];
@@ -449,7 +455,7 @@ struct multipath {
 	int prkey_source;
 	struct be64 reservation_key;
 	uint8_t sa_flags;
-	unsigned char prflag;
+	int prflag;
 	int all_tg_pt;
 	struct gen_multipath generic_mp;
 	bool fpin_must_reload;
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index e65fb75c..7ee2729f 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -1277,6 +1277,10 @@ cli_shutdown (void * v, struct strbuf *reply, void * data)
 static int
 cli_getprstatus (void * v, struct strbuf *reply, void * data)
 {
+	static const char * const prflag_str[] = {
+		[PRFLAG_UNSET] = "unset\n",
+		[PRFLAG_SET] = "set\n",
+	};
 	struct multipath * mpp;
 	struct vectors * vecs = (struct vectors *)data;
 	char * param = get_keyparam(v, KEY_MAP);
@@ -1287,10 +1291,7 @@ cli_getprstatus (void * v, struct strbuf *reply, void * data)
 	if (!mpp)
 		return 1;
 
-	condlog(3, "%s: prflag = %u", param, (unsigned int)mpp->prflag);
-
-	if (print_strbuf(reply, "%d", mpp->prflag) < 0)
-		return 1;
+	append_strbuf_str(reply, prflag_str[mpp->prflag]);
 
 	condlog(3, "%s: reply = %s", param, get_strbuf_str(reply));
 
@@ -1310,8 +1311,8 @@ cli_setprstatus(void * v, struct strbuf *reply, void * data)
 	if (!mpp)
 		return 1;
 
-	if (!mpp->prflag) {
-		mpp->prflag = 1;
+	if (mpp->prflag != PRFLAG_SET) {
+		mpp->prflag = PRFLAG_SET;
 		condlog(2, "%s: prflag set", param);
 	}
 
@@ -1332,8 +1333,8 @@ cli_unsetprstatus(void * v, struct strbuf *reply, void * data)
 	if (!mpp)
 		return 1;
 
-	if (mpp->prflag) {
-		mpp->prflag = 0;
+	if (mpp->prflag != PRFLAG_UNSET) {
+		mpp->prflag = PRFLAG_UNSET;
 		condlog(2, "%s: prflag unset", param);
 	}
 
diff --git a/multipathd/main.c b/multipathd/main.c
index f7212d7b..722235c7 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -647,9 +647,9 @@ fail:
 
 	sync_map_state(mpp);
 
-	if (!mpp->prflag)
+	if (mpp->prflag == PRFLAG_UNSET)
 		update_map_pr(mpp);
-	if (mpp->prflag)
+	if (mpp->prflag == PRFLAG_SET)
 		pr_register_active_paths(mpp);
 
 	if (retries < 0)
@@ -1200,7 +1200,7 @@ ev_add_path (struct path * pp, struct vectors * vecs, int need_do_map)
 	int start_waiter = 0;
 	int ret;
 	int ro;
-	unsigned char prflag = 0;
+	unsigned char prflag = PRFLAG_UNSET;
 
 	/*
 	 * need path UID to go any further
@@ -1330,7 +1330,7 @@ rescan:
 	if (retries >= 0) {
 		if (start_waiter)
 			update_map_pr(mpp);
-		if (mpp->prflag && !prflag)
+		if (mpp->prflag == PRFLAG_SET && prflag == PRFLAG_UNSET)
 				pr_register_active_paths(mpp);
 		condlog(2, "%s [%s]: path added to devmap %s",
 			pp->dev, pp->dev_t, mpp->alias);
@@ -2492,7 +2492,7 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 		}
 
 		if (newstate == PATH_UP || newstate == PATH_GHOST) {
-			if (pp->mpp->prflag) {
+			if (pp->mpp->prflag == PRFLAG_SET) {
 				/*
 				 * Check Persistent Reservation.
 				 */
@@ -2865,7 +2865,7 @@ configure (struct vectors * vecs, enum force_reload_types reload_type)
 		if (remember_wwid(mpp->wwid) == 1)
 			trigger_paths_udev_change(mpp, true);
 		update_map_pr(mpp);
-		if (mpp->prflag)
+		if (mpp->prflag == PRFLAG_SET)
 			pr_register_active_paths(mpp);
 	}
 
@@ -3840,7 +3840,7 @@ void *  mpath_pr_event_handler_fn (void * pathp )
 	{
 		condlog(0,"%s: Reservation registration failed. Error: %d", pp->dev, ret);
 	}
-	mpp->prflag = 1;
+	mpp->prflag = PRFLAG_SET;
 
 	free(param);
 out:
-- 
2.17.2

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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [dm-devel] [PATCH 3/6] multipathd: handle no active paths in update_map_pr
  2022-12-20 23:41 [dm-devel] [PATCH 0/6] multipath: persistent reservation fixes Benjamin Marzinski
  2022-12-20 23:41 ` [dm-devel] [PATCH 1/6] multipathd: make pr registration consistent Benjamin Marzinski
  2022-12-20 23:41 ` [dm-devel] [PATCH 2/6] libmultipath: make prflag an enum Benjamin Marzinski
@ 2022-12-20 23:41 ` Benjamin Marzinski
  2022-12-20 23:41 ` [dm-devel] [PATCH 4/6] multipathd: add missing newline to cli_del_map reply Benjamin Marzinski
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 9+ messages in thread
From: Benjamin Marzinski @ 2022-12-20 23:41 UTC (permalink / raw)
  To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck

When a multipath device is first created, if it has a reservation key
configured, update_map_pr() will check for a matching key on the active
paths. If there were no active paths to check with, multipathd was
leaving mpp->prflag in PRFLAG_UNSET, as if there were no matching keys.
It's possible that when update_map_pr() is called, all the paths will be
in the PATH_PENDING state because the checkers haven't completed yet. In
this case, multipathd was treating the device as having no registered
keys without ever checking.

To solve this, multipath devices now start with prflag = PRFLAG_UNKNOWN.
It will remain in this state until multipathd actually tries to get the
registered keys down a path. If the map is in this state, it will check
newly active paths, and if it finds a matching key, it will register
the key down all active paths.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmpathpersist/mpath_persist_int.c |  8 ++++++++
 libmultipath/structs.h              |  1 +
 multipathd/cli_handlers.c           |  1 +
 multipathd/main.c                   | 19 ++++++++++++++-----
 4 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/libmpathpersist/mpath_persist_int.c b/libmpathpersist/mpath_persist_int.c
index a84d9474..8b52b746 100644
--- a/libmpathpersist/mpath_persist_int.c
+++ b/libmpathpersist/mpath_persist_int.c
@@ -738,6 +738,7 @@ int update_map_pr(struct multipath *mpp)
 	if (!get_be64(mpp->reservation_key))
 	{
 		/* Nothing to do. Assuming pr mgmt feature is disabled*/
+		mpp->prflag = PRFLAG_UNSET;
 		condlog(4, "%s: reservation_key not set in multipath.conf",
 			mpp->alias);
 		return MPATH_PR_SUCCESS;
@@ -749,6 +750,13 @@ int update_map_pr(struct multipath *mpp)
 		condlog(0,"%s : failed to alloc resp in update_map_pr", mpp->alias);
 		return MPATH_PR_OTHER;
 	}
+	if (count_active_paths(mpp) == 0)
+	{
+		condlog(0,"%s: No available paths to check pr status",
+			mpp->alias);
+		return MPATH_PR_OTHER;
+	}
+	mpp->prflag = PRFLAG_UNSET;
 	ret = mpath_prin_activepath(mpp, MPATH_PRIN_RKEY_SA, resp, noisy);
 
 	if (ret != MPATH_PR_SUCCESS )
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index f2265300..e2294323 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -377,6 +377,7 @@ typedef int (pgpolicyfn) (struct multipath *, vector);
 
 
 enum prflag_value {
+	PRFLAG_UNKNOWN,
 	PRFLAG_UNSET,
 	PRFLAG_SET,
 };
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 7ee2729f..ec5db1b8 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -1278,6 +1278,7 @@ static int
 cli_getprstatus (void * v, struct strbuf *reply, void * data)
 {
 	static const char * const prflag_str[] = {
+		[PRFLAG_UNKNOWN] = "unknown\n",
 		[PRFLAG_UNSET] = "unset\n",
 		[PRFLAG_SET] = "set\n",
 	};
diff --git a/multipathd/main.c b/multipathd/main.c
index 722235c7..bdeffe76 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -647,7 +647,7 @@ fail:
 
 	sync_map_state(mpp);
 
-	if (mpp->prflag == PRFLAG_UNSET)
+	if (mpp->prflag != PRFLAG_SET)
 		update_map_pr(mpp);
 	if (mpp->prflag == PRFLAG_SET)
 		pr_register_active_paths(mpp);
@@ -1330,7 +1330,7 @@ rescan:
 	if (retries >= 0) {
 		if (start_waiter)
 			update_map_pr(mpp);
-		if (mpp->prflag == PRFLAG_SET && prflag == PRFLAG_UNSET)
+		if (mpp->prflag == PRFLAG_SET && prflag != PRFLAG_SET)
 				pr_register_active_paths(mpp);
 		condlog(2, "%s [%s]: path added to devmap %s",
 			pp->dev, pp->dev_t, mpp->alias);
@@ -2492,13 +2492,17 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 		}
 
 		if (newstate == PATH_UP || newstate == PATH_GHOST) {
-			if (pp->mpp->prflag == PRFLAG_SET) {
+			if (pp->mpp->prflag != PRFLAG_UNSET) {
+				int prflag = pp->mpp->prflag;
 				/*
 				 * Check Persistent Reservation.
 				 */
 				condlog(2, "%s: checking persistent "
 					"reservation registration", pp->dev);
 				mpath_pr_event_handle(pp);
+				if (pp->mpp->prflag == PRFLAG_SET &&
+				    prflag != PRFLAG_SET)
+					pr_register_active_paths(pp->mpp);
 			}
 		}
 
@@ -3788,6 +3792,7 @@ void *  mpath_pr_event_handler_fn (void * pathp )
 		goto out;
 	}
 
+	mpp->prflag = PRFLAG_UNSET;
 	ret = prin_do_scsi_ioctl(pp->dev, MPATH_PRIN_RKEY_SA, resp, 0);
 	if (ret != MPATH_PR_SUCCESS )
 	{
@@ -3858,12 +3863,12 @@ int mpath_pr_event_handle(struct path *pp)
 	struct multipath * mpp;
 
 	if (pp->bus != SYSFS_BUS_SCSI)
-		return 0;
+		goto no_pr;
 
 	mpp = pp->mpp;
 
 	if (!get_be64(mpp->reservation_key))
-		return -1;
+		goto no_pr;
 
 	pthread_attr_init(&attr);
 	pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_JOINABLE);
@@ -3876,4 +3881,8 @@ int mpath_pr_event_handle(struct path *pp)
 	pthread_attr_destroy(&attr);
 	rc = pthread_join(thread, NULL);
 	return 0;
+
+no_pr:
+	pp->mpp->prflag = PRFLAG_UNSET;
+	return 0;
 }
-- 
2.17.2

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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [dm-devel] [PATCH 4/6] multipathd: add missing newline to cli_del_map reply
  2022-12-20 23:41 [dm-devel] [PATCH 0/6] multipath: persistent reservation fixes Benjamin Marzinski
                   ` (2 preceding siblings ...)
  2022-12-20 23:41 ` [dm-devel] [PATCH 3/6] multipathd: handle no active paths in update_map_pr Benjamin Marzinski
@ 2022-12-20 23:41 ` Benjamin Marzinski
  2022-12-20 23:41 ` [dm-devel] [PATCH 5/6] libmultipath: skip extra vector work in remove_maps Benjamin Marzinski
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 9+ messages in thread
From: Benjamin Marzinski @ 2022-12-20 23:41 UTC (permalink / raw)
  To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/cli_handlers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index ec5db1b8..44bf43df 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -760,7 +760,7 @@ cli_del_map (void * v, struct strbuf *reply, void * data)
 	}
 	rc = ev_remove_map(param, alias, minor, vecs);
 	if (rc == 2)
-		append_strbuf_str(reply, "delayed");
+		append_strbuf_str(reply, "delayed\n");
 
 	free(alias);
 	return rc;
-- 
2.17.2

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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [dm-devel] [PATCH 5/6] libmultipath: skip extra vector work in remove_maps
  2022-12-20 23:41 [dm-devel] [PATCH 0/6] multipath: persistent reservation fixes Benjamin Marzinski
                   ` (3 preceding siblings ...)
  2022-12-20 23:41 ` [dm-devel] [PATCH 4/6] multipathd: add missing newline to cli_del_map reply Benjamin Marzinski
@ 2022-12-20 23:41 ` Benjamin Marzinski
  2022-12-20 23:41 ` [dm-devel] [PATCH 6/6] libmultipath: orphan paths if coalesce_paths frees newmp Benjamin Marzinski
  2022-12-21 20:42 ` [dm-devel] [PATCH 0/6] multipath: persistent reservation fixes Martin Wilck
  6 siblings, 0 replies; 9+ messages in thread
From: Benjamin Marzinski @ 2022-12-20 23:41 UTC (permalink / raw)
  To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck

Instead of repeatedly removing the first vector element, and shifting
the rest to fill in, call remove_map() without a vector, so it just
frees the devices. The vector will be completely cleaned up by
vector_free() immediately afterwards.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/structs_vec.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 5a618767..f3fdc5a6 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -392,10 +392,8 @@ remove_maps(struct vectors * vecs)
 	if (!vecs)
 		return;
 
-	vector_foreach_slot (vecs->mpvec, mpp, i) {
-		remove_map(mpp, vecs->pathvec, vecs->mpvec);
-		i--;
-	}
+	vector_foreach_slot (vecs->mpvec, mpp, i)
+		remove_map(mpp, vecs->pathvec, NULL);
 
 	vector_free(vecs->mpvec);
 	vecs->mpvec = NULL;
-- 
2.17.2

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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [dm-devel] [PATCH 6/6] libmultipath: orphan paths if coalesce_paths frees newmp
  2022-12-20 23:41 [dm-devel] [PATCH 0/6] multipath: persistent reservation fixes Benjamin Marzinski
                   ` (4 preceding siblings ...)
  2022-12-20 23:41 ` [dm-devel] [PATCH 5/6] libmultipath: skip extra vector work in remove_maps Benjamin Marzinski
@ 2022-12-20 23:41 ` Benjamin Marzinski
  2022-12-21 20:42 ` [dm-devel] [PATCH 0/6] multipath: persistent reservation fixes Martin Wilck
  6 siblings, 0 replies; 9+ messages in thread
From: Benjamin Marzinski @ 2022-12-20 23:41 UTC (permalink / raw)
  To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck

If coalesce_paths() is called without a mpvec, it will free all the
multipath devices on newmp at the end. This will clear pp->mpp from the
path, but it doesn't completely unitialize them. cli_add_map() can call
coalsce_paths() this way, when adding a device that doesn't currently
exist. cli_add_map() first creates the device in the kernel, and then
calls ev_add_map() to add it to multipathd. If something goes wrong in
ev_add_map(), the paths will still be initialized, even though they're
orphans.

Fix this by calling remove_map() to orphan the paths that belong to
the multipath devices being deleted by coalesce_paths().

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/configure.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index e551047a..e689f8a7 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -1273,8 +1273,11 @@ int coalesce_paths (struct vectors *vecs, vector mpvec, char *refwwid,
 	ret = CP_OK;
 out:
 	free(size_mismatch_seen);
-	if (!mpvec)
-		free_multipathvec(newmp, KEEP_PATHS);
+	if (!mpvec) {
+		vector_foreach_slot (newmp, mpp, i)
+			remove_map(mpp, vecs->pathvec, NULL);
+		vector_free(newmp);
+	}
 	return ret;
 }
 
-- 
2.17.2

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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [dm-devel] [PATCH 0/6] multipath: persistent reservation fixes
  2022-12-20 23:41 [dm-devel] [PATCH 0/6] multipath: persistent reservation fixes Benjamin Marzinski
                   ` (5 preceding siblings ...)
  2022-12-20 23:41 ` [dm-devel] [PATCH 6/6] libmultipath: orphan paths if coalesce_paths frees newmp Benjamin Marzinski
@ 2022-12-21 20:42 ` Martin Wilck
  6 siblings, 0 replies; 9+ messages in thread
From: Martin Wilck @ 2022-12-21 20:42 UTC (permalink / raw)
  To: bmarzins, christophe.varoqui; +Cc: dm-devel

On Tue, 2022-12-20 at 17:41 -0600, Benjamin Marzinski wrote:
> The first three patches fix issues with multipathd's persistent
> reservation handling. As long as mpathpersist is run on a multipath
> device while multipathd is montioring it, everything works fine. But
> if
> new paths to that device appear or first become active while
> multipathd
> isn't running or the multipath device doesn't exist, multipathd might
> not register keys to those paths when it starts up or adds the
> multipath
> device.
> 
> These patches also fix an issue where, if there are no active paths
> for
> multipathd to use for checking registered keys on device creation, it
> will treat the device as if there are no registered keys.
> 
> The other three patches fix issues I found while looking into the
> persistent reservation problems.
> 
> Benjamin Marzinski (6):
>   multipathd: make pr registration consistent
>   libmultipath: make prflag an enum
>   multipathd: handle no active paths in update_map_pr
>   multipathd: add missing newline to cli_del_map reply
>   libmultipath: skip extra vector work in remove_maps
>   libmultipath: orphan paths if coalesce_paths frees newmp
> 
>  libmpathpersist/mpath_persist_int.c | 10 ++++-
>  libmultipath/configure.c            |  7 +++-
>  libmultipath/structs.h              |  9 ++++-
>  libmultipath/structs_vec.c          |  6 +--
>  multipathd/cli_handlers.c           | 20 +++++-----
>  multipathd/main.c                   | 60 ++++++++++++++++++++-------
> --
>  6 files changed, 77 insertions(+), 35 deletions(-)
> 

For the series:
Reviewed-by: Martin Wilck <mwilck@suse.com>

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [dm-devel] [PATCH 2/6] libmultipath: make prflag an enum
  2022-12-20 23:41 ` [dm-devel] [PATCH 2/6] libmultipath: make prflag an enum Benjamin Marzinski
@ 2023-01-25 10:40   ` Martin Wilck
  0 siblings, 0 replies; 9+ messages in thread
From: Martin Wilck @ 2023-01-25 10:40 UTC (permalink / raw)
  To: bmarzins, christophe.varoqui; +Cc: dm-devel

Hi Ben,

On Tue, 2022-12-20 at 17:41 -0600, Benjamin Marzinski wrote:
> In preparation for a future patch, make prflag an enum, and change
> the
> reply of cli_getprstatus() to a string.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

this patch changes ABI. Thus I pushed 730629d [1] to the "queue" branch
[2].

Martin

[1] https://github.com/openSUSE/multipath-tools/commit/730629d604e4ea30f7902fa347d815cd05a60002
[2] https://github.com/openSUSE/multipath-tools/tree/queue


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


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2023-01-25 10:40 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-20 23:41 [dm-devel] [PATCH 0/6] multipath: persistent reservation fixes Benjamin Marzinski
2022-12-20 23:41 ` [dm-devel] [PATCH 1/6] multipathd: make pr registration consistent Benjamin Marzinski
2022-12-20 23:41 ` [dm-devel] [PATCH 2/6] libmultipath: make prflag an enum Benjamin Marzinski
2023-01-25 10:40   ` Martin Wilck
2022-12-20 23:41 ` [dm-devel] [PATCH 3/6] multipathd: handle no active paths in update_map_pr Benjamin Marzinski
2022-12-20 23:41 ` [dm-devel] [PATCH 4/6] multipathd: add missing newline to cli_del_map reply Benjamin Marzinski
2022-12-20 23:41 ` [dm-devel] [PATCH 5/6] libmultipath: skip extra vector work in remove_maps Benjamin Marzinski
2022-12-20 23:41 ` [dm-devel] [PATCH 6/6] libmultipath: orphan paths if coalesce_paths frees newmp Benjamin Marzinski
2022-12-21 20:42 ` [dm-devel] [PATCH 0/6] multipath: persistent reservation fixes Martin Wilck

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).