[PATCH v3 00/15] multipathd: More map reload handling, and checkerloop work

[PATCH v3 00/15] multipathd: More map reload handling, and checkerloop work

[Martin Wilck]

This patch set goes on top of Ben's set [1] for github issue 105 [2].

Changes v2 -> v3 (all based on the review by Ben Marzinski):

03/15: See 15/15 below
04/15: replaced the previous patch, which decreased the checker interval to
       1 if the map was still inconsistent after reloading, by a warning.
       It's unlikely that this condition will ever occur, but if it does,
       we want to know.
08/15: Re-added the call to switch_pathgroups which had been mistakenly deleted.
15/15: New, compensate the fact that it is now more likely to miss the
       call to enable_group() in update_path_state().

Changes v1 -> v2:

Removed patch 3 and 4 of v1 and replaced them by an alternative approach.
Instead of allowing map and path removal in the checker loop, the kernel sync
is moved towards the end.

Patch 5 ff. in v1 contained a bug in the new checker_finished() function.
If one "tick" function returned true, the other might not be executed any
more. In v2, all tick functions are executed, and the action to be taken is
selected according to the combined results.  Also, we won't call
reload_and_sync_map() when we've already called update_map().

Patch 13 is new in v2. Patch 8 from v1 has been moved after patch 13.

(In the thread following the review of v1, I mistakenly wrote about an
upcoming "v4" of this patch set. That was wrong, I meant this v2 here).

Cover letter of v1:

The first patch implements the remark I had on patch 2 on Ben's set, the 2nd
is a minor cleanup.

Patch 3 moves the sync_mpp() call from the beginning to the end of the
checkerloop, as suggested by Ben in [3]. If an inconsitency is found
(mpp->need_reload), we reload the map and schedule another sync for the
next tick (patch 4).

Patch 5 ff. reshuffle the code in checkerloop(). There is now one function,
checker_finished(), that takes all actions that need to be done with the vecs
lock taken after the checkers have finished. checkerloop() enters this
function immediately when the checkers have finished, without dropping and
re-acquiring the vecs lock. The map reload logic is completely handled in this
function.

The various _tick() functions don't loop over mpvec any more; they are now
just called for a single mpp, and they simply return true if a map reload is
required.  The actual reload action differs: if missing_uev_wait_tick()
requests a reload, it needs to be a full update_map() (which calls
adopt_paths()), whereas in the other cases, reload_and_sync_map() is sufficient.
Patch 12 changes the reload action for the ghost delay tick.

Patch 13 removes maps if that are not found in the kernel any more. This
obsoletes the map garbage collector. Unlike the logic in v1, we won't remove
maps on arbitrary error conditions any more.

Reviews & comments welcome.

Regards
Martin

[1] https://lore.kernel.org/dm-devel/20241205035638.1218953-1-bmarzins@redhat.com/
[2] https://github.com/opensvc/multipath-tools/issues/105
[3] https://lore.kernel.org/dm-devel/Z1iUekRg8sai8HLT@redhat.com/

Martin Wilck (15):
  multipathd: don't reload map in update_mpp_prio()
  multipathd: remove dm_get_info() call from refresh_multipath()
  multipathd: sync maps at end of checkerloop
  multipathd: emit a warning if a map remains inconsistent after reload
  multipathd: move yielding for waiters to start of checkerloop
  multipathd: add checker_finished()
  multipathd: move "tick" calls into checker_finished()
  multipathd: don't call reload_and_sync_map() from
    deferred_failback_tick()
  multipathd: move retry_count_tick() into existing mpvec loop
  multipathd: don't call update_map() from missing_uev_wait_tick()
  multipathd: don't call udpate_map() from ghost_delay_tick()
  multipathd: only call reload_and_sync_map() when ghost delay expires
  multipathd: remove non-existent maps in checkerloop
  multipathd: remove mpvec_garbage_collector()
  multipathd: enable pathgroups in checker_finished()

 libmultipath/structs.h     |   2 +-
 libmultipath/structs_vec.c |   1 -
 multipathd/main.c          | 317 +++++++++++++++++--------------------
 3 files changed, 149 insertions(+), 171 deletions(-)

-- 
2.47.1

[PATCH v3 01/15] multipathd: don't reload map in update_mpp_prio()

[Martin Wilck]

Rather, return a bool indicating whether checkerloop() should call
reload_and_sync_map().

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index d7d4a26..fa684e6 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2689,25 +2689,25 @@ update_path_state (struct vectors * vecs, struct path * pp)
 	return chkr_new_path_up ? CHECK_PATH_NEW_UP : CHECK_PATH_CHECKED;
 }
 
-static int
-update_mpp_prio(struct vectors *vecs, struct multipath *mpp)
+/* Return value: true if the map needs to be reloaded */
+static bool update_mpp_prio(struct multipath *mpp)
 {
 	bool need_reload, changed;
 	enum prio_update_type prio_update = mpp->prio_update;
 	mpp->prio_update = PRIO_UPDATE_NONE;
 
 	if (mpp->wait_for_udev || prio_update == PRIO_UPDATE_NONE)
-		return 0;
+		return false;
 	condlog(4, "prio refresh");
 
 	changed = update_prio(mpp, prio_update != PRIO_UPDATE_NORMAL);
 	if (prio_update == PRIO_UPDATE_MARGINAL)
-		return reload_and_sync_map(mpp, vecs);
+		return true;
 	if (changed && mpp->pgpolicyfn == (pgpolicyfn *)group_by_prio &&
 	    mpp->pgfailback == -FAILBACK_IMMEDIATE) {
 		condlog(2, "%s: path priorities changed. reloading",
 			mpp->alias);
-		return reload_and_sync_map(mpp, vecs);
+		return true;
 	}
 	if (need_switch_pathgroup(mpp, &need_reload)) {
 		if (mpp->pgfailback > 0 &&
@@ -2718,12 +2718,12 @@ update_mpp_prio(struct vectors *vecs, struct multipath *mpp)
 			 (prio_update == PRIO_UPDATE_NEW_PATH &&
 			  followover_should_failback(mpp))) {
 			if (need_reload)
-				return reload_and_sync_map(mpp, vecs);
+				return true;
 			else
 				switch_pathgroup(mpp);
 		}
 	}
-	return 0;
+	return false;
 }
 
 static int
@@ -3040,13 +3040,11 @@ checkerloop (void *ap)
 							     start_time.tv_sec);
 			if (checker_state == CHECKER_FINISHED) {
 				vector_foreach_slot(vecs->mpvec, mpp, i) {
-					if (update_mpp_prio(vecs, mpp) == 2 ||
-					    (mpp->need_reload &&
-					     mpp->synced_count > 0 &&
-					     reload_and_sync_map(mpp, vecs) == 2)) {
+					if ((update_mpp_prio(mpp) ||
+					     (mpp->need_reload && mpp->synced_count > 0)) &&
+					    reload_and_sync_map(mpp, vecs) == 2)
 						/* multipath device deleted */
 						i--;
-					}
 				}
 			}
 			lock_cleanup_pop(vecs->lock);
-- 
2.47.1

[PATCH v3 02/15] multipathd: remove dm_get_info() call from refresh_multipath()

[Martin Wilck]

If the map is inaccessible, update_multipath_strings() will fail.
There is no need for the extra call to dm_get_info().

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index fa684e6..4a28fbb 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -503,20 +503,12 @@ remove_maps_and_stop_waiters(struct vectors *vecs)
 
 int refresh_multipath(struct vectors *vecs, struct multipath *mpp)
 {
-	if (dm_get_info(mpp->alias, &mpp->dmi) != DMP_OK) {
-		/* Error accessing table */
-		condlog(2, "%s: cannot access table", mpp->alias);
-		goto out;
-	}
-
 	if (update_multipath_strings(mpp, vecs->pathvec) != DMP_OK) {
-		condlog(0, "%s: failed to setup multipath", mpp->alias);
-		goto out;
+		condlog(0, "%s: failed to read map from kernel", mpp->alias);
+		remove_map_and_stop_waiter(mpp, vecs);
+		return 1;
 	}
 	return 0;
-out:
-	remove_map_and_stop_waiter(mpp, vecs);
-	return 1;
 }
 
 int setup_multipath(struct vectors *vecs, struct multipath *mpp)
-- 
2.47.1

[PATCH v3 03/15] multipathd: sync maps at end of checkerloop

[Martin Wilck]

Rather than calling sync_mpp early in the checkerloop and tracking
map synchronization with synced_count, call sync_mpp() in the CHECKER_FINISHED
state, if either at least one path of the map has been checked in the current
iteration, or the sync tick has expired. This avoids potentially deleting
paths from the pathvec through the do_sync_mpp() -> update_multipath_strings()
-> sync_paths -> check_removed_paths() call chain while we're iterating over
the pathvec. Also, the time gap between obtaining path states and syncing
the state with the kernel is smaller this way.

Suggested-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/structs.h     |  2 +-
 libmultipath/structs_vec.c |  1 -
 multipathd/main.c          | 26 +++++++++++---------------
 3 files changed, 12 insertions(+), 17 deletions(-)

diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 6a30c59..9d22bdd 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -471,7 +471,7 @@ struct multipath {
 	int ghost_delay_tick;
 	int queue_mode;
 	unsigned int sync_tick;
-	int synced_count;
+	int checker_count;
 	enum prio_update_type prio_update;
 	uid_t uid;
 	gid_t gid;
diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 7a4e3eb..6aa744d 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -530,7 +530,6 @@ update_multipath_table (struct multipath *mpp, vector pathvec, int flags)
 	conf = get_multipath_config();
 	mpp->sync_tick = conf->max_checkint;
 	put_multipath_config(conf);
-	mpp->synced_count++;
 
 	r = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_MPATH_ONLY,
 			  (mapid_t) { .str = mpp->alias },
diff --git a/multipathd/main.c b/multipathd/main.c
index 4a28fbb..e4e6bf7 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2470,7 +2470,7 @@ sync_mpp(struct vectors * vecs, struct multipath *mpp, unsigned int ticks)
 	if (mpp->sync_tick)
 		mpp->sync_tick -= (mpp->sync_tick > ticks) ? ticks :
 				  mpp->sync_tick;
-	if (mpp->sync_tick)
+	if (mpp->sync_tick && !mpp->checker_count)
 		return;
 
 	do_sync_mpp(vecs, mpp);
@@ -2513,12 +2513,6 @@ update_path_state (struct vectors * vecs, struct path * pp)
 		return handle_path_wwid_change(pp, vecs)? CHECK_PATH_REMOVED :
 							  CHECK_PATH_SKIPPED;
 	}
-	if (pp->mpp->synced_count == 0) {
-		do_sync_mpp(vecs, pp->mpp);
-		/* if update_multipath_strings orphaned the path, quit early */
-		if (!pp->mpp)
-			return CHECK_PATH_SKIPPED;
-	}
 	if ((newstate != PATH_UP && newstate != PATH_GHOST &&
 	     newstate != PATH_PENDING) && (pp->state == PATH_DELAYED)) {
 		/* If path state become failed again cancel path delay state */
@@ -2918,9 +2912,11 @@ check_paths(struct vectors *vecs, unsigned int ticks)
 	vector_foreach_slot(vecs->pathvec, pp, i) {
 		if (pp->is_checked != CHECK_PATH_UNCHECKED)
 			continue;
-		if (pp->mpp)
+		if (pp->mpp) {
 			pp->is_checked = check_path(pp, ticks);
-		else
+			if (pp->is_checked == CHECK_PATH_STARTED)
+				pp->mpp->checker_count++;
+		} else
 			pp->is_checked = check_uninitialized_path(pp, ticks);
 		if (pp->is_checked == CHECK_PATH_STARTED &&
 		    checker_need_wait(&pp->checker))
@@ -3014,12 +3010,10 @@ checkerloop (void *ap)
 			pthread_cleanup_push(cleanup_lock, &vecs->lock);
 			lock(&vecs->lock);
 			pthread_testcancel();
-			vector_foreach_slot(vecs->mpvec, mpp, i)
-				mpp->synced_count = 0;
 			if (checker_state == CHECKER_STARTING) {
 				vector_foreach_slot(vecs->mpvec, mpp, i) {
-					sync_mpp(vecs, mpp, ticks);
 					mpp->prio_update = PRIO_UPDATE_NONE;
+					mpp->checker_count = 0;
 				}
 				vector_foreach_slot(vecs->pathvec, pp, i)
 					pp->is_checked = CHECK_PATH_UNCHECKED;
@@ -3032,11 +3026,13 @@ checkerloop (void *ap)
 							     start_time.tv_sec);
 			if (checker_state == CHECKER_FINISHED) {
 				vector_foreach_slot(vecs->mpvec, mpp, i) {
-					if ((update_mpp_prio(mpp) ||
-					     (mpp->need_reload && mpp->synced_count > 0)) &&
-					    reload_and_sync_map(mpp, vecs) == 2)
+					sync_mpp(vecs, mpp, ticks);
+					if ((update_mpp_prio(mpp) || mpp->need_reload) &&
+					    reload_and_sync_map(mpp, vecs) == 2) {
 						/* multipath device deleted */
 						i--;
+						continue;
+					}
 				}
 			}
 			lock_cleanup_pop(vecs->lock);
-- 
2.47.1

Re: [PATCH v3 03/15] multipathd: sync maps at end of checkerloop

[Benjamin Marzinski]

On Fri, Jan 17, 2025 at 09:27:26PM +0100, Martin Wilck wrote:
> Rather than calling sync_mpp early in the checkerloop and tracking
> map synchronization with synced_count, call sync_mpp() in the CHECKER_FINISHED
> state, if either at least one path of the map has been checked in the current
> iteration, or the sync tick has expired. This avoids potentially deleting
> paths from the pathvec through the do_sync_mpp() -> update_multipath_strings()
> -> sync_paths -> check_removed_paths() call chain while we're iterating over
> the pathvec. Also, the time gap between obtaining path states and syncing
> the state with the kernel is smaller this way.
> 
> Suggested-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

> ---
>  libmultipath/structs.h     |  2 +-
>  libmultipath/structs_vec.c |  1 -
>  multipathd/main.c          | 26 +++++++++++---------------
>  3 files changed, 12 insertions(+), 17 deletions(-)
> 
> diff --git a/libmultipath/structs.h b/libmultipath/structs.h
> index 6a30c59..9d22bdd 100644
> --- a/libmultipath/structs.h
> +++ b/libmultipath/structs.h
> @@ -471,7 +471,7 @@ struct multipath {
>  	int ghost_delay_tick;
>  	int queue_mode;
>  	unsigned int sync_tick;
> -	int synced_count;
> +	int checker_count;
>  	enum prio_update_type prio_update;
>  	uid_t uid;
>  	gid_t gid;
> diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
> index 7a4e3eb..6aa744d 100644
> --- a/libmultipath/structs_vec.c
> +++ b/libmultipath/structs_vec.c
> @@ -530,7 +530,6 @@ update_multipath_table (struct multipath *mpp, vector pathvec, int flags)
>  	conf = get_multipath_config();
>  	mpp->sync_tick = conf->max_checkint;
>  	put_multipath_config(conf);
> -	mpp->synced_count++;
>  
>  	r = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_MPATH_ONLY,
>  			  (mapid_t) { .str = mpp->alias },
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 4a28fbb..e4e6bf7 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -2470,7 +2470,7 @@ sync_mpp(struct vectors * vecs, struct multipath *mpp, unsigned int ticks)
>  	if (mpp->sync_tick)
>  		mpp->sync_tick -= (mpp->sync_tick > ticks) ? ticks :
>  				  mpp->sync_tick;
> -	if (mpp->sync_tick)
> +	if (mpp->sync_tick && !mpp->checker_count)
>  		return;
>  
>  	do_sync_mpp(vecs, mpp);
> @@ -2513,12 +2513,6 @@ update_path_state (struct vectors * vecs, struct path * pp)
>  		return handle_path_wwid_change(pp, vecs)? CHECK_PATH_REMOVED :
>  							  CHECK_PATH_SKIPPED;
>  	}
> -	if (pp->mpp->synced_count == 0) {
> -		do_sync_mpp(vecs, pp->mpp);
> -		/* if update_multipath_strings orphaned the path, quit early */
> -		if (!pp->mpp)
> -			return CHECK_PATH_SKIPPED;
> -	}
>  	if ((newstate != PATH_UP && newstate != PATH_GHOST &&
>  	     newstate != PATH_PENDING) && (pp->state == PATH_DELAYED)) {
>  		/* If path state become failed again cancel path delay state */
> @@ -2918,9 +2912,11 @@ check_paths(struct vectors *vecs, unsigned int ticks)
>  	vector_foreach_slot(vecs->pathvec, pp, i) {
>  		if (pp->is_checked != CHECK_PATH_UNCHECKED)
>  			continue;
> -		if (pp->mpp)
> +		if (pp->mpp) {
>  			pp->is_checked = check_path(pp, ticks);
> -		else
> +			if (pp->is_checked == CHECK_PATH_STARTED)
> +				pp->mpp->checker_count++;
> +		} else
>  			pp->is_checked = check_uninitialized_path(pp, ticks);
>  		if (pp->is_checked == CHECK_PATH_STARTED &&
>  		    checker_need_wait(&pp->checker))
> @@ -3014,12 +3010,10 @@ checkerloop (void *ap)
>  			pthread_cleanup_push(cleanup_lock, &vecs->lock);
>  			lock(&vecs->lock);
>  			pthread_testcancel();
> -			vector_foreach_slot(vecs->mpvec, mpp, i)
> -				mpp->synced_count = 0;
>  			if (checker_state == CHECKER_STARTING) {
>  				vector_foreach_slot(vecs->mpvec, mpp, i) {
> -					sync_mpp(vecs, mpp, ticks);
>  					mpp->prio_update = PRIO_UPDATE_NONE;
> +					mpp->checker_count = 0;
>  				}
>  				vector_foreach_slot(vecs->pathvec, pp, i)
>  					pp->is_checked = CHECK_PATH_UNCHECKED;
> @@ -3032,11 +3026,13 @@ checkerloop (void *ap)
>  							     start_time.tv_sec);
>  			if (checker_state == CHECKER_FINISHED) {
>  				vector_foreach_slot(vecs->mpvec, mpp, i) {
> -					if ((update_mpp_prio(mpp) ||
> -					     (mpp->need_reload && mpp->synced_count > 0)) &&
> -					    reload_and_sync_map(mpp, vecs) == 2)
> +					sync_mpp(vecs, mpp, ticks);
> +					if ((update_mpp_prio(mpp) || mpp->need_reload) &&
> +					    reload_and_sync_map(mpp, vecs) == 2) {
>  						/* multipath device deleted */
>  						i--;
> +						continue;
> +					}
>  				}
>  			}
>  			lock_cleanup_pop(vecs->lock);
> -- 
> 2.47.1

[PATCH v3 04/15] multipathd: emit a warning if a map remains inconsistent after reload

[Martin Wilck]

After reading the kernel device-mapper table, update_pathvec_from_dm()
sets the mpp->need_reload flag if an inconsistent state was found (often a
path with wrong WWID). We expect reload_and_sync_map() to fix this situation.
If this is not the case (need_reload still set after map reloading), it's
most probably a bug.

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

diff --git a/multipathd/main.c b/multipathd/main.c
index e4e6bf7..ff3031e 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -3026,13 +3026,20 @@ checkerloop (void *ap)
 							     start_time.tv_sec);
 			if (checker_state == CHECKER_FINISHED) {
 				vector_foreach_slot(vecs->mpvec, mpp, i) {
+					bool inconsistent;
+
 					sync_mpp(vecs, mpp, ticks);
-					if ((update_mpp_prio(mpp) || mpp->need_reload) &&
+					inconsistent = mpp->need_reload;
+					if ((update_mpp_prio(mpp) || inconsistent) &&
 					    reload_and_sync_map(mpp, vecs) == 2) {
 						/* multipath device deleted */
 						i--;
 						continue;
 					}
+					/* need_reload was cleared in dm_addmap and then set again */
+					if (inconsistent && mpp->need_reload)
+						condlog(1, "BUG: %s; map remained in inconsistent state after reload",
+							mpp->alias);
 				}
 			}
 			lock_cleanup_pop(vecs->lock);
-- 
2.47.1

Re: [PATCH v3 04/15] multipathd: emit a warning if a map remains inconsistent after reload

[Benjamin Marzinski]

On Fri, Jan 17, 2025 at 09:27:27PM +0100, Martin Wilck wrote:
> After reading the kernel device-mapper table, update_pathvec_from_dm()
> sets the mpp->need_reload flag if an inconsistent state was found (often a
> path with wrong WWID). We expect reload_and_sync_map() to fix this situation.
> If this is not the case (need_reload still set after map reloading), it's
> most probably a bug.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

> ---
>  multipathd/main.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index e4e6bf7..ff3031e 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -3026,13 +3026,20 @@ checkerloop (void *ap)
>  							     start_time.tv_sec);
>  			if (checker_state == CHECKER_FINISHED) {
>  				vector_foreach_slot(vecs->mpvec, mpp, i) {
> +					bool inconsistent;
> +
>  					sync_mpp(vecs, mpp, ticks);
> -					if ((update_mpp_prio(mpp) || mpp->need_reload) &&
> +					inconsistent = mpp->need_reload;
> +					if ((update_mpp_prio(mpp) || inconsistent) &&
>  					    reload_and_sync_map(mpp, vecs) == 2) {
>  						/* multipath device deleted */
>  						i--;
>  						continue;
>  					}
> +					/* need_reload was cleared in dm_addmap and then set again */
> +					if (inconsistent && mpp->need_reload)
> +						condlog(1, "BUG: %s; map remained in inconsistent state after reload",
> +							mpp->alias);
>  				}
>  			}
>  			lock_cleanup_pop(vecs->lock);
> -- 
> 2.47.1

[PATCH v3 05/15] multipathd: move yielding for waiters to start of checkerloop

[Martin Wilck]

This simplifies the lock-taking logic and prepares the following
patch.

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index ff3031e..25aab09 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -3007,6 +3007,16 @@ checkerloop (void *ap)
 			struct multipath *mpp;
 			int i;
 
+			if (checker_state != CHECKER_STARTING) {
+				struct timespec wait = { .tv_nsec = 10000, };
+				if (checker_state == CHECKER_WAITING_FOR_PATHS) {
+					/* wait 5ms */
+					wait.tv_nsec = 5 * 1000 * 1000;
+					checker_state = CHECKER_UPDATING_PATHS;
+				}
+				nanosleep(&wait, NULL);
+			}
+
 			pthread_cleanup_push(cleanup_lock, &vecs->lock);
 			lock(&vecs->lock);
 			pthread_testcancel();
@@ -3043,16 +3053,6 @@ checkerloop (void *ap)
 				}
 			}
 			lock_cleanup_pop(vecs->lock);
-			if (checker_state != CHECKER_FINISHED) {
-				/* Yield to waiters */
-				struct timespec wait = { .tv_nsec = 10000, };
-				if (checker_state == CHECKER_WAITING_FOR_PATHS) {
-					/* wait 5ms */
-					wait.tv_nsec = 5 * 1000 * 1000;
-					checker_state = CHECKER_UPDATING_PATHS;
-				}
-				nanosleep(&wait, NULL);
-			}
 		}
 
 		pthread_cleanup_push(cleanup_lock, &vecs->lock);
-- 
2.47.1

[PATCH v3 06/15] multipathd: add checker_finished()

[Martin Wilck]

Move the code that handles the checker_state == CHECKER_FINISHED state
into a separate function for better readability. Subsequent patches will
add code to this function.

Replace the "&& reload_and_sync_map()" conditional by a subordinate if
statement, as this expresses the logic of the code better.

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 43 +++++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 25aab09..988c82c 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2967,6 +2967,29 @@ update_paths(struct vectors *vecs, int *num_paths_p, time_t start_secs)
 	return CHECKER_FINISHED;
 }
 
+static void checker_finished(struct vectors *vecs, unsigned int ticks)
+{
+	struct multipath *mpp;
+	int i;
+
+	vector_foreach_slot(vecs->mpvec, mpp, i) {
+		bool inconsistent;
+
+		sync_mpp(vecs, mpp, ticks);
+		inconsistent = mpp->need_reload;
+		if (update_mpp_prio(mpp) || inconsistent)
+			if (reload_and_sync_map(mpp, vecs) == 2) {
+				/* multipath device deleted */
+				i--;
+				continue;
+			}
+		/* need_reload was cleared in dm_addmap and then set again */
+		if (inconsistent && mpp->need_reload)
+			condlog(1, "BUG: %s; map remained in inconsistent state after reload",
+				mpp->alias);
+	}
+}
+
 static void *
 checkerloop (void *ap)
 {
@@ -3034,24 +3057,8 @@ checkerloop (void *ap)
 			if (checker_state == CHECKER_UPDATING_PATHS)
 				checker_state = update_paths(vecs, &num_paths,
 							     start_time.tv_sec);
-			if (checker_state == CHECKER_FINISHED) {
-				vector_foreach_slot(vecs->mpvec, mpp, i) {
-					bool inconsistent;
-
-					sync_mpp(vecs, mpp, ticks);
-					inconsistent = mpp->need_reload;
-					if ((update_mpp_prio(mpp) || inconsistent) &&
-					    reload_and_sync_map(mpp, vecs) == 2) {
-						/* multipath device deleted */
-						i--;
-						continue;
-					}
-					/* need_reload was cleared in dm_addmap and then set again */
-					if (inconsistent && mpp->need_reload)
-						condlog(1, "BUG: %s; map remained in inconsistent state after reload",
-							mpp->alias);
-				}
-			}
+			if (checker_state == CHECKER_FINISHED)
+				checker_finished(vecs, ticks);
 			lock_cleanup_pop(vecs->lock);
 		}
 
-- 
2.47.1

[PATCH v3 07/15] multipathd: move "tick" calls into checker_finished()

[Martin Wilck]

The various "tick" functions will only be called after CHECKER_FINISHED
is reached. So we might as well move them into the respective code block
into checker_finished(). This way don't have to drop and re-take he lock
when all paths have been checked.

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 988c82c..6df8769 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2988,6 +2988,11 @@ static void checker_finished(struct vectors *vecs, unsigned int ticks)
 			condlog(1, "BUG: %s; map remained in inconsistent state after reload",
 				mpp->alias);
 	}
+	deferred_failback_tick(vecs);
+	retry_count_tick(vecs->mpvec);
+	missing_uev_wait_tick(vecs);
+	ghost_delay_tick(vecs);
+	partial_retrigger_tick(vecs->pathvec);
 }
 
 static void *
@@ -3062,16 +3067,6 @@ checkerloop (void *ap)
 			lock_cleanup_pop(vecs->lock);
 		}
 
-		pthread_cleanup_push(cleanup_lock, &vecs->lock);
-		lock(&vecs->lock);
-		pthread_testcancel();
-		deferred_failback_tick(vecs);
-		retry_count_tick(vecs->mpvec);
-		missing_uev_wait_tick(vecs);
-		ghost_delay_tick(vecs);
-		partial_retrigger_tick(vecs->pathvec);
-		lock_cleanup_pop(vecs->lock);
-
 		if (count)
 			count--;
 		else {
-- 
2.47.1

[PATCH v3 08/15] multipathd: don't call reload_and_sync_map() from deferred_failback_tick()

[Martin Wilck]

Instead, move the call inside the existing loop over vecs->mpvec and call
reload_and_sync_map() from checker_finished(). Note that we can't simply
put the call deferred_failback_tick() in the "if" condition, because we
need to adjust the ticks even if update_mpp_prio() returns true. For
consistency, use separate bool variables for each condition that would
necessitate a reload.

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

diff --git a/multipathd/main.c b/multipathd/main.c
index 6df8769..8c2712d 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2076,32 +2076,22 @@ ghost_delay_tick(struct vectors *vecs)
 	}
 }
 
-static void
-deferred_failback_tick (struct vectors *vecs)
+static bool deferred_failback_tick(struct multipath *mpp)
 {
-	struct multipath * mpp;
-	int i;
 	bool need_reload;
 
-	vector_foreach_slot (vecs->mpvec, mpp, i) {
-		/*
-		 * deferred failback getting sooner
-		 */
-		if (mpp->pgfailback > 0 && mpp->failback_tick > 0) {
-			mpp->failback_tick--;
+	if (mpp->pgfailback <= 0 || mpp->failback_tick <= 0)
+		return false;
 
-			if (!mpp->failback_tick &&
-			    need_switch_pathgroup(mpp, &need_reload)) {
-				if (need_reload) {
-					if (reload_and_sync_map(mpp, vecs) == 2) {
-						/* multipath device removed */
-						i--;
-					}
-				} else
-					switch_pathgroup(mpp);
-			}
-		}
+	mpp->failback_tick--;
+	if (!mpp->failback_tick &&
+	    need_switch_pathgroup(mpp, &need_reload)) {
+		if (need_reload)
+			return true;
+		else
+			switch_pathgroup(mpp);
 	}
+	return false;
 }
 
 static void
@@ -2973,11 +2963,13 @@ static void checker_finished(struct vectors *vecs, unsigned int ticks)
 	int i;
 
 	vector_foreach_slot(vecs->mpvec, mpp, i) {
-		bool inconsistent;
+		bool inconsistent, prio_reload, failback_reload;
 
 		sync_mpp(vecs, mpp, ticks);
 		inconsistent = mpp->need_reload;
-		if (update_mpp_prio(mpp) || inconsistent)
+		prio_reload = update_mpp_prio(mpp);
+		failback_reload = deferred_failback_tick(mpp);
+		if (prio_reload || failback_reload || inconsistent)
 			if (reload_and_sync_map(mpp, vecs) == 2) {
 				/* multipath device deleted */
 				i--;
@@ -2988,7 +2980,6 @@ static void checker_finished(struct vectors *vecs, unsigned int ticks)
 			condlog(1, "BUG: %s; map remained in inconsistent state after reload",
 				mpp->alias);
 	}
-	deferred_failback_tick(vecs);
 	retry_count_tick(vecs->mpvec);
 	missing_uev_wait_tick(vecs);
 	ghost_delay_tick(vecs);
-- 
2.47.1

Re: [PATCH v3 08/15] multipathd: don't call reload_and_sync_map() from deferred_failback_tick()

[Benjamin Marzinski]

On Fri, Jan 17, 2025 at 09:27:31PM +0100, Martin Wilck wrote:
> Instead, move the call inside the existing loop over vecs->mpvec and call
> reload_and_sync_map() from checker_finished(). Note that we can't simply
> put the call deferred_failback_tick() in the "if" condition, because we
> need to adjust the ticks even if update_mpp_prio() returns true. For
> consistency, use separate bool variables for each condition that would
> necessitate a reload.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

> ---
>  multipathd/main.c | 39 +++++++++++++++------------------------
>  1 file changed, 15 insertions(+), 24 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 6df8769..8c2712d 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -2076,32 +2076,22 @@ ghost_delay_tick(struct vectors *vecs)
>  	}
>  }
>  
> -static void
> -deferred_failback_tick (struct vectors *vecs)
> +static bool deferred_failback_tick(struct multipath *mpp)
>  {
> -	struct multipath * mpp;
> -	int i;
>  	bool need_reload;
>  
> -	vector_foreach_slot (vecs->mpvec, mpp, i) {
> -		/*
> -		 * deferred failback getting sooner
> -		 */
> -		if (mpp->pgfailback > 0 && mpp->failback_tick > 0) {
> -			mpp->failback_tick--;
> +	if (mpp->pgfailback <= 0 || mpp->failback_tick <= 0)
> +		return false;
>  
> -			if (!mpp->failback_tick &&
> -			    need_switch_pathgroup(mpp, &need_reload)) {
> -				if (need_reload) {
> -					if (reload_and_sync_map(mpp, vecs) == 2) {
> -						/* multipath device removed */
> -						i--;
> -					}
> -				} else
> -					switch_pathgroup(mpp);
> -			}
> -		}
> +	mpp->failback_tick--;
> +	if (!mpp->failback_tick &&
> +	    need_switch_pathgroup(mpp, &need_reload)) {
> +		if (need_reload)
> +			return true;
> +		else
> +			switch_pathgroup(mpp);
>  	}
> +	return false;
>  }
>  
>  static void
> @@ -2973,11 +2963,13 @@ static void checker_finished(struct vectors *vecs, unsigned int ticks)
>  	int i;
>  
>  	vector_foreach_slot(vecs->mpvec, mpp, i) {
> -		bool inconsistent;
> +		bool inconsistent, prio_reload, failback_reload;
>  
>  		sync_mpp(vecs, mpp, ticks);
>  		inconsistent = mpp->need_reload;
> -		if (update_mpp_prio(mpp) || inconsistent)
> +		prio_reload = update_mpp_prio(mpp);
> +		failback_reload = deferred_failback_tick(mpp);
> +		if (prio_reload || failback_reload || inconsistent)
>  			if (reload_and_sync_map(mpp, vecs) == 2) {
>  				/* multipath device deleted */
>  				i--;
> @@ -2988,7 +2980,6 @@ static void checker_finished(struct vectors *vecs, unsigned int ticks)
>  			condlog(1, "BUG: %s; map remained in inconsistent state after reload",
>  				mpp->alias);
>  	}
> -	deferred_failback_tick(vecs);
>  	retry_count_tick(vecs->mpvec);
>  	missing_uev_wait_tick(vecs);
>  	ghost_delay_tick(vecs);
> -- 
> 2.47.1

[PATCH v3 09/15] multipathd: move retry_count_tick() into existing mpvec loop

[Martin Wilck]

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 8c2712d..753f48c 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2095,21 +2095,17 @@ static bool deferred_failback_tick(struct multipath *mpp)
 }
 
 static void
-retry_count_tick(vector mpvec)
+retry_count_tick(struct multipath *mpp)
 {
-	struct multipath *mpp;
-	unsigned int i;
+	if (mpp->retry_tick <= 0)
+		return;
 
-	vector_foreach_slot (mpvec, mpp, i) {
-		if (mpp->retry_tick > 0) {
-			mpp->stat_total_queueing_time++;
-			condlog(4, "%s: Retrying.. No active path", mpp->alias);
-			if(--mpp->retry_tick == 0) {
-				mpp->stat_map_failures++;
-				dm_queue_if_no_path(mpp, 0);
-				condlog(2, "%s: Disable queueing", mpp->alias);
-			}
-		}
+	mpp->stat_total_queueing_time++;
+	condlog(4, "%s: Retrying.. No active path", mpp->alias);
+	if(--mpp->retry_tick == 0) {
+		mpp->stat_map_failures++;
+		dm_queue_if_no_path(mpp, 0);
+		condlog(2, "%s: Disable queueing", mpp->alias);
 	}
 }
 
@@ -2979,8 +2975,8 @@ static void checker_finished(struct vectors *vecs, unsigned int ticks)
 		if (inconsistent && mpp->need_reload)
 			condlog(1, "BUG: %s; map remained in inconsistent state after reload",
 				mpp->alias);
+		retry_count_tick(mpp);
 	}
-	retry_count_tick(vecs->mpvec);
 	missing_uev_wait_tick(vecs);
 	ghost_delay_tick(vecs);
 	partial_retrigger_tick(vecs->pathvec);
-- 
2.47.1

[PATCH v3 10/15] multipathd: don't call update_map() from missing_uev_wait_tick()

[Martin Wilck]

Instead, check for missing uevents in the existing mpvec loop.
Note that if the uevent tick expires, we need to call update_map() rather than
reload_and_sync_map(), because the paths have not been added to the multipath
(see wait_for_udev handling ev_add_path()).

The set of actions taken by update_map() is a superset of those in
reload_and_sync_map(), thus we don't need to call the latter if we've already
called the former.

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 44 ++++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 753f48c..2989cb1 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2029,29 +2029,19 @@ followover_should_failback(struct multipath *mpp)
 	return 0;
 }
 
-static void
-missing_uev_wait_tick(struct vectors *vecs)
+/* Returns true if update_map() needs to be called */
+static bool
+missing_uev_wait_tick(struct multipath *mpp, bool *timed_out)
 {
-	struct multipath * mpp;
-	int i;
-	int timed_out = 0;
+	if (mpp->wait_for_udev && --mpp->uev_wait_tick <= 0) {
+		int wait = mpp->wait_for_udev;
 
-	vector_foreach_slot (vecs->mpvec, mpp, i) {
-		if (mpp->wait_for_udev && --mpp->uev_wait_tick <= 0) {
-			timed_out = 1;
-			condlog(0, "%s: timeout waiting on creation uevent. enabling reloads", mpp->alias);
-			if (mpp->wait_for_udev > 1 &&
-			    update_map(mpp, vecs, 0)) {
-				/* update_map removed map */
-				i--;
-				continue;
-			}
-			mpp->wait_for_udev = 0;
-		}
+		mpp->wait_for_udev = 0;
+		*timed_out = true;
+		condlog(0, "%s: timeout waiting on creation uevent. enabling reloads", mpp->alias);
+		return wait > 1;
 	}
-
-	if (timed_out && !need_to_delay_reconfig(vecs))
-		unblock_reconfigure();
+	return false;
 }
 
 static void
@@ -2956,16 +2946,25 @@ update_paths(struct vectors *vecs, int *num_paths_p, time_t start_secs)
 static void checker_finished(struct vectors *vecs, unsigned int ticks)
 {
 	struct multipath *mpp;
+	bool uev_timed_out = false;
 	int i;
 
 	vector_foreach_slot(vecs->mpvec, mpp, i) {
 		bool inconsistent, prio_reload, failback_reload;
+		bool uev_wait_reload;
 
 		sync_mpp(vecs, mpp, ticks);
 		inconsistent = mpp->need_reload;
 		prio_reload = update_mpp_prio(mpp);
 		failback_reload = deferred_failback_tick(mpp);
-		if (prio_reload || failback_reload || inconsistent)
+		uev_wait_reload = missing_uev_wait_tick(mpp, &uev_timed_out);
+		if (uev_wait_reload) {
+			if (update_map(mpp, vecs, 0)) {
+				/* multipath device deleted */
+				i--;
+				continue;
+			}
+		} else if (prio_reload || failback_reload || inconsistent)
 			if (reload_and_sync_map(mpp, vecs) == 2) {
 				/* multipath device deleted */
 				i--;
@@ -2977,7 +2976,8 @@ static void checker_finished(struct vectors *vecs, unsigned int ticks)
 				mpp->alias);
 		retry_count_tick(mpp);
 	}
-	missing_uev_wait_tick(vecs);
+	if (uev_timed_out && !need_to_delay_reconfig(vecs))
+		unblock_reconfigure();
 	ghost_delay_tick(vecs);
 	partial_retrigger_tick(vecs->pathvec);
 }
-- 
2.47.1

[PATCH v3 11/15] multipathd: don't call udpate_map() from ghost_delay_tick()

[Martin Wilck]

Instead, move the call into the existing mpvec loop and call update_map()
from checker_finished().

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 33 ++++++++++++---------------------
 1 file changed, 12 insertions(+), 21 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 2989cb1..176d9bf 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2044,26 +2044,17 @@ missing_uev_wait_tick(struct multipath *mpp, bool *timed_out)
 	return false;
 }
 
-static void
-ghost_delay_tick(struct vectors *vecs)
+static bool
+ghost_delay_tick(struct  multipath * mpp)
 {
-	struct multipath * mpp;
-	int i;
-
-	vector_foreach_slot (vecs->mpvec, mpp, i) {
-		if (mpp->ghost_delay_tick <= 0)
-			continue;
-		if (--mpp->ghost_delay_tick <= 0) {
-			condlog(0, "%s: timed out waiting for active path",
-				mpp->alias);
-			mpp->force_udev_reload = 1;
-			if (update_map(mpp, vecs, 0) != 0) {
-				/* update_map removed map */
-				i--;
-				continue;
-			}
-		}
+	if (mpp->ghost_delay_tick <= 0)
+		return false;
+	if (--mpp->ghost_delay_tick <= 0) {
+		condlog(0, "%s: timed out waiting for active path", mpp->alias);
+		mpp->force_udev_reload = 1;
+		return true;
 	}
+	return false;
 }
 
 static bool deferred_failback_tick(struct multipath *mpp)
@@ -2951,14 +2942,15 @@ static void checker_finished(struct vectors *vecs, unsigned int ticks)
 
 	vector_foreach_slot(vecs->mpvec, mpp, i) {
 		bool inconsistent, prio_reload, failback_reload;
-		bool uev_wait_reload;
+		bool uev_wait_reload, ghost_reload;
 
 		sync_mpp(vecs, mpp, ticks);
 		inconsistent = mpp->need_reload;
 		prio_reload = update_mpp_prio(mpp);
 		failback_reload = deferred_failback_tick(mpp);
 		uev_wait_reload = missing_uev_wait_tick(mpp, &uev_timed_out);
-		if (uev_wait_reload) {
+		ghost_reload = ghost_delay_tick(mpp);
+		if (uev_wait_reload || ghost_reload) {
 			if (update_map(mpp, vecs, 0)) {
 				/* multipath device deleted */
 				i--;
@@ -2978,7 +2970,6 @@ static void checker_finished(struct vectors *vecs, unsigned int ticks)
 	}
 	if (uev_timed_out && !need_to_delay_reconfig(vecs))
 		unblock_reconfigure();
-	ghost_delay_tick(vecs);
 	partial_retrigger_tick(vecs->pathvec);
 }
 
-- 
2.47.1

[PATCH v3 12/15] multipathd: only call reload_and_sync_map() when ghost delay expires

[Martin Wilck]

While we're waiting for active paths to appear when ghost delay is
enabled, we do add new paths to multipathd's internal mpp data structure
as they are discovered (unlike missing uevent case, where we orphan
the paths instead). When the ghost_delay timer expires, it should be
sufficient to call the more light-weight reload_and_sync_map() instead
of update_map().

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

diff --git a/multipathd/main.c b/multipathd/main.c
index 176d9bf..4063e82 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2950,13 +2950,13 @@ static void checker_finished(struct vectors *vecs, unsigned int ticks)
 		failback_reload = deferred_failback_tick(mpp);
 		uev_wait_reload = missing_uev_wait_tick(mpp, &uev_timed_out);
 		ghost_reload = ghost_delay_tick(mpp);
-		if (uev_wait_reload || ghost_reload) {
+		if (uev_wait_reload) {
 			if (update_map(mpp, vecs, 0)) {
 				/* multipath device deleted */
 				i--;
 				continue;
 			}
-		} else if (prio_reload || failback_reload || inconsistent)
+		} else if (prio_reload || failback_reload || ghost_reload || inconsistent)
 			if (reload_and_sync_map(mpp, vecs) == 2) {
 				/* multipath device deleted */
 				i--;
-- 
2.47.1

[PATCH v3 13/15] multipathd: remove non-existent maps in checkerloop

[Martin Wilck]

If update_multipath_strings() returns DMP_NOT_FOUND for a map that
we are tracking in the checker, the map must have been removed by
an external entity. Remove it.

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 4063e82..792b94a 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2413,7 +2413,7 @@ get_new_state(struct path *pp)
 	return newstate;
 }
 
-static void
+static int
 do_sync_mpp(struct vectors * vecs, struct multipath *mpp)
 {
 	int i, ret;
@@ -2426,21 +2426,22 @@ do_sync_mpp(struct vectors * vecs, struct multipath *mpp)
 			"couldn't synchronize with kernel state");
 		vector_foreach_slot (mpp->paths, pp, i)
 			pp->dmstate = PSTATE_UNDEF;
-		return;
+		return ret;
 	}
 	set_no_path_retry(mpp);
+	return ret;
 }
 
-static void
+static int
 sync_mpp(struct vectors * vecs, struct multipath *mpp, unsigned int ticks)
 {
 	if (mpp->sync_tick)
 		mpp->sync_tick -= (mpp->sync_tick > ticks) ? ticks :
 				  mpp->sync_tick;
 	if (mpp->sync_tick && !mpp->checker_count)
-		return;
+		return DMP_OK;
 
-	do_sync_mpp(vecs, mpp);
+	return do_sync_mpp(vecs, mpp);
 }
 
 static int
@@ -2944,7 +2945,11 @@ static void checker_finished(struct vectors *vecs, unsigned int ticks)
 		bool inconsistent, prio_reload, failback_reload;
 		bool uev_wait_reload, ghost_reload;
 
-		sync_mpp(vecs, mpp, ticks);
+		if (sync_mpp(vecs, mpp, ticks) == DMP_NOT_FOUND) {
+			remove_map_and_stop_waiter(mpp, vecs);
+			i--;
+			continue;
+		}
 		inconsistent = mpp->need_reload;
 		prio_reload = update_mpp_prio(mpp);
 		failback_reload = deferred_failback_tick(mpp);
-- 
2.47.1

[PATCH v3 14/15] multipathd: remove mpvec_garbage_collector()

[Martin Wilck]

This function duplicates functionality that we now have in the
checker_finished() code path. Remove it.

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 31 -------------------------------
 1 file changed, 31 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 792b94a..310d7ef 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1967,24 +1967,6 @@ enable_group(struct path * pp)
 	}
 }
 
-static void
-mpvec_garbage_collector (struct vectors * vecs)
-{
-	struct multipath * mpp;
-	int i;
-
-	if (!vecs->mpvec)
-		return;
-
-	vector_foreach_slot (vecs->mpvec, mpp, i) {
-		if (mpp && mpp->alias && !dm_map_present(mpp->alias)) {
-			condlog(2, "%s: remove dead map", mpp->alias);
-			remove_map_and_stop_waiter(mpp, vecs);
-			i--;
-		}
-	}
-}
-
 /* This is called after a path has started working again. It the multipath
  * device for this path uses the followover failback type, and this is the
  * best pathgroup, and this is the first path in the pathgroup to come back
@@ -2983,7 +2965,6 @@ checkerloop (void *ap)
 {
 	struct vectors *vecs;
 	struct path *pp;
-	int count = 0;
 	struct timespec last_time;
 	struct config *conf;
 	int foreign_tick = 0;
@@ -3050,18 +3031,6 @@ checkerloop (void *ap)
 			lock_cleanup_pop(vecs->lock);
 		}
 
-		if (count)
-			count--;
-		else {
-			pthread_cleanup_push(cleanup_lock, &vecs->lock);
-			lock(&vecs->lock);
-			pthread_testcancel();
-			condlog(4, "map garbage collection");
-			mpvec_garbage_collector(vecs);
-			count = MAPGCINT;
-			lock_cleanup_pop(vecs->lock);
-		}
-
 		get_monotonic_time(&end_time);
 		timespecsub(&end_time, &start_time, &diff_time);
 		if (num_paths) {
-- 
2.47.1

[PATCH v3 15/15] multipathd: enable pathgroups in checker_finished()

[Martin Wilck]

multipathd calls enable_group() from update_path_state() if a path in a
previously disabled pathgroup is reinstated. This call may be mistakenly
skipped if the path group status wasn't up-to-date while update_path_state()
was executed. This can happen after applying the previous patch "multipathd:
sync maps at end of checkerloop", if the kernel has disabled the group during
the last checker interval.

Therefore add another check in checker_finished() after calling sync_mpp(),
and enable groups if necessary. This step can be skipped if the map was
reloaded, because after a reload, all pathgroups are enabled by default.

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

diff --git a/multipathd/main.c b/multipathd/main.c
index 310d7ef..98abadc 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2917,6 +2917,34 @@ update_paths(struct vectors *vecs, int *num_paths_p, time_t start_secs)
 	return CHECKER_FINISHED;
 }
 
+static void enable_pathgroups(struct multipath *mpp)
+{
+	struct pathgroup *pgp;
+	int i;
+
+	vector_foreach_slot(mpp->pg, pgp, i) {
+		struct path *pp;
+		int j;
+
+		if (pgp->status != PGSTATE_DISABLED)
+			continue;
+
+		vector_foreach_slot(pgp->paths, pp, j) {
+			if (pp->state != PATH_UP)
+				continue;
+
+			if (dm_enablegroup(mpp->alias, i + 1) == 0) {
+				condlog(2, "%s: enabled pathgroup #%i",
+					mpp->alias, i + 1);
+				pgp->status = PGSTATE_ENABLED;
+			} else
+				condlog(2, "%s: failed to enable pathgroup #%i",
+					mpp->alias, i + 1);
+			break;
+		}
+	}
+}
+
 static void checker_finished(struct vectors *vecs, unsigned int ticks)
 {
 	struct multipath *mpp;
@@ -2943,12 +2971,16 @@ static void checker_finished(struct vectors *vecs, unsigned int ticks)
 				i--;
 				continue;
 			}
-		} else if (prio_reload || failback_reload || ghost_reload || inconsistent)
+		} else if (prio_reload || failback_reload || ghost_reload || inconsistent) {
 			if (reload_and_sync_map(mpp, vecs) == 2) {
 				/* multipath device deleted */
 				i--;
 				continue;
 			}
+		} else
+			/* not necessary after map reloads */
+			enable_pathgroups(mpp);
+
 		/* need_reload was cleared in dm_addmap and then set again */
 		if (inconsistent && mpp->need_reload)
 			condlog(1, "BUG: %s; map remained in inconsistent state after reload",
-- 
2.47.1

Re: [PATCH v3 15/15] multipathd: enable pathgroups in checker_finished()

[Benjamin Marzinski]

On Fri, Jan 17, 2025 at 09:27:38PM +0100, Martin Wilck wrote:
> multipathd calls enable_group() from update_path_state() if a path in a
> previously disabled pathgroup is reinstated. This call may be mistakenly
> skipped if the path group status wasn't up-to-date while update_path_state()
> was executed. This can happen after applying the previous patch "multipathd:
> sync maps at end of checkerloop", if the kernel has disabled the group during
> the last checker interval.
> 
> Therefore add another check in checker_finished() after calling sync_mpp(),
> and enable groups if necessary. This step can be skipped if the map was
> reloaded, because after a reload, all pathgroups are enabled by default.

The logic running in checker_finished() isn't the same as the logic
running in update_path_state(). It update_path_state(), we only enable a
pathgroup when a path switches to PATH_UP. In checker_finished(), we
enable it whenever a path is in PATH_UP. I worry that this might not
always be correct. Perhaps instead of just checking if the path is in
PATH_UP, we should also make sure that pp->is_checked isn't in
CHECK_PATH_SKIPPED, which would mean that the checker is pending or
messed up, and the path is using its old state. This still assumes that
the path switched to some other state before the pathgroup got delayed
in re-initializing again, but that seems pretty safe. I just don't want
to go re-enabling pathgroups where the controller is actually busy,
since I'm pretty sure that the kernel usually does the right thing
without multipathd's help.

Alternatively, we could make the check_finished() logic match the
update_path_state() logic exactly by just setting a flag in the path's
pathgroup during enable_group() (and possibly not call dm_enablegroup()
at all, but I suppose that there could be a benefit to re-enabling the
group as soon as possible. I'm still kinda fuzzy on whether the kernel's
own pathgroup re-enabling code makes all this redundant). Then, in
enable_pathrgoups(), instead of checking each path, we just need to
check if pgp->need_reenable is set for the pathgroup().

The other benefit to using a flag like pgp->need_reenable is that we
could clear it on all pathgroups if we called switch_pathgroups(), since
that will cause the kernel to enable all the pathgroups anyways, which
makes our pgp->status invalid. Although we probably should also update
pgp->status to be PGSTATE_ENABLED (or at least PGSTATE_UNDEF) if we
were messing with the pathgroups in switch_pathgroups(). And if we
update pgp->status, that would avoid unnecessary re-enables with my
first idea as well (since no pathgroups would be disabled anymore). 

But perhaps the best answer is to just to say that this is a corner case
where we skip a multipathd action that might be totally unnecessary. And
even if it is a problem, it will be fixed if multipathd ever decides
that the kernel is using the wrong pathgroup and should switch, or
whenever the table gets reloaded. Maybe the whole patch is unnecessary.

Thoughts? I'm clearly thinking too much.
-Ben

> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipathd/main.c | 34 +++++++++++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 310d7ef..98abadc 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -2917,6 +2917,34 @@ update_paths(struct vectors *vecs, int *num_paths_p, time_t start_secs)
>  	return CHECKER_FINISHED;
>  }
>  
> +static void enable_pathgroups(struct multipath *mpp)
> +{
> +	struct pathgroup *pgp;
> +	int i;
> +
> +	vector_foreach_slot(mpp->pg, pgp, i) {
> +		struct path *pp;
> +		int j;
> +
> +		if (pgp->status != PGSTATE_DISABLED)
> +			continue;
> +
> +		vector_foreach_slot(pgp->paths, pp, j) {
> +			if (pp->state != PATH_UP)
> +				continue;
> +
> +			if (dm_enablegroup(mpp->alias, i + 1) == 0) {
> +				condlog(2, "%s: enabled pathgroup #%i",
> +					mpp->alias, i + 1);
> +				pgp->status = PGSTATE_ENABLED;
> +			} else
> +				condlog(2, "%s: failed to enable pathgroup #%i",
> +					mpp->alias, i + 1);
> +			break;
> +		}
> +	}
> +}
> +
>  static void checker_finished(struct vectors *vecs, unsigned int ticks)
>  {
>  	struct multipath *mpp;
> @@ -2943,12 +2971,16 @@ static void checker_finished(struct vectors *vecs, unsigned int ticks)
>  				i--;
>  				continue;
>  			}
> -		} else if (prio_reload || failback_reload || ghost_reload || inconsistent)
> +		} else if (prio_reload || failback_reload || ghost_reload || inconsistent) {
>  			if (reload_and_sync_map(mpp, vecs) == 2) {
>  				/* multipath device deleted */
>  				i--;
>  				continue;
>  			}
> +		} else
> +			/* not necessary after map reloads */
> +			enable_pathgroups(mpp);
> +
>  		/* need_reload was cleared in dm_addmap and then set again */
>  		if (inconsistent && mpp->need_reload)
>  			condlog(1, "BUG: %s; map remained in inconsistent state after reload",
> -- 
> 2.47.1

Re: [PATCH v3 15/15] multipathd: enable pathgroups in checker_finished()

[Martin Wilck]

On Mon, 2025-01-20 at 18:51 -0500, Benjamin Marzinski wrote:
> On Fri, Jan 17, 2025 at 09:27:38PM +0100, Martin Wilck wrote:
> > multipathd calls enable_group() from update_path_state() if a path
> > in a
> > previously disabled pathgroup is reinstated. This call may be
> > mistakenly
> > skipped if the path group status wasn't up-to-date while
> > update_path_state()
> > was executed. This can happen after applying the previous patch
> > "multipathd:
> > sync maps at end of checkerloop", if the kernel has disabled the
> > group during
> > the last checker interval.
> > 
> > Therefore add another check in checker_finished() after calling
> > sync_mpp(),
> > and enable groups if necessary. This step can be skipped if the map
> > was
> > reloaded, because after a reload, all pathgroups are enabled by
> > default.
> 
> The logic running in checker_finished() isn't the same as the logic
> running in update_path_state(). It update_path_state(), we only
> enable a
> pathgroup when a path switches to PATH_UP. In checker_finished(), we
> enable it whenever a path is in PATH_UP. I worry that this might not
> always be correct. Perhaps instead of just checking if the path is in
> PATH_UP, we should also make sure that pp->is_checked isn't in
> CHECK_PATH_SKIPPED, which would mean that the checker is pending or
> messed up, and the path is using its old state. This still assumes
> that
> the path switched to some other state before the pathgroup got
> delayed
> in re-initializing again, but that seems pretty safe. I just don't
> want
> to go re-enabling pathgroups where the controller is actually busy,
> since I'm pretty sure that the kernel usually does the right thing
> without multipathd's help.

OK. I'll see if I can add a test for the checker state.

> Alternatively, we could make the check_finished() logic match the
> update_path_state() logic exactly by just setting a flag in the
> path's
> pathgroup during enable_group() (and possibly not call
> dm_enablegroup()
> at all, 

That was my first thought, but it doesn't work, because we call  
free_pgvec(mpp->pg, KEEP_PATHS) update_multipath_strings(), and
reallocate the vector in disassemble_map. I guess we could change that,
but it would be another intrusive patch set. Currently we just can't
store any persistent properties in struct pathgroup.

> but I suppose that there could be a benefit to re-enabling the
> group as soon as possible. I'm still kinda fuzzy on whether the
> kernel's
> own pathgroup re-enabling code makes all this redundant).

The kernel will even try "bypassed" PGs if it finds no other. But that
means that e.g. the marginal pathgroup would be tried before disabled
PGs [1]. In general, I don't think we can rely on the kernel in this
respect.

>  Then, in
> enable_pathrgoups(), instead of checking each path, we just need to
> check if pgp->need_reenable is set for the pathgroup().

As I said, as long as we discard the PGs, I guess we'll have to test
the path flags. It's difficult to come up with a decent test case for
this, in particular if checking for PATH_UP is not enough...


> [...]
> 
> But perhaps the best answer is to just to say that this is a corner
> case
> where we skip a multipathd action that might be totally unnecessary.
> And
> even if it is a problem, it will be fixed if multipathd ever decides
> that the kernel is using the wrong pathgroup and should switch, or
> whenever the table gets reloaded. Maybe the whole patch is
> unnecessary.

I'm going to submit a v4 that adds a test for the checker flags.

Btw, I could only find one place in the kernel code where the kernel
actively sets the "bypassed" flag, when the SCSI device handler returns
SCSL_DH_DEV_TEMP_BUSY [2]. This seems rather questionable to me. First
of all, this seems to assume that PGs are mapped to "controllers", i.e.
something like "group_by_tpg", which isn't necessarily the case.
Second, while the EMC device handler uses this error code in this way,
in the ALUA handler it rather means a memory allocation failure, and
the other device handlers don't support it at all.

I think we can conclude that the kernel setting "bypassed" on a path
group by itself is indeed a corner case, except for EMC Clariion, which
is quite dated by now.

> Thoughts? I'm clearly thinking too much.
> 

No, you are not. I strongly appreciate these comments.

Regards,
Martin

[1] Unrelated wild thought: in view of the logic the kernel applies for
"bypassed" PGs, it might actually make sense to use this flag for 
marginal pathgroups. They would be tried if all else fails, which is
more or less the behavior we want for them. What do you think?

[2] https://elixir.bootlin.com/linux/v6.12.6/source/drivers/md/dm-mpath.c#L1564

Re: [PATCH v3 15/15] multipathd: enable pathgroups in checker_finished()

[Benjamin Marzinski]

On Tue, Jan 21, 2025 at 12:12:50PM +0100, Martin Wilck wrote:
> On Mon, 2025-01-20 at 18:51 -0500, Benjamin Marzinski wrote:
> > On Fri, Jan 17, 2025 at 09:27:38PM +0100, Martin Wilck wrote:
> 
> > Alternatively, we could make the check_finished() logic match the
> > update_path_state() logic exactly by just setting a flag in the
> > path's
> > pathgroup during enable_group() (and possibly not call
> > dm_enablegroup()
> > at all, 
> 
> That was my first thought, but it doesn't work, because we call  
> free_pgvec(mpp->pg, KEEP_PATHS) update_multipath_strings(), and
> reallocate the vector in disassemble_map. I guess we could change that,
> but it would be another intrusive patch set. Currently we just can't
> store any persistent properties in struct pathgroup.

Oh, yeah. nuts.

> > but I suppose that there could be a benefit to re-enabling the
> > group as soon as possible. I'm still kinda fuzzy on whether the
> > kernel's
> > own pathgroup re-enabling code makes all this redundant).
> 
> The kernel will even try "bypassed" PGs if it finds no other. But that
> means that e.g. the marginal pathgroup would be tried before disabled
> PGs [1]. In general, I don't think we can rely on the kernel in this
> respect.
>
> >  Then, in
> > enable_pathrgoups(), instead of checking each path, we just need to
> > check if pgp->need_reenable is set for the pathgroup().
> 
> As I said, as long as we discard the PGs, I guess we'll have to test
> the path flags. It's difficult to come up with a decent test case for
> this, in particular if checking for PATH_UP is not enough...
> 
> 
> > [...]
> > 
> > But perhaps the best answer is to just to say that this is a corner
> > case
> > where we skip a multipathd action that might be totally unnecessary.
> > And
> > even if it is a problem, it will be fixed if multipathd ever decides
> > that the kernel is using the wrong pathgroup and should switch, or
> > whenever the table gets reloaded. Maybe the whole patch is
> > unnecessary.
> 
> I'm going to submit a v4 that adds a test for the checker flags.

Seems reasonable.

> 
> Btw, I could only find one place in the kernel code where the kernel
> actively sets the "bypassed" flag, when the SCSI device handler returns
> SCSL_DH_DEV_TEMP_BUSY [2]. This seems rather questionable to me. First
> of all, this seems to assume that PGs are mapped to "controllers", i.e.
> something like "group_by_tpg", which isn't necessarily the case.
> Second, while the EMC device handler uses this error code in this way,
> in the ALUA handler it rather means a memory allocation failure, and
> the other device handlers don't support it at all.

Yeah, I really wish I knew how this was supposed to work. For the ALUA
devices, it's likely fine to clear bypassed when the device is in
PATH_UP. Either the hardware controller just ran into a temporary error,
and we should go back to using this pathgroup based on its priority, or
there is a persistent memory issue, it which case it likey doesn't
matter what we do, since the system has larger problems than using a
non-optimal pathgroup.

It's the EMC devices that seem like there should be a real right and
wrong way to do things. For instance, even though the code doesn't
currently clear bypassed for PATH_GHOST devices, that might be the right
thing to do.  Imagine if you tried to switch to a new pathgroup, but the
controller had some issue and so you bypassed it, and that issue was
later resolved.  In that case, you want to clear the bypassed flag. But
since you are using some other controller, the paths in the bypassed
pathgroup could well be in PATH_GHOST. This means that you are less
likely to pick that pathgroup in the future, so the bypass flag won't
get cleared. And multipathd won't do anything to fix this. So the
correct answer might be to just check for
pp->is_checked == CHECK_PATH_NEW_UP. But maybe not. I dunno.

> I think we can conclude that the kernel setting "bypassed" on a path
> group by itself is indeed a corner case, except for EMC Clariion, which
> is quite dated by now.
> 
> > Thoughts? I'm clearly thinking too much.
> > 
> 
> No, you are not. I strongly appreciate these comments.
> 
> Regards,
> Martin
> 
> [1] Unrelated wild thought: in view of the logic the kernel applies for
> "bypassed" PGs, it might actually make sense to use this flag for 
> marginal pathgroups. They would be tried if all else fails, which is
> more or less the behavior we want for them. What do you think?

Maybe, but I kind of doubt it. It will get cleared by the kernel if we
ever call switch_pathgroup(), but we could reset it. AFAICS the only
real "benefit" would be that regular bypassed pathgroups would get used
instead of marginal pathgroups. But if we're doing the right thing with
clearing the bypassed flag, then when a pathgroup is bypassed, it likely
can't be initialized (at least for the EMC handler). It might make more
sense to use a marginal pathgroup that can be initialized instead of a
regular pathgroup that quite possibly can't at all.

-Ben

> [2] https://elixir.bootlin.com/linux/v6.12.6/source/drivers/md/dm-mpath.c#L1564