Re: [PATCH v2 12/22] multipathd: split check_paths into two functions

[Benjamin Marzinski <bmarzins@redhat.com>]

On Thu, Oct 03, 2024 at 10:41:20PM +0200, Martin Wilck wrote:
> On Thu, 2024-09-12 at 17:49 -0400, Benjamin Marzinski wrote:
> > Instead of checking and updating each path, the checkerloop now
> > starts
> > the checkers on all the paths in check_paths(), and then goes back
> > and
> > updates all the paths in update_paths(). Since the async checkers use
> > an
> > absolute time to wait for before returning PATH_PENDING, only one
> > checker actually needs to be waited for in update_paths(). The rest
> > will
> > already have reached their timeout when update_path() is called for
> > them.
> > 
> > The check_paths() and update_paths() loop over the pathvec instead of
> > looping through the multipath device paths to avoid having to restart
> > checking of the device paths when the multipath device needs to be
> > resynced while updating the paths.
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  libmultipath/structs.h |  10 +++-
> >  multipathd/main.c      | 105 +++++++++++++++++++--------------------
> > --
> >  2 files changed, 57 insertions(+), 58 deletions(-)
> > 
> > diff --git a/libmultipath/structs.h b/libmultipath/structs.h
> > index d8231e95..af8e31e9 100644
> > --- a/libmultipath/structs.h
> > +++ b/libmultipath/structs.h
> > @@ -314,6 +314,14 @@ enum recheck_wwid_states {
> >  	RECHECK_WWID_ON = YNU_YES,
> >  };
> >  
> > +enum check_path_states {
> > +	CHECK_PATH_UNCHECKED,
> > +	CHECK_PATH_STARTED,
> > +	CHECK_PATH_CHECKED,
> > +	CHECK_PATH_SKIPPED,
> > +	CHECK_PATH_REMOVED,
> > +};
> > +
> >  struct vpd_vendor_page {
> >  	int pg;
> >  	const char *name;
> > @@ -395,7 +403,7 @@ struct path {
> >  	int fast_io_fail;
> >  	unsigned int dev_loss;
> >  	int eh_deadline;
> > -	bool is_checked;
> > +	enum check_path_states is_checked;
> >  	bool can_use_env_uid;
> >  	unsigned int checker_timeout;
> >  	/* configlet pointers */
> > diff --git a/multipathd/main.c b/multipathd/main.c
> > index 45d40559..9519b6c5 100644
> > --- a/multipathd/main.c
> > +++ b/multipathd/main.c
> > @@ -2389,13 +2389,6 @@ sync_mpp(struct vectors * vecs, struct
> > multipath *mpp, unsigned int ticks)
> >  	do_sync_mpp(vecs, mpp);
> >  }
> >  
> > -enum check_path_return {
> > -	CHECK_PATH_STARTED,
> > -	CHECK_PATH_CHECKED,
> > -	CHECK_PATH_SKIPPED,
> > -	CHECK_PATH_REMOVED,
> > -};
> > -
> >  static int
> >  update_path_state (struct vectors * vecs, struct path * pp)
> >  {
> > @@ -2735,6 +2728,7 @@ check_uninitialized_path(struct path * pp,
> > unsigned int ticks)
> >  	conf = get_multipath_config();
> >  	retrigger_tries = conf->retrigger_tries;
> >  	pp->tick = conf->max_checkint;
> > +	pp->checkint = conf->checkint;
> >  	put_multipath_config(conf);
> >  
> >  	if (pp->initialized == INIT_MISSING_UDEV) {
> > @@ -2778,6 +2772,10 @@ update_uninitialized_path(struct vectors *
> > vecs, struct path * pp)
> >  	int newstate, ret;
> >  	struct config *conf;
> >  
> > +	if (pp->initialized != INIT_NEW && pp->initialized !=
> > INIT_FAILED &&
> > +	    pp->initialized != INIT_MISSING_UDEV)
> > +		return CHECK_PATH_SKIPPED;
> > +
> >  	newstate = get_new_state(pp);
> >  
> 
> It seems to me that this hunk and the previous one belong into 11/22.

Setting pp->checkint could certainly go in the earlier patch. It's just
there to match check_path(), although to be honest, I'm still not sure
that there is any way for either check_path() or
check_uninitialized_path() to get called with pp->checkint unset.
 
The checking the path->initialized state in update_uninitialized_path()
makes less sense. That code is already in check_unintialized_path(). In
11/22, update_uninitialized_path() is always called immediately after
check_unintialized_path() if it's called at all, and pp->initialized
never has a chance to change.

In this patch, check_unintialized_path() is called in check_paths(), and
it's possible that path checking takes too long, and we need take a
break and yield to other vecs lock waiters before we call
update_paths(), where update_uninitialized_path() is called. In this
case, it's possible that a uevent got processed and changed
pp->initialized, so we need to recheck it in
update_uninitialized_path().

-Ben

> Martin