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

[PATCH 00/13] 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].

The first patch implements the remark I had on patch 2 on Ben's set.

Patch 2-4 add map reload logic for the case when update_pathvec_from_dm()
detects an inconsistency: in this case, the map is now reloaded before
the checkers are actually started. If the inconsistency can't be cleared,
another attempt will be made when the checkers have finished.

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.
The last patch changes the reload action for the ghost delay tick.

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

Martin Wilck (13):
  multipathd: don't reload map in update_mpp_prio()
  multipathd: remove dm_get_info() call from refresh_multipath()
  multipathd: allow map removal in do_sync_mpp()
  multipathd: reload maps in do_sync_mpp() if necessary
  multipathd: move yielding for waiters to start of checkerloop
  multipathd: add checker_finished()
  multipathd: move "tick" calls into checker_finished()
  multipathd: remove mpvec_garbage_collector()
  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/main.c | 295 +++++++++++++++++++---------------------------
 1 file changed, 120 insertions(+), 175 deletions(-)

-- 
2.47.0

[PATCH 01/13] 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>
---
 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.0

[PATCH 02/13] 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>
---
 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.0

[PATCH 03/13] multipathd: allow map removal in do_sync_mpp()

[Martin Wilck]

We previously didn't allow map removal inside the checker loop. But
with the late updates to the checkerloop code, it should be safe to orphan
paths and delete maps even in this situation. We remove such maps everywhere
else in the code already, whenever refresh_multipath() or setup_multipath()
is called.

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

diff --git a/multipathd/main.c b/multipathd/main.c
index 4a28fbb..131dab6 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2446,34 +2446,30 @@ get_new_state(struct path *pp)
 	return newstate;
 }
 
-static void
-do_sync_mpp(struct vectors * vecs, struct multipath *mpp)
+/* Returns true if the mpp was deleted */
+static int
+do_sync_mpp(struct vectors *vecs, struct multipath *mpp)
 {
-	int i, ret;
-	struct path *pp;
+	int ret;
+
+	ret = refresh_multipath(vecs, mpp);
+	if (ret)
+		return ret;
 
-	ret = update_multipath_strings(mpp, vecs->pathvec);
-	if (ret != DMP_OK) {
-		condlog(1, "%s: %s", mpp->alias, ret == DMP_NOT_FOUND ?
-			"device not found" :
-			"couldn't synchronize with kernel state");
-		vector_foreach_slot (mpp->paths, pp, i)
-			pp->dmstate = PSTATE_UNDEF;
-		return;
-	}
 	set_no_path_retry(mpp);
+	return 0;
 }
 
-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)
-		return;
+		return 0;
 
-	do_sync_mpp(vecs, mpp);
+	return do_sync_mpp(vecs, mpp);
 }
 
 static int
@@ -2513,12 +2509,10 @@ 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 (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;
-	}
+		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 */
@@ -3018,8 +3012,11 @@ checkerloop (void *ap)
 				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;
+					if (sync_mpp(vecs, mpp, ticks))
+						/* map deleted */
+						i--;
+					else
+						mpp->prio_update = PRIO_UPDATE_NONE;
 				}
 				vector_foreach_slot(vecs->pathvec, pp, i)
 					pp->is_checked = CHECK_PATH_UNCHECKED;
-- 
2.47.0

Re: [PATCH 03/13] multipathd: allow map removal in do_sync_mpp()

[Benjamin Marzinski]

On Sat, Dec 07, 2024 at 12:36:07AM +0100, Martin Wilck wrote:
> We previously didn't allow map removal inside the checker loop. But
> with the late updates to the checkerloop code, it should be safe to orphan
> paths and delete maps even in this situation. We remove such maps everywhere
> else in the code already, whenever refresh_multipath() or setup_multipath()
> is called.

I don't think that this is safe. It's possible that the multipath device
has paths in the INIT_REMOVED or INIT_PARTIAL state. These will get
silently removed from the pathvec if we remove the map here. This will
mess up our iteration through the pathvec in update_paths(). Perhaps a
better idea would be to set mpp->sync_ticks to 0 if
update_multipath_strings() fails in do_sync_mpp(). This would force a
refresh by sync_mpp() at the start of the next loop in checkerloop(),
where it can safely remove the multipath device.

-Ben
 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipathd/main.c | 43 ++++++++++++++++++++-----------------------
>  1 file changed, 20 insertions(+), 23 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 4a28fbb..131dab6 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -2446,34 +2446,30 @@ get_new_state(struct path *pp)
>  	return newstate;
>  }
>  
> -static void
> -do_sync_mpp(struct vectors * vecs, struct multipath *mpp)
> +/* Returns true if the mpp was deleted */
> +static int
> +do_sync_mpp(struct vectors *vecs, struct multipath *mpp)
>  {
> -	int i, ret;
> -	struct path *pp;
> +	int ret;
> +
> +	ret = refresh_multipath(vecs, mpp);
> +	if (ret)
> +		return ret;
>  
> -	ret = update_multipath_strings(mpp, vecs->pathvec);
> -	if (ret != DMP_OK) {
> -		condlog(1, "%s: %s", mpp->alias, ret == DMP_NOT_FOUND ?
> -			"device not found" :
> -			"couldn't synchronize with kernel state");
> -		vector_foreach_slot (mpp->paths, pp, i)
> -			pp->dmstate = PSTATE_UNDEF;
> -		return;
> -	}
>  	set_no_path_retry(mpp);
> +	return 0;
>  }
>  
> -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)
> -		return;
> +		return 0;
>  
> -	do_sync_mpp(vecs, mpp);
> +	return do_sync_mpp(vecs, mpp);
>  }
>  
>  static int
> @@ -2513,12 +2509,10 @@ 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 (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;
> -	}
> +		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 */
> @@ -3018,8 +3012,11 @@ checkerloop (void *ap)
>  				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;
> +					if (sync_mpp(vecs, mpp, ticks))
> +						/* map deleted */
> +						i--;
> +					else
> +						mpp->prio_update = PRIO_UPDATE_NONE;
>  				}
>  				vector_foreach_slot(vecs->pathvec, pp, i)
>  					pp->is_checked = CHECK_PATH_UNCHECKED;
> -- 
> 2.47.0

Re: [PATCH 03/13] multipathd: allow map removal in do_sync_mpp()

[Benjamin Marzinski]

On Tue, Dec 10, 2024 at 02:02:32PM -0500, Benjamin Marzinski wrote:
> On Sat, Dec 07, 2024 at 12:36:07AM +0100, Martin Wilck wrote:
> > We previously didn't allow map removal inside the checker loop. But
> > with the late updates to the checkerloop code, it should be safe to orphan
> > paths and delete maps even in this situation. We remove such maps everywhere
> > else in the code already, whenever refresh_multipath() or setup_multipath()
> > is called.
> 
> I don't think that this is safe. It's possible that the multipath device
> has paths in the INIT_REMOVED or INIT_PARTIAL state. These will get
> silently removed from the pathvec if we remove the map here. This will
> mess up our iteration through the pathvec in update_paths(). Perhaps a
> better idea would be to set mpp->sync_ticks to 0 if
> update_multipath_strings() fails in do_sync_mpp(). This would force a
> refresh by sync_mpp() at the start of the next loop in checkerloop(),
> where it can safely remove the multipath device.

If we go this route we should probably rename do_sync_map() to
sync_map() and rename sync_map() to something like check_refresh_map(),
since it would now be calling refresh_map() instead of do_sync_map().

> 
> -Ben
>  
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  multipathd/main.c | 43 ++++++++++++++++++++-----------------------
> >  1 file changed, 20 insertions(+), 23 deletions(-)
> > 
> > diff --git a/multipathd/main.c b/multipathd/main.c
> > index 4a28fbb..131dab6 100644
> > --- a/multipathd/main.c
> > +++ b/multipathd/main.c
> > @@ -2446,34 +2446,30 @@ get_new_state(struct path *pp)
> >  	return newstate;
> >  }
> >  
> > -static void
> > -do_sync_mpp(struct vectors * vecs, struct multipath *mpp)
> > +/* Returns true if the mpp was deleted */
> > +static int
> > +do_sync_mpp(struct vectors *vecs, struct multipath *mpp)
> >  {
> > -	int i, ret;
> > -	struct path *pp;
> > +	int ret;
> > +
> > +	ret = refresh_multipath(vecs, mpp);
> > +	if (ret)
> > +		return ret;
> >  
> > -	ret = update_multipath_strings(mpp, vecs->pathvec);
> > -	if (ret != DMP_OK) {
> > -		condlog(1, "%s: %s", mpp->alias, ret == DMP_NOT_FOUND ?
> > -			"device not found" :
> > -			"couldn't synchronize with kernel state");
> > -		vector_foreach_slot (mpp->paths, pp, i)
> > -			pp->dmstate = PSTATE_UNDEF;
> > -		return;
> > -	}
> >  	set_no_path_retry(mpp);
> > +	return 0;
> >  }
> >  
> > -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)
> > -		return;
> > +		return 0;
> >  
> > -	do_sync_mpp(vecs, mpp);
> > +	return do_sync_mpp(vecs, mpp);
> >  }
> >  
> >  static int
> > @@ -2513,12 +2509,10 @@ 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 (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;
> > -	}
> > +		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 */
> > @@ -3018,8 +3012,11 @@ checkerloop (void *ap)
> >  				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;
> > +					if (sync_mpp(vecs, mpp, ticks))
> > +						/* map deleted */
> > +						i--;
> > +					else
> > +						mpp->prio_update = PRIO_UPDATE_NONE;
> >  				}
> >  				vector_foreach_slot(vecs->pathvec, pp, i)
> >  					pp->is_checked = CHECK_PATH_UNCHECKED;
> > -- 
> > 2.47.0

Re: [PATCH 03/13] multipathd: allow map removal in do_sync_mpp()

[Martin Wilck]

On Tue, 2024-12-10 at 14:02 -0500, Benjamin Marzinski wrote:
> On Sat, Dec 07, 2024 at 12:36:07AM +0100, Martin Wilck wrote:
> > We previously didn't allow map removal inside the checker loop. But
> > with the late updates to the checkerloop code, it should be safe to
> > orphan
> > paths and delete maps even in this situation. We remove such maps
> > everywhere
> > else in the code already, whenever refresh_multipath() or
> > setup_multipath()
> > is called.
> 
> I don't think that this is safe. It's possible that the multipath
> device
> has paths in the INIT_REMOVED or INIT_PARTIAL state. These will get
> silently removed from the pathvec if we remove the map here. This
> will
> mess up our iteration through the pathvec in update_paths(). 

Hm. You're right. But that applies to the current code in 0.11.0 PR as
well, because we'd call 

   do_sync_mpp()
      update_multipath_strings() 
         sync_paths()
            check_removed_paths()
               vector_del_slot(pathvec, i--);

Or am I missing something?

It seems to me that the only safe way to handle this is to refrain from
deleting paths from the pathvec anywhere deep in the call stack. Even
if we can avoid this situation now by moving the sync towards the end
of the checker loop, I believe that in the long run we need to fix
these traps in our code, because it's just so easy to get this wrong.

I wonder if we need yet another path state, of if we could simply set
these entries in the pathvec to NULL. That sounds crazy, but it might
actually be doable. Not 0.11.0 material, though.

> Perhaps a
> better idea would be to set mpp->sync_ticks to 0 if
> update_multipath_strings() fails in do_sync_mpp(). This would force a
> refresh by sync_mpp() at the start of the next loop in checkerloop(),
> where it can safely remove the multipath device.

I like the idea of your other post to move the sync to the
CHECKER_FINISHED state.

Thanks,
Martin

Re: [PATCH 03/13] multipathd: allow map removal in do_sync_mpp()

[Benjamin Marzinski]

On Tue, Dec 10, 2024 at 10:05:14PM +0100, Martin Wilck wrote:
> On Tue, 2024-12-10 at 14:02 -0500, Benjamin Marzinski wrote:
> > On Sat, Dec 07, 2024 at 12:36:07AM +0100, Martin Wilck wrote:
> > > We previously didn't allow map removal inside the checker loop. But
> > > with the late updates to the checkerloop code, it should be safe to
> > > orphan
> > > paths and delete maps even in this situation. We remove such maps
> > > everywhere
> > > else in the code already, whenever refresh_multipath() or
> > > setup_multipath()
> > > is called.
> > 
> > I don't think that this is safe. It's possible that the multipath
> > device
> > has paths in the INIT_REMOVED or INIT_PARTIAL state. These will get
> > silently removed from the pathvec if we remove the map here. This
> > will
> > mess up our iteration through the pathvec in update_paths(). 
> 
> Hm. You're right. But that applies to the current code in 0.11.0 PR as
> well, because we'd call 
> 
>    do_sync_mpp()
>       update_multipath_strings() 
>          sync_paths()
>             check_removed_paths()
>                vector_del_slot(pathvec, i--);
> 
> Or am I missing something?

Nope. Your right. Nuts.
 
> It seems to me that the only safe way to handle this is to refrain from
> deleting paths from the pathvec anywhere deep in the call stack. Even
> if we can avoid this situation now by moving the sync towards the end
> of the checker loop, I believe that in the long run we need to fix
> these traps in our code, because it's just so easy to get this wrong.
> 
> I wonder if we need yet another path state, of if we could simply set
> these entries in the pathvec to NULL. That sounds crazy, but it might
> actually be doable. Not 0.11.0 material, though.

I think we could just not call check_removed_paths() in sync_paths(). We
would still orphan all the paths that were no longer part of the
multipath device, and set pp->mpp for all the paths that are part of the
device just like before, but we wouldn't delete the paths from
pathvec there. Instead we would call check_removed_paths() in
refresh_multipath(), so we did it after loads and in update_multipath.

I'm pretty sure that should be fine. If the device table changed and
removed a path so that we can free it, either we reloaded the device,
and we will call setup_multipath() after the reload, or something
external did, and multipathd will see an event for that and call
setup_multipath() via update_multipath().

Does that make sense?

> > Perhaps a
> > better idea would be to set mpp->sync_ticks to 0 if
> > update_multipath_strings() fails in do_sync_mpp(). This would force a
> > refresh by sync_mpp() at the start of the next loop in checkerloop(),
> > where it can safely remove the multipath device.
> 
> I like the idea of your other post to move the sync to the
> CHECKER_FINISHED state.
> 
> Thanks,
> Martin
> 

Re: [PATCH 03/13] multipathd: allow map removal in do_sync_mpp()

[Martin Wilck]

On Tue, 2024-12-10 at 17:49 -0500, Benjamin Marzinski wrote:
> On Tue, Dec 10, 2024 at 10:05:14PM +0100, Martin Wilck wrote:
> > On Tue, 2024-12-10 at 14:02 -0500, Benjamin Marzinski wrote:
> > > On Sat, Dec 07, 2024 at 12:36:07AM +0100, Martin Wilck wrote:
> > > > We previously didn't allow map removal inside the checker loop.
> > > > But
> > > > with the late updates to the checkerloop code, it should be
> > > > safe to
> > > > orphan
> > > > paths and delete maps even in this situation. We remove such
> > > > maps
> > > > everywhere
> > > > else in the code already, whenever refresh_multipath() or
> > > > setup_multipath()
> > > > is called.
> > > 
> > > I don't think that this is safe. It's possible that the multipath
> > > device
> > > has paths in the INIT_REMOVED or INIT_PARTIAL state. These will
> > > get
> > > silently removed from the pathvec if we remove the map here. This
> > > will
> > > mess up our iteration through the pathvec in update_paths(). 
> > 
> > Hm. You're right. But that applies to the current code in 0.11.0 PR
> > as
> > well, because we'd call 
> > 
> >    do_sync_mpp()
> >       update_multipath_strings() 
> >          sync_paths()
> >             check_removed_paths()
> >                vector_del_slot(pathvec, i--);
> > 
> > Or am I missing something?
> 
> Nope. Your right. Nuts.
>  
> > It seems to me that the only safe way to handle this is to refrain
> > from
> > deleting paths from the pathvec anywhere deep in the call stack.
> > Even
> > if we can avoid this situation now by moving the sync towards the
> > end
> > of the checker loop, I believe that in the long run we need to fix
> > these traps in our code, because it's just so easy to get this
> > wrong.
> > 
> > I wonder if we need yet another path state, of if we could simply
> > set
> > these entries in the pathvec to NULL. That sounds crazy, but it
> > might
> > actually be doable. Not 0.11.0 material, though.
> 
> I think we could just not call check_removed_paths() in sync_paths().
> We
> would still orphan all the paths that were no longer part of the
> multipath device, and set pp->mpp for all the paths that are part of
> the
> device just like before, but we wouldn't delete the paths from
> pathvec there. Instead we would call check_removed_paths() in
> refresh_multipath(), so we did it after loads and in
> update_multipath.
> 
> I'm pretty sure that should be fine. If the device table changed and
> removed a path so that we can free it, either we reloaded the device,
> and we will call setup_multipath() after the reload, or something
> external did, and multipathd will see an event for that and call
> setup_multipath() via update_multipath().
> 
> Does that make sense?

I think we'll be fine with my upcoming patch set, which will call
reload_and_sync_map() only from checker_finished(). I don't change
sync_paths() in this set so far.

I'm a little concerned about refresh_multipath() and
reload_and_sync_map() being called from various CLI functions. But I
won't start digging into that now. Maps may get removed in CLI calls,
so what.

Thanks,
Martin

Re: [PATCH 03/13] multipathd: allow map removal in do_sync_mpp()

[Benjamin Marzinski]

On Sat, Dec 07, 2024 at 12:36:07AM +0100, Martin Wilck wrote:
> We previously didn't allow map removal inside the checker loop. But
> with the late updates to the checkerloop code, it should be safe to orphan
> paths and delete maps even in this situation. We remove such maps everywhere
> else in the code already, whenever refresh_multipath() or setup_multipath()
> is called.

Actually, thinking about this more, what do we get by proactively
deleting the multipath device if something goes wrong in the checker? If
we successfully reload a device, but can't sync it with the kernel,
that's one thing, But that was triggered by a change in the device, and
we know that when we reloaded the device, device-mapper was working. I'm
leery of possibly deleting the map because of a transient device-mapper
issue.  I'm not sure if on a check that we do repeatedly, we should
delete the device on an error.  We haven't in the past, and as far as I
know, it doesn't cause problems.  

Without a benefit to doing this, I'm not sure it makes sense.

-Ben

> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipathd/main.c | 43 ++++++++++++++++++++-----------------------
>  1 file changed, 20 insertions(+), 23 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 4a28fbb..131dab6 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -2446,34 +2446,30 @@ get_new_state(struct path *pp)
>  	return newstate;
>  }
>  
> -static void
> -do_sync_mpp(struct vectors * vecs, struct multipath *mpp)
> +/* Returns true if the mpp was deleted */
> +static int
> +do_sync_mpp(struct vectors *vecs, struct multipath *mpp)
>  {
> -	int i, ret;
> -	struct path *pp;
> +	int ret;
> +
> +	ret = refresh_multipath(vecs, mpp);
> +	if (ret)
> +		return ret;
>  
> -	ret = update_multipath_strings(mpp, vecs->pathvec);
> -	if (ret != DMP_OK) {
> -		condlog(1, "%s: %s", mpp->alias, ret == DMP_NOT_FOUND ?
> -			"device not found" :
> -			"couldn't synchronize with kernel state");
> -		vector_foreach_slot (mpp->paths, pp, i)
> -			pp->dmstate = PSTATE_UNDEF;
> -		return;
> -	}
>  	set_no_path_retry(mpp);
> +	return 0;
>  }
>  
> -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)
> -		return;
> +		return 0;
>  
> -	do_sync_mpp(vecs, mpp);
> +	return do_sync_mpp(vecs, mpp);
>  }
>  
>  static int
> @@ -2513,12 +2509,10 @@ 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 (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;
> -	}
> +		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 */
> @@ -3018,8 +3012,11 @@ checkerloop (void *ap)
>  				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;
> +					if (sync_mpp(vecs, mpp, ticks))
> +						/* map deleted */
> +						i--;
> +					else
> +						mpp->prio_update = PRIO_UPDATE_NONE;
>  				}
>  				vector_foreach_slot(vecs->pathvec, pp, i)
>  					pp->is_checked = CHECK_PATH_UNCHECKED;
> -- 
> 2.47.0

Re: [PATCH 03/13] multipathd: allow map removal in do_sync_mpp()

[Martin Wilck]

On Tue, 2024-12-10 at 18:30 -0500, Benjamin Marzinski wrote:
> On Sat, Dec 07, 2024 at 12:36:07AM +0100, Martin Wilck wrote:
> > We previously didn't allow map removal inside the checker loop. But
> > with the late updates to the checkerloop code, it should be safe to
> > orphan
> > paths and delete maps even in this situation. We remove such maps
> > everywhere
> > else in the code already, whenever refresh_multipath() or
> > setup_multipath()
> > is called.
> 
> Actually, thinking about this more, what do we get by proactively
> deleting the multipath device if something goes wrong in the checker?
> If
> we successfully reload a device, but can't sync it with the kernel,
> that's one thing, But that was triggered by a change in the device,
> and
> we know that when we reloaded the device, device-mapper was working.
> I'm
> leery of possibly deleting the map because of a transient device-
> mapper
> issue.  I'm not sure if on a check that we do repeatedly, we should
> delete the device on an error.  We haven't in the past, and as far as
> I
> know, it doesn't cause problems.  

I don't disagree. But the same can be said for basically all call
chains where setup_multipath() is called for an existing map. I was
just following the pattern that we use e.g. in ev_add_path(), or in
update_mpp_prio(). Why would we treat the checker and path addition
differently in this respect?

If we look at this pragmatically (assuming that multipathd gets the
parameters right), the most probable reason for a map reload failure is
failure to open a path device in bdev_open(), either because the device
doesn't exist, or because it's busy or otherwise unavailable. If this
happens in ev_add_path(), the likely reason is that the path just added
was busy, and the smartest action upon such a failure would probably be
to just undo that addition. We currently don't do that; we remove the
entire map, which is questionable, as you state correctly.

In the checker, this can't happen. Obviously, no other process can grab
a path device while the device mapper is holding it, so -EBUSY won't
occur if we reload an existing map. Even device deletion doesn't cause
failure on reload. It is possible to delete a SCSI device while it's
mapped, and execute a table reload / suspend / resume cycle on the map
while referencing the deleted device. The kernel keeps holding the
reference to the deleted device, and will simply mark it as
failed. This holds also if the mapped paths are re-grouped or re-
ordered in the table. Failure occurs only if we temporarily remove the
device from the map and re-add it, because as soon as the device is
removed from the map's dm table, its refcount drops to zero, and it's
gone for good.

IOW, reloading a map with a table containing only already-mapped
devices will never fail, except in extreme situations like kernel OOM.

Thus, AFAICS, the only relevant scenario where a reloading would fail
is trying to add a path device that was not previously mapped, and
that's either busy (perhaps in another map) or has been deleted, IOW
only when we reload after calling adopt_paths(). This is where we could
improve. If we fail to reload after adopting new paths, we could fall
back to the existing table, and perhaps try to add paths one by one.
Again, this is post-0.11 material.

OTOH, practially impossible is not totally impossible, so we need to be
prepared to map reload failure either way. IMO the best thing we can do
in this case is to keep using the kernel's map, and retry reloading
later. 

The only critical situation is WWID change of path devices. We must try
to fix this situation ASAP when we detect it. I'm unsure what the best
action is if a reload fails in that situation, though (other than
failing the path, as we already do).

Martin

Re: [PATCH 03/13] multipathd: allow map removal in do_sync_mpp()

[Benjamin Marzinski]

On Wed, Dec 11, 2024 at 01:06:46PM +0100, Martin Wilck wrote:
> On Tue, 2024-12-10 at 18:30 -0500, Benjamin Marzinski wrote:
> > On Sat, Dec 07, 2024 at 12:36:07AM +0100, Martin Wilck wrote:
> > > We previously didn't allow map removal inside the checker loop. But
> > > with the late updates to the checkerloop code, it should be safe to
> > > orphan
> > > paths and delete maps even in this situation. We remove such maps
> > > everywhere
> > > else in the code already, whenever refresh_multipath() or
> > > setup_multipath()
> > > is called.
> > 
> > Actually, thinking about this more, what do we get by proactively
> > deleting the multipath device if something goes wrong in the checker?
> > If
> > we successfully reload a device, but can't sync it with the kernel,
> > that's one thing, But that was triggered by a change in the device,
> > and
> > we know that when we reloaded the device, device-mapper was working.
> > I'm
> > leery of possibly deleting the map because of a transient device-
> > mapper
> > issue.  I'm not sure if on a check that we do repeatedly, we should
> > delete the device on an error.  We haven't in the past, and as far as
> > I
> > know, it doesn't cause problems.  
> 
> I don't disagree. But the same can be said for basically all call
> chains where setup_multipath() is called for an existing map. I was
> just following the pattern that we use e.g. in ev_add_path(), or in
> update_mpp_prio(). Why would we treat the checker and path addition
> differently in this respect?

I'm confused here. ev_add_path() doesn't remove the device if the reload
fails. If a reload fails, the table should stay the same. That's why I
said that in other cases where we delete the device, we know that when
we just reloaded the device, device-mapper was working. Looking at the
code, that isn't really true. After failed reloads, we still call
setup_multipath to update our state, and we will delete the device if
that fails.
 
> If we look at this pragmatically (assuming that multipathd gets the
> parameters right), the most probable reason for a map reload failure is
> failure to open a path device in bdev_open(), either because the device
> doesn't exist, or because it's busy or otherwise unavailable. If this
> happens in ev_add_path(), the likely reason is that the path just added
> was busy, and the smartest action upon such a failure would probably be
> to just undo that addition. We currently don't do that; we remove the
> entire map, which is questionable, as you state correctly.

This is why we call setup_multipath after failed reloads, to make sure
multipathd's view of the multipath device resyncs with the kernel's,
which hasn't changed from what it was before the reload failed.

> In the checker, this can't happen. Obviously, no other process can grab
> a path device while the device mapper is holding it, so -EBUSY won't
> occur if we reload an existing map. Even device deletion doesn't cause
> failure on reload. It is possible to delete a SCSI device while it's
> mapped, and execute a table reload / suspend / resume cycle on the map
> while referencing the deleted device. The kernel keeps holding the
> reference to the deleted device, and will simply mark it as
> failed. This holds also if the mapped paths are re-grouped or re-
> ordered in the table. Failure occurs only if we temporarily remove the
> device from the map and re-add it, because as soon as the device is
> removed from the map's dm table, its refcount drops to zero, and it's
> gone for good.
> 
> IOW, reloading a map with a table containing only already-mapped
> devices will never fail, except in extreme situations like kernel OOM.

Maybe I should clarify my position a bit. I am fine with reloading the
device in the checkerloop if something has changed. This obviously
does run a very small risk of something going wrong and a device getting
removed unnecessarily, but we know that we need to reload the device, so
we should.

What I would rather avoid is reloading the device because we failed to
get it's state in do_sync_mpp(). We don't do this because we know that
something has changed. We do this as a safety measure to deal with
corner cases where our state doesn't match the kernel's and we didn't
get an event. Double checking this each time we check a path in a
device saves having to catch all these corner cases elsewhere. But it's
almost always completely unnecessary, and we're doing it on every
multipath device every couple of seconds, unlike reloading a device,
which is rare.

> Thus, AFAICS, the only relevant scenario where a reloading would fail
> is trying to add a path device that was not previously mapped, and
> that's either busy (perhaps in another map) or has been deleted, IOW
> only when we reload after calling adopt_paths(). This is where we could
> improve. If we fail to reload after adopting new paths, we could fall
> back to the existing table, and perhaps try to add paths one by one.
> Again, this is post-0.11 material.
> 
> OTOH, practially impossible is not totally impossible, so we need to be
> prepared to map reload failure either way. IMO the best thing we can do
> in this case is to keep using the kernel's map, and retry reloading
> later. 

I'm not actually worried about the kernel so much as libdevmapper. It is
not designed for multi-threaded processes, and that has bitten us in the
past. For intance, it's why we don't delete devices in dmevent_loop() on
libdevmapper errors. dm_get_events() just waits and retries if getting
the device list fails, and for each device, it calls dm_is_mpath and
will only delete a device on DM_IS_MPATH_NO, which is what I suggested
for the cleanup function.

I'm pretty sure we've handled all of the known issues here, with fixes
like:
02d4bf07 ("libmultipath: protect racy libdevmapper calls with a mutex")
34e01d2f ("multipath-tools: don't call dm_lib_release() any more")

I'd rather not risk having missed some issue that could cause a
temporary error in a function that we call every couple of seconds
(almost always unnecessarily).

-Ben

> The only critical situation is WWID change of path devices. We must try
> to fix this situation ASAP when we detect it. I'm unsure what the best
> action is if a reload fails in that situation, though (other than
> failing the path, as we already do).
> 
> Martin

Re: [PATCH 03/13] multipathd: allow map removal in do_sync_mpp()

[Martin Wilck]

On Wed, 2024-12-11 at 12:09 -0500, Benjamin Marzinski wrote:
> On Wed, Dec 11, 2024 at 01:06:46PM +0100, Martin Wilck wrote:
> > On Tue, 2024-12-10 at 18:30 -0500, Benjamin Marzinski wrote:
> > > On Sat, Dec 07, 2024 at 12:36:07AM +0100, Martin Wilck wrote:
> > > > We previously didn't allow map removal inside the checker loop.
> > > > But
> > > > with the late updates to the checkerloop code, it should be
> > > > safe to
> > > > orphan
> > > > paths and delete maps even in this situation. We remove such
> > > > maps
> > > > everywhere
> > > > else in the code already, whenever refresh_multipath() or
> > > > setup_multipath()
> > > > is called.
> > > 
> > > Actually, thinking about this more, what do we get by proactively
> > > deleting the multipath device if something goes wrong in the
> > > checker?
> > > If
> > > we successfully reload a device, but can't sync it with the
> > > kernel,
> > > that's one thing, But that was triggered by a change in the
> > > device,
> > > and
> > > we know that when we reloaded the device, device-mapper was
> > > working.
> > > I'm
> > > leery of possibly deleting the map because of a transient device-
> > > mapper
> > > issue.  I'm not sure if on a check that we do repeatedly, we
> > > should
> > > delete the device on an error.  We haven't in the past, and as
> > > far as
> > > I
> > > know, it doesn't cause problems.  
> > 
> > I don't disagree. But the same can be said for basically all call
> > chains where setup_multipath() is called for an existing map. I was
> > just following the pattern that we use e.g. in ev_add_path(), or in
> > update_mpp_prio(). Why would we treat the checker and path addition
> > differently in this respect?
> 
> I'm confused here. 

Well, I was writing confused things. My thinking was going in circles
about the removal of paths and maps, and I didn't properly distinguish
between map reloading and updating the state from the kernel.

Sorry.

> ev_add_path() doesn't remove the device if the reload
> fails. If a reload fails, the table should stay the same. That's why
> I
> said that in other cases where we delete the device, we know that
> when
> we just reloaded the device, device-mapper was working. Looking at
> the
> code, that isn't really true. After failed reloads, we still call
> setup_multipath to update our state, and we will delete the device if
> that fails.


> This is why we call setup_multipath after failed reloads, to make
> sure
> multipathd's view of the multipath device resyncs with the kernel's,
> which hasn't changed from what it was before the reload failed.

Right.

> > In the checker, this can't happen. Obviously, no other process can
> > grab
> > a path device while the device mapper is holding it, so -EBUSY
> > won't
> > occur if we reload an existing map. Even device deletion doesn't
> > cause
> > failure on reload. It is possible to delete a SCSI device while
> > it's
> > mapped, and execute a table reload / suspend / resume cycle on the
> > map
> > while referencing the deleted device. The kernel keeps holding the
> > reference to the deleted device, and will simply mark it as
> > failed. This holds also if the mapped paths are re-grouped or re-
> > ordered in the table. Failure occurs only if we temporarily remove
> > the
> > device from the map and re-add it, because as soon as the device is
> > removed from the map's dm table, its refcount drops to zero, and
> > it's
> > gone for good.
> > 
> > IOW, reloading a map with a table containing only already-mapped
> > devices will never fail, except in extreme situations like kernel
> > OOM.
> 
> Maybe I should clarify my position a bit. I am fine with reloading
> the
> device in the checkerloop if something has changed. This obviously
> does run a very small risk of something going wrong and a device
> getting
> removed unnecessarily, but we know that we need to reload the device,
> so
> we should.
> 
> What I would rather avoid is reloading the device because we failed
> to
> get it's state in do_sync_mpp().

FTR, in my v4 patchset, I won't try to do that any more.

> I'm not actually worried about the kernel so much as libdevmapper. It
> is
> not designed for multi-threaded processes, and that has bitten us in
> the
> past. For intance, it's why we don't delete devices in dmevent_loop()
> on
> libdevmapper errors. dm_get_events() just waits and retries if
> getting
> the device list fails, and for each device, it calls dm_is_mpath and
> will only delete a device on DM_IS_MPATH_NO, which is what I
> suggested
> for the cleanup function.
> 
> I'm pretty sure we've handled all of the known issues here, with
> fixes
> like:
> 02d4bf07 ("libmultipath: protect racy libdevmapper calls with a
> mutex")
> 34e01d2f ("multipath-tools: don't call dm_lib_release() any more")
> 
> I'd rather not risk having missed some issue that could cause a
> temporary error in a function that we call every couple of seconds
> (almost always unnecessarily).

Ok, getting it. I thought that an error in DM_TABLE_STATUS must almost
neccessarily mean -ENXIO (from the kernel pov), which would mean that
some external entity removed the device, and that we should act as if
someone had used the "remove map" CLI command. But I didn't think about
libdevmapper.

Martin

Re: [PATCH 03/13] multipathd: allow map removal in do_sync_mpp()

[Martin Wilck]

On Wed, 2024-12-11 at 21:20 +0100, Martin Wilck wrote:
> On Wed, 2024-12-11 at 12:09 -0500, Benjamin Marzinski wrote:
> 
> 
> > I'm not actually worried about the kernel so much as libdevmapper.
> > It
> > is
> > not designed for multi-threaded processes, and that has bitten us
> > in
> > the
> > past. For intance, it's why we don't delete devices in
> > dmevent_loop()
> > on
> > libdevmapper errors. dm_get_events() just waits and retries if
> > getting
> > the device list fails, and for each device, it calls dm_is_mpath
> > and
> > will only delete a device on DM_IS_MPATH_NO, which is what I
> > suggested
> > for the cleanup function.
> > 
> > I'm pretty sure we've handled all of the known issues here, with
> > fixes
> > like:
> > 02d4bf07 ("libmultipath: protect racy libdevmapper calls with a
> > mutex")
> > 34e01d2f ("multipath-tools: don't call dm_lib_release() any more")
> > 
> > I'd rather not risk having missed some issue that could cause a
> > temporary error in a function that we call every couple of seconds
> > (almost always unnecessarily).
> 
> Ok, getting it. I thought that an error in DM_TABLE_STATUS must
> almost
> neccessarily mean -ENXIO (from the kernel pov), which would mean that
> some external entity removed the device, and that we should act as if
> someone had used the "remove map" CLI command. But I didn't think
> about
> libdevmapper.

But will libdevmapper return ENXIO if it's somehow interally confused?
I don't think so. I believe that if we see this error code, removing
the map is the right thing to do.

I consider adding a patch on top of the v4 series that does this. 
If you reject it, fine :-)

Regards,
Martin

Re: [PATCH 03/13] multipathd: allow map removal in do_sync_mpp()

[Benjamin Marzinski]

On Wed, Dec 11, 2024 at 09:33:40PM +0100, Martin Wilck wrote:
> On Wed, 2024-12-11 at 21:20 +0100, Martin Wilck wrote:
> > On Wed, 2024-12-11 at 12:09 -0500, Benjamin Marzinski wrote:
> > 
> > 
> > > I'm not actually worried about the kernel so much as libdevmapper.
> > > It
> > > is
> > > not designed for multi-threaded processes, and that has bitten us
> > > in
> > > the
> > > past. For intance, it's why we don't delete devices in
> > > dmevent_loop()
> > > on
> > > libdevmapper errors. dm_get_events() just waits and retries if
> > > getting
> > > the device list fails, and for each device, it calls dm_is_mpath
> > > and
> > > will only delete a device on DM_IS_MPATH_NO, which is what I
> > > suggested
> > > for the cleanup function.
> > > 
> > > I'm pretty sure we've handled all of the known issues here, with
> > > fixes
> > > like:
> > > 02d4bf07 ("libmultipath: protect racy libdevmapper calls with a
> > > mutex")
> > > 34e01d2f ("multipath-tools: don't call dm_lib_release() any more")
> > > 
> > > I'd rather not risk having missed some issue that could cause a
> > > temporary error in a function that we call every couple of seconds
> > > (almost always unnecessarily).
> > 
> > Ok, getting it. I thought that an error in DM_TABLE_STATUS must
> > almost
> > neccessarily mean -ENXIO (from the kernel pov), which would mean that
> > some external entity removed the device, and that we should act as if
> > someone had used the "remove map" CLI command. But I didn't think
> > about
> > libdevmapper.
> 
> But will libdevmapper return ENXIO if it's somehow interally confused?
> I don't think so. I believe that if we see this error code, removing
> the map is the right thing to do.

I don't think that shouldn't ever happen.

https://github.com/lvmteam/lvm2/blob/928b8e9c6eaf871b3405b91c64eac5ea854f2572/device_mapper/ioctl/libdm-iface.c#L2100

If libdevmapper gets an ENXIO from the kernel, it ends up setting
dmi.exists to 0 instead of returning the error.

-Ben
 
> I consider adding a patch on top of the v4 series that does this. 
> If you reject it, fine :-)
> 
> Regards,
> Martin

Re: [PATCH 03/13] multipathd: allow map removal in do_sync_mpp()

[Martin Wilck]

On Thu, 2024-12-12 at 12:12 -0500, Benjamin Marzinski wrote:
> On Wed, Dec 11, 2024 at 09:33:40PM +0100, Martin Wilck wrote:
> > On Wed, 2024-12-11 at 21:20 +0100, Martin Wilck wrote:
> > > On Wed, 2024-12-11 at 12:09 -0500, Benjamin Marzinski wrote:
> > > 
> > > 
> > > > I'm not actually worried about the kernel so much as
> > > > libdevmapper.
> > > > It
> > > > is
> > > > not designed for multi-threaded processes, and that has bitten
> > > > us
> > > > in
> > > > the
> > > > past. For intance, it's why we don't delete devices in
> > > > dmevent_loop()
> > > > on
> > > > libdevmapper errors. dm_get_events() just waits and retries if
> > > > getting
> > > > the device list fails, and for each device, it calls
> > > > dm_is_mpath
> > > > and
> > > > will only delete a device on DM_IS_MPATH_NO, which is what I
> > > > suggested
> > > > for the cleanup function.
> > > > 
> > > > I'm pretty sure we've handled all of the known issues here,
> > > > with
> > > > fixes
> > > > like:
> > > > 02d4bf07 ("libmultipath: protect racy libdevmapper calls with a
> > > > mutex")
> > > > 34e01d2f ("multipath-tools: don't call dm_lib_release() any
> > > > more")
> > > > 
> > > > I'd rather not risk having missed some issue that could cause a
> > > > temporary error in a function that we call every couple of
> > > > seconds
> > > > (almost always unnecessarily).
> > > 
> > > Ok, getting it. I thought that an error in DM_TABLE_STATUS must
> > > almost
> > > neccessarily mean -ENXIO (from the kernel pov), which would mean
> > > that
> > > some external entity removed the device, and that we should act
> > > as if
> > > someone had used the "remove map" CLI command. But I didn't think
> > > about
> > > libdevmapper.
> > 
> > But will libdevmapper return ENXIO if it's somehow interally
> > confused?
> > I don't think so. I believe that if we see this error code,
> > removing
> > the map is the right thing to do.
> 
> I don't think that shouldn't ever happen.
> 
> https://github.com/lvmteam/lvm2/blob/928b8e9c6eaf871b3405b91c64eac5ea854f2572/device_mapper/ioctl/libdm-iface.c#L2100
> 
> If libdevmapper gets an ENXIO from the kernel, it ends up setting
> dmi.exists to 0 instead of returning the error.

I meant the kernel ioctl return code. Sorry for being unclear.

From the the point of view of libmultipath, it doesn't matter.
libmp_mapinfo tests for ENXIO and dmi.exists, and returns DMP_NOT_FOUND
in both cases.

Martin

Re: [PATCH 03/13] multipathd: allow map removal in do_sync_mpp()

[Benjamin Marzinski]

On Thu, Dec 12, 2024 at 06:18:40PM +0100, Martin Wilck wrote:
> On Thu, 2024-12-12 at 12:12 -0500, Benjamin Marzinski wrote:
> > On Wed, Dec 11, 2024 at 09:33:40PM +0100, Martin Wilck wrote:
> > > On Wed, 2024-12-11 at 21:20 +0100, Martin Wilck wrote:
> > > > On Wed, 2024-12-11 at 12:09 -0500, Benjamin Marzinski wrote:
> > > > 
> > > > 
> > > > > I'm not actually worried about the kernel so much as
> > > > > libdevmapper.
> > > > > It
> > > > > is
> > > > > not designed for multi-threaded processes, and that has bitten
> > > > > us
> > > > > in
> > > > > the
> > > > > past. For intance, it's why we don't delete devices in
> > > > > dmevent_loop()
> > > > > on
> > > > > libdevmapper errors. dm_get_events() just waits and retries if
> > > > > getting
> > > > > the device list fails, and for each device, it calls
> > > > > dm_is_mpath
> > > > > and
> > > > > will only delete a device on DM_IS_MPATH_NO, which is what I
> > > > > suggested
> > > > > for the cleanup function.
> > > > > 
> > > > > I'm pretty sure we've handled all of the known issues here,
> > > > > with
> > > > > fixes
> > > > > like:
> > > > > 02d4bf07 ("libmultipath: protect racy libdevmapper calls with a
> > > > > mutex")
> > > > > 34e01d2f ("multipath-tools: don't call dm_lib_release() any
> > > > > more")
> > > > > 
> > > > > I'd rather not risk having missed some issue that could cause a
> > > > > temporary error in a function that we call every couple of
> > > > > seconds
> > > > > (almost always unnecessarily).
> > > > 
> > > > Ok, getting it. I thought that an error in DM_TABLE_STATUS must
> > > > almost
> > > > neccessarily mean -ENXIO (from the kernel pov), which would mean
> > > > that
> > > > some external entity removed the device, and that we should act
> > > > as if
> > > > someone had used the "remove map" CLI command. But I didn't think
> > > > about
> > > > libdevmapper.
> > > 
> > > But will libdevmapper return ENXIO if it's somehow interally
> > > confused?
> > > I don't think so. I believe that if we see this error code,
> > > removing
> > > the map is the right thing to do.
> > 
> > I don't think that shouldn't ever happen.
> > 
> > https://github.com/lvmteam/lvm2/blob/928b8e9c6eaf871b3405b91c64eac5ea854f2572/device_mapper/ioctl/libdm-iface.c#L2100
> > 
> > If libdevmapper gets an ENXIO from the kernel, it ends up setting
> > dmi.exists to 0 instead of returning the error.
> 
> I meant the kernel ioctl return code. Sorry for being unclear.
> 
> >From the the point of view of libmultipath, it doesn't matter.
> libmp_mapinfo tests for ENXIO and dmi.exists, and returns DMP_NOT_FOUND
> in both cases.

Sure, that makes sense then. If libdevmapper told us the device doesn't
exist, we can safely remove it.

-Ben
 
> Martin

[PATCH 04/13] multipathd: reload maps in do_sync_mpp() if necessary

[Martin Wilck]

update_pathvec_from_dm() may set mpp->need_reload if it finds inconsistent
settings. In this case, the map should be reloaded, but so far we don't
do this reliably. A previous patch added a call to reload_and_sync_map()
in the CHECKER_FINISHED state, but in the mean time the checker may have
waited for checker threads to finish, and may have dropped and re-acquired the
vecs lock. As mpp->need_reload is a serious but rare condition, also try
to fix it early in the checker loop. Because of the previous patch, we
can call reload_and_sync_map() here.

Fixes: https://github.com/opensvc/multipath-tools/issues/105
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 131dab6..18083c7 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2450,12 +2450,25 @@ get_new_state(struct path *pp)
 static int
 do_sync_mpp(struct vectors *vecs, struct multipath *mpp)
 {
-	int ret;
+	const int MAX_RETRIES = 1;
+	int ret, retry = 0;
 
+try_again:
 	ret = refresh_multipath(vecs, mpp);
 	if (ret)
 		return ret;
 
+	else if (mpp->need_reload) {
+		if (retry++ < MAX_RETRIES) {
+			condlog(2, "%s: %s needs reload", __func__, mpp->alias);
+			if (reload_and_sync_map(mpp, vecs) == 2)
+				/* map removed */
+				return 1;
+			goto try_again;
+		} else
+			condlog(1, "%s: %s still needs reload after %d retries",
+				__func__, mpp->alias, retry);
+	}
 	set_no_path_retry(mpp);
 	return 0;
 }
-- 
2.47.0

Re: [PATCH 04/13] multipathd: reload maps in do_sync_mpp() if necessary

[Benjamin Marzinski]

On Sat, Dec 07, 2024 at 12:36:08AM +0100, Martin Wilck wrote:
> update_pathvec_from_dm() may set mpp->need_reload if it finds inconsistent
> settings. In this case, the map should be reloaded, but so far we don't
> do this reliably. A previous patch added a call to reload_and_sync_map()
> in the CHECKER_FINISHED state, but in the mean time the checker may have
> waited for checker threads to finish, and may have dropped and re-acquired the
> vecs lock. As mpp->need_reload is a serious but rare condition, also try
> to fix it early in the checker loop. Because of the previous patch, we
> can call reload_and_sync_map() here.

Again, if the map has any paths in the INIT_PARTIAL or INIT_REMOVED
state, the reload will remove them from the pathvec and mess up our
looping through it. Reloading maps with need_reload will already happen
at the end of this path check loop, so it will still get done very
quickly.

Also, since we try to get all the paths for a device checked in the same
checker loop now, it seems to me that there could a benefit to waiting
till the end, to sync our state with the kernel. This could avoid some
path ping-ponging in a corner case. If a path failed and the kernel
noticed it while we are in a checker loop, but haven't gotten to
update_path_state() for that path yet, the path will still appear up to
multipathd. In this case, if we reloaded the multipath device while
updating an earlier path in the device, we would reinstate this failed
path in sync_map_state(), only for us or the kernel to fail it again
right afterwards.

-Ben
 
> Fixes: https://github.com/opensvc/multipath-tools/issues/105
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipathd/main.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 131dab6..18083c7 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -2450,12 +2450,25 @@ get_new_state(struct path *pp)
>  static int
>  do_sync_mpp(struct vectors *vecs, struct multipath *mpp)
>  {
> -	int ret;
> +	const int MAX_RETRIES = 1;
> +	int ret, retry = 0;
>  
> +try_again:
>  	ret = refresh_multipath(vecs, mpp);
>  	if (ret)
>  		return ret;
>  
> +	else if (mpp->need_reload) {
> +		if (retry++ < MAX_RETRIES) {
> +			condlog(2, "%s: %s needs reload", __func__, mpp->alias);
> +			if (reload_and_sync_map(mpp, vecs) == 2)
> +				/* map removed */
> +				return 1;
> +			goto try_again;
> +		} else
> +			condlog(1, "%s: %s still needs reload after %d retries",
> +				__func__, mpp->alias, retry);
> +	}
>  	set_no_path_retry(mpp);
>  	return 0;
>  }
> -- 
> 2.47.0

[PATCH 05/13] 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>
---
 multipathd/main.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 18083c7..5f16094 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -3018,6 +3018,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();
@@ -3050,16 +3060,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.0

[PATCH 06/13] 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.

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

diff --git a/multipathd/main.c b/multipathd/main.c
index 5f16094..354fadf 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2978,6 +2978,20 @@ update_paths(struct vectors *vecs, int *num_paths_p, time_t start_secs)
 	return CHECKER_FINISHED;
 }
 
+static void checker_finished(struct vectors *vecs)
+{
+	struct multipath *mpp;
+	int i;
+
+	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)
+			/* multipath device deleted */
+			i--;
+	}
+}
+
 static void *
 checkerloop (void *ap)
 {
@@ -3050,15 +3064,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) {
-					if ((update_mpp_prio(mpp) ||
-					     (mpp->need_reload && mpp->synced_count > 0)) &&
-					    reload_and_sync_map(mpp, vecs) == 2)
-						/* multipath device deleted */
-						i--;
-				}
-			}
+			if (checker_state == CHECKER_FINISHED)
+				checker_finished(vecs);
 			lock_cleanup_pop(vecs->lock);
 		}
 
-- 
2.47.0

[PATCH 07/13] 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>
---
 multipathd/main.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 354fadf..7e844a5 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2990,6 +2990,11 @@ static void checker_finished(struct vectors *vecs)
 			/* multipath device deleted */
 			i--;
 	}
+	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 *
@@ -3069,16 +3074,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.0

[PATCH 08/13] multipathd: remove mpvec_garbage_collector()

[Martin Wilck]

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

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

diff --git a/multipathd/main.c b/multipathd/main.c
index 7e844a5..43f6cc8 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
@@ -3002,7 +2984,6 @@ checkerloop (void *ap)
 {
 	struct vectors *vecs;
 	struct path *pp;
-	int count = 0;
 	struct timespec last_time;
 	struct config *conf;
 	int foreign_tick = 0;
@@ -3074,18 +3055,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.0

Re: [PATCH 08/13] multipathd: remove mpvec_garbage_collector()

[Benjamin Marzinski]

On Sat, Dec 07, 2024 at 12:36:12AM +0100, Martin Wilck wrote:
> This function duplicates functionality that we now have in the
> do_sync_mpp() code path. Remove it.

If we decide not to delete devices on do_sync_mpp() errors in the
checker, we probably want something like this to say around, perhaps
rolled into finish_checker().

But to go along with my feeling that we shouldn't be deleting the device
on errors in the checker, perhaps it should call dm_is_mpath() and only
delete the device on DM_IS_MPATH_NO.

Thoughts?

-Ben
 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipathd/main.c | 31 -------------------------------
>  1 file changed, 31 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 7e844a5..43f6cc8 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
> @@ -3002,7 +2984,6 @@ checkerloop (void *ap)
>  {
>  	struct vectors *vecs;
>  	struct path *pp;
> -	int count = 0;
>  	struct timespec last_time;
>  	struct config *conf;
>  	int foreign_tick = 0;
> @@ -3074,18 +3055,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.0

[PATCH 09/13] 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().

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

diff --git a/multipathd/main.c b/multipathd/main.c
index 43f6cc8..8425354 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2058,32 +2058,20 @@ 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) &&
+	    need_reload)
+		return true;
+	else
+		return false;
 }
 
 static void
@@ -2967,12 +2955,12 @@ static void checker_finished(struct vectors *vecs)
 
 	vector_foreach_slot(vecs->mpvec, mpp, i) {
 		if ((update_mpp_prio(mpp) ||
-		     (mpp->need_reload && mpp->synced_count > 0)) &&
+		     (mpp->need_reload && mpp->synced_count > 0) ||
+		     deferred_failback_tick(mpp)) &&
 		    reload_and_sync_map(mpp, vecs) == 2)
 			/* multipath device deleted */
 			i--;
 	}
-	deferred_failback_tick(vecs);
 	retry_count_tick(vecs->mpvec);
 	missing_uev_wait_tick(vecs);
 	ghost_delay_tick(vecs);
-- 
2.47.0

[PATCH 10/13] multipathd: move retry_count_tick() into existing mpvec loop

[Martin Wilck]

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

diff --git a/multipathd/main.c b/multipathd/main.c
index 8425354..4cf5493 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2075,21 +2075,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);
 	}
 }
 
@@ -2960,8 +2956,9 @@ static void checker_finished(struct vectors *vecs)
 		    reload_and_sync_map(mpp, vecs) == 2)
 			/* multipath device deleted */
 			i--;
+		else
+			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.0

[PATCH 11/13] 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()).

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

diff --git a/multipathd/main.c b/multipathd/main.c
index 4cf5493..4478cc9 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2011,29 +2011,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
@@ -2947,9 +2937,16 @@ update_paths(struct vectors *vecs, int *num_paths_p, time_t start_secs)
 static void checker_finished(struct vectors *vecs)
 {
 	struct multipath *mpp;
+	bool uev_timed_out = false;
 	int i;
 
 	vector_foreach_slot(vecs->mpvec, mpp, i) {
+		if (missing_uev_wait_tick(mpp, &uev_timed_out) &&
+		    update_map(mpp, vecs, 0)) {
+			/* multipath device deleted */
+			i--;
+			continue;
+		}
 		if ((update_mpp_prio(mpp) ||
 		     (mpp->need_reload && mpp->synced_count > 0) ||
 		     deferred_failback_tick(mpp)) &&
@@ -2959,7 +2956,8 @@ static void checker_finished(struct vectors *vecs)
 		else
 			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.0

Re: [PATCH 11/13] multipathd: don't call update_map() from missing_uev_wait_tick()

[Benjamin Marzinski]

On Sat, Dec 07, 2024 at 12:36:15AM +0100, Martin Wilck wrote:
> 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()).
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipathd/main.c | 40 +++++++++++++++++++---------------------
>  1 file changed, 19 insertions(+), 21 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 4cf5493..4478cc9 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -2011,29 +2011,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
> @@ -2947,9 +2937,16 @@ update_paths(struct vectors *vecs, int *num_paths_p, time_t start_secs)
>  static void checker_finished(struct vectors *vecs)
>  {
>  	struct multipath *mpp;
> +	bool uev_timed_out = false;
>  	int i;
>  
>  	vector_foreach_slot(vecs->mpvec, mpp, i) {
> +		if (missing_uev_wait_tick(mpp, &uev_timed_out) &&
> +		    update_map(mpp, vecs, 0)) {
> +			/* multipath device deleted */
> +			i--;
> +			continue;
> +		}

Looking at this made me think we should probably be adding a check
in reload_and_sync_map() for mpp->wait_for_udev. If it's no-zero,
we should set it to 2 (and we might want to make it symoblic too)
and skip the reload. Otherwise we'll be reloading when we shouldn't be.

>  		if ((update_mpp_prio(mpp) ||
>  		     (mpp->need_reload && mpp->synced_count > 0) ||
>  		     deferred_failback_tick(mpp)) &&
> @@ -2959,7 +2956,8 @@ static void checker_finished(struct vectors *vecs)
>  		else
>  			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.0

[PATCH 12/13] 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>
---
 multipathd/main.c | 31 +++++++++++--------------------
 1 file changed, 11 insertions(+), 20 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 4478cc9..e48fa79 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2026,26 +2026,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)
@@ -2941,7 +2932,8 @@ static void checker_finished(struct vectors *vecs)
 	int i;
 
 	vector_foreach_slot(vecs->mpvec, mpp, i) {
-		if (missing_uev_wait_tick(mpp, &uev_timed_out) &&
+		if ((missing_uev_wait_tick(mpp, &uev_timed_out) ||
+		     ghost_delay_tick(mpp)) &&
 		    update_map(mpp, vecs, 0)) {
 			/* multipath device deleted */
 			i--;
@@ -2958,7 +2950,6 @@ static void checker_finished(struct vectors *vecs)
 	}
 	if (uev_timed_out && !need_to_delay_reconfig(vecs))
 		unblock_reconfigure();
-	ghost_delay_tick(vecs);
 	partial_retrigger_tick(vecs->pathvec);
 }
 
-- 
2.47.0

[PATCH 13/13] 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>
---
 multipathd/main.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index e48fa79..8cca0ce 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2932,8 +2932,7 @@ static void checker_finished(struct vectors *vecs)
 	int i;
 
 	vector_foreach_slot(vecs->mpvec, mpp, i) {
-		if ((missing_uev_wait_tick(mpp, &uev_timed_out) ||
-		     ghost_delay_tick(mpp)) &&
+		if (missing_uev_wait_tick(mpp, &uev_timed_out) &&
 		    update_map(mpp, vecs, 0)) {
 			/* multipath device deleted */
 			i--;
@@ -2941,7 +2940,8 @@ static void checker_finished(struct vectors *vecs)
 		}
 		if ((update_mpp_prio(mpp) ||
 		     (mpp->need_reload && mpp->synced_count > 0) ||
-		     deferred_failback_tick(mpp)) &&
+		     deferred_failback_tick(mpp) ||
+		     ghost_delay_tick(mpp)) &&
 		    reload_and_sync_map(mpp, vecs) == 2)
 			/* multipath device deleted */
 			i--;
-- 
2.47.0

Re: [PATCH 00/13] multipathd: More map reload handling, and checkerloop work

[Benjamin Marzinski]

On Sat, Dec 07, 2024 at 12:36:04AM +0100, Martin Wilck wrote:
> This patch set goes on top of Ben's set [1] for github issue 105 [2].
> 
> The first patch implements the remark I had on patch 2 on Ben's set.
> 
> Patch 2-4 add map reload logic for the case when update_pathvec_from_dm()
> detects an inconsistency: in this case, the map is now reloaded before
> the checkers are actually started. If the inconsistency can't be cleared,
> another attempt will be made when the checkers have finished.
> 
> 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.
> The last patch changes the reload action for the ghost delay tick.
> 
> Reviews & comments welcome.

For all the patches I haven't commented on:
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

> 
> Regards
> Martin
> 
> [1] https://lore.kernel.org/dm-devel/20241205035638.1218953-1-bmarzins@redhat.com/
> [2] https://github.com/opensvc/multipath-tools/issues/105
> 
> Martin Wilck (13):
>   multipathd: don't reload map in update_mpp_prio()
>   multipathd: remove dm_get_info() call from refresh_multipath()
>   multipathd: allow map removal in do_sync_mpp()
>   multipathd: reload maps in do_sync_mpp() if necessary
>   multipathd: move yielding for waiters to start of checkerloop
>   multipathd: add checker_finished()
>   multipathd: move "tick" calls into checker_finished()
>   multipathd: remove mpvec_garbage_collector()
>   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/main.c | 295 +++++++++++++++++++---------------------------
>  1 file changed, 120 insertions(+), 175 deletions(-)
> 
> -- 
> 2.47.0