Re: [PATCH 03/15] libmultipath: split out the code to wait for pending checkers

[Benjamin Marzinski <bmarzins@redhat.com>]

On Wed, Sep 04, 2024 at 05:01:39PM +0200, Martin Wilck wrote:
> On Wed, 2024-08-28 at 18:17 -0400, Benjamin Marzinski wrote:

<snip>

> > diff --git a/libmultipath/checkers/tur.c
> > b/libmultipath/checkers/tur.c
> > index a2905af5..95af5214 100644
> > --- a/libmultipath/checkers/tur.c
> > +++ b/libmultipath/checkers/tur.c
> > @@ -323,6 +323,49 @@ static int tur_check_async_timeout(struct
> > checker *c)
> >  	return (now.tv_sec > ct->time);
> >  }
> >  
> > +int check_pending(struct checker *c, struct timespec endtime)
> > +{
> > +	struct tur_checker_context *ct = c->context;
> > +	int r, tur_status = PATH_PENDING;
> > +
> > +	pthread_mutex_lock(&ct->lock);
> > +
> > +	for (r = 0;
> > +	     r == 0 && ct->state == PATH_PENDING &&
> > +	     ct->msgid == MSG_TUR_RUNNING;
> > +	     r = pthread_cond_timedwait(&ct->active, &ct->lock,
> > &endtime));
> > +
> > +	if (!r) {
> > +		tur_status = ct->state;
> > +		c->msgid = ct->msgid;
> > +	}
> > +	pthread_mutex_unlock(&ct->lock);
> > +	if (tur_status == PATH_PENDING && c->msgid ==
> > MSG_TUR_RUNNING) {
> > +		condlog(4, "%d:%d : tur checker still running",
> > +			major(ct->devt), minor(ct->devt));
> > +	} else {
> > +		int running = uatomic_xchg(&ct->running, 0);
> > +		if (running)
> > +			pthread_cancel(ct->thread);
> > +		ct->thread = 0;
> > +	}
> > +
> > +	return tur_status;
> > +}
> > +
> > +int libcheck_pending(struct checker *c)
> > +{
> > +	struct timespec endtime;
> > +	struct tur_checker_context *ct = c->context;
> > +
> > +	/* The if path checker isn't running, just return the
> > exiting value. */
> > +	if (!ct || !ct->thread)
> > +		return c->path_state;
> > +
> > +	get_monotonic_time(&endtime);
> > +	return check_pending(c, endtime);
> 
> Does this work? 
> 
> https://pubs.opengroup.org/onlinepubs/009695299/functions/pthread_cond_timedwait.html
> says that "an error is returned [...] if the absolute time specified by
> abstime has already been passed at the time of the call".
> 
> Which would always be the case if you pass a timestamp that you 
> fetched previously unmodified, meaning that you'd always get ETIMEDOUT.
> Or am I misreading the docs?

No. You are reading the docs right.  I'm just don't understand why
that's a problem.  When you start the checker, you set a timeout that
you want to wait for, to give the checker a chance to complete in this
checker loop. Once that timeout has passed, if the thread is still
running, then libcheck_pending should return PATH_PENDING.
pthread_cond_timedwait() doesn't get called unless the thread is
running. So I would argue that pthread_cond_timedwait() always failing
once that timeout has passed is the correct thing to do.

Or are you arguing for having check_pending() manually check if the
timeout has passed, and skipping the call to pthread_cond_timedwait() in
that case? 

With the directio checker, we need to call get_events() even once our
timeout has passed, since we need to handle any async IOs that have
completed since the last call to check_pending(). But in there's no need
to call pthread_cond_timedwait() for the tur checker, so we could skip
it if we saw that the timeout had already passed. I would argue that
this is just duplicating the check from pthread_cond_timedwait() in
check_pending() for no real benfit though.

Or am I missing something here?

-Ben

> Martin
> 
> 
> 
> 
> 
> 
> 
> > +}
> > +
> >  int libcheck_check(struct checker * c)
> >  {
> >  	struct tur_checker_context *ct = c->context;
> > @@ -437,27 +480,7 @@ int libcheck_check(struct checker * c)
> >  			return tur_check(c->fd, c->timeout, &c-
> > >msgid);
> >  		}
> >  		tur_timeout(&tsp);
> > -		pthread_mutex_lock(&ct->lock);
> > -
> > -		for (r = 0;
> > -		     r == 0 && ct->state == PATH_PENDING &&
> > -			     ct->msgid == MSG_TUR_RUNNING;
> > -		     r = pthread_cond_timedwait(&ct->active, &ct-
> > >lock, &tsp));
> > -
> > -		if (!r) {
> > -			tur_status = ct->state;
> > -			c->msgid = ct->msgid;
> > -	}
> > -		pthread_mutex_unlock(&ct->lock);
> > -		if (tur_status == PATH_PENDING && c->msgid ==
> > MSG_TUR_RUNNING) {
> > -			condlog(4, "%d:%d : tur checker still
> > running",
> > -				major(ct->devt), minor(ct->devt));
> > -		} else {
> > -			int running = uatomic_xchg(&ct->running, 0);
> > -			if (running)
> > -				pthread_cancel(ct->thread);
> > -			ct->thread = 0;
> > -		}
> > +		tur_status = check_pending(c, tsp);
> >  	}
> >  
> >  	return tur_status;