Re: [PATCH v2 19/22] libmultipath: add libcheck_need_wait checker function

[Benjamin Marzinski <bmarzins@redhat.com>]

On Thu, Oct 03, 2024 at 11:15:21PM +0200, Martin Wilck wrote:
> On Thu, 2024-09-12 at 17:49 -0400, Benjamin Marzinski wrote:
> > Add a new optional checker class function, libcheck_need_wait() and a
> > new function to call it, checker_need_wait(). This can be used to see
> > if
> > a path_checker is currently running. This will be used to determine
> > if
> > there are pending checkers that need to be waited for.
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  libmultipath/checkers.c          | 12 +++++++++++-
> >  libmultipath/checkers.h          |  4 +++-
> >  libmultipath/checkers/directio.c | 10 ++++++++++
> >  libmultipath/checkers/tur.c      | 10 ++++++++++
> >  4 files changed, 34 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c
> > index f3e98352..e2eda58d 100644
> > --- a/libmultipath/checkers.c
> > +++ b/libmultipath/checkers.c
> > @@ -27,6 +27,7 @@ struct checker_class {
> >  	void (*reset)(void);		     /* to reset the global
> > variables */
> >  	void *(*thread)(void *);	     /* async thread entry
> > point */
> >  	int (*pending)(struct checker *);    /* to recheck pending
> > paths */
> > +	bool (*need_wait)(struct checker *); /* checker needs
> > waiting for */
> >  	const char **msgtable;
> >  	short msgtable_size;
> >  };
> > @@ -182,7 +183,8 @@ static struct checker_class
> > *add_checker_class(const char *name)
> >  	c->reset = (void (*)(void)) dlsym(c->handle,
> > "libcheck_reset");
> >  	c->thread = (void *(*)(void*)) dlsym(c->handle,
> > "libcheck_thread");
> >  	c->pending = (int (*)(struct checker *)) dlsym(c->handle,
> > "libcheck_pending");
> > -	/* These 4 functions can be NULL. call dlerror() to clear
> > out any
> > +	c->need_wait = (bool (*)(struct checker *)) dlsym(c->handle,
> > "libcheck_need_wait");
> > +	/* These 5 functions can be NULL. call dlerror() to clear
> > out any
> >  	 * error string */
> >  	dlerror();
> >  
> > @@ -313,6 +315,14 @@ int checker_get_state(struct checker *c)
> >  	return c->path_state;
> >  }
> >  
> > +bool checker_need_wait(struct checker *c)
> > +{
> > +	if (!c || !c->cls || c->path_state != PATH_PENDING ||
> > +	    !c->cls->need_wait)
> > +		return false;
> > +	return c->cls->need_wait(c);
> > +}
> > +
> >  void checker_check (struct checker * c, int path_state)
> >  {
> >  	if (!c)
> > diff --git a/libmultipath/checkers.h b/libmultipath/checkers.h
> > index b2342a1b..da91f499 100644
> > --- a/libmultipath/checkers.h
> > +++ b/libmultipath/checkers.h
> > @@ -2,6 +2,7 @@
> >  #define CHECKERS_H_INCLUDED
> >  
> >  #include <pthread.h>
> > +#include <stdbool.h>
> >  #include "list.h"
> >  #include "defaults.h"
> >  
> > @@ -171,6 +172,7 @@ struct checker_context {
> >  int start_checker_thread (pthread_t *thread, const pthread_attr_t
> > *attr,
> >  			  struct checker_context *ctx);
> >  int checker_get_state(struct checker *c);
> > +bool checker_need_wait(struct checker *c);
> >  void checker_check (struct checker *, int);
> >  int checker_is_sync(const struct checker *);
> >  const char *checker_name (const struct checker *);
> > @@ -191,7 +193,7 @@ void *libcheck_thread(struct checker_context
> > *ctx);
> >  void libcheck_reset(void);
> >  int libcheck_mp_init(struct checker *);
> >  int libcheck_pending(struct checker *c);
> > -
> > +bool libcheck_need_wait(struct checker *c);
> >  
> >  /*
> >   * msgid => message map.
> > diff --git a/libmultipath/checkers/directio.c
> > b/libmultipath/checkers/directio.c
> > index 904e3071..4beed02e 100644
> > --- a/libmultipath/checkers/directio.c
> > +++ b/libmultipath/checkers/directio.c
> > @@ -65,6 +65,7 @@ struct directio_context {
> >  	struct aio_group *aio_grp;
> >  	struct async_req *req;
> >  	struct timespec endtime;
> > +	bool waited_for;
> >  };
> >  
> >  static struct aio_group *
> > @@ -295,6 +296,7 @@ check_pending(struct directio_context *ct, struct
> > timespec endtime)
> >  	int r;
> >  	struct timespec currtime, timeout;
> >  
> > +	ct->waited_for = true;
> 
> Not sure if it's important, but strictly speaking, we have only waited
> for the checker if the endtime was in the future, which is often not
> the case. Otherwise we'd just have polled.

But we always set the endtime in the future when we're starting a new
request, so the first time the we call check_pending() we don't return
till we either get an answer or endtime has passed, which is what we
want waited_for to check (that we've give the checker till endtime to
respond).

-Ben

> 
> Martin
> 
> 
> >  	while(1) {
> >  		get_monotonic_time(&currtime);
> >  		timespecsub(&endtime, &currtime, &timeout);
> > @@ -346,6 +348,7 @@ check_state(int fd, struct directio_context *ct,
> > int sync, int timeout_secs)
> >  		get_monotonic_time(&ct->endtime);
> >  		ct->endtime.tv_nsec += 1000 * 1000;
> >  		normalize_timespec(&ct->endtime);
> > +		ct->waited_for = false;
> >  	}
> >  	ct->running++;
> >  	if (!sync)
> > @@ -386,6 +389,13 @@ static void set_msgid(struct checker *c, int
> > state)
> >  	}
> >  }
> >  
> > +bool libcheck_need_wait(struct checker *c)
> > +{
> > +	struct directio_context *ct = (struct directio_context *)c-
> > >context;
> > +	return (ct && ct->running && ct->req->state == PATH_PENDING
> > &&
> > +		!ct->waited_for);
> > +}
> > +
> >  int libcheck_pending(struct checker *c)
> >  {
> >  	int rc;
> > diff --git a/libmultipath/checkers/tur.c
> > b/libmultipath/checkers/tur.c
> > index 81db565b..41d6b9c3 100644
> > --- a/libmultipath/checkers/tur.c
> > +++ b/libmultipath/checkers/tur.c
> > @@ -59,6 +59,7 @@ struct tur_checker_context {
> >  	struct checker_context ctx;
> >  	unsigned int nr_timeouts;
> >  	struct timespec endtime;
> > +	bool waited_for;
> >  };
> >  
> >  int libcheck_init (struct checker * c)
> > @@ -351,9 +352,17 @@ int check_pending(struct checker *c)
> >  		ct->thread = 0;
> >  	}
> >  
> > +	ct->waited_for = true;
> >  	return tur_status;
> >  }
> >  
> > +bool libcheck_need_wait(struct checker *c)
> > +{
> > +	struct tur_checker_context *ct = c->context;
> > +	return (ct && ct->thread && uatomic_read(&ct->running) != 0
> > &&
> > +		!ct->waited_for);
> > +}
> > +
> >  int libcheck_pending(struct checker *c)
> >  {
> >  	struct tur_checker_context *ct = c->context;
> > @@ -463,6 +472,7 @@ int libcheck_check(struct checker * c)
> >  		pthread_mutex_unlock(&ct->lock);
> >  		ct->fd = c->fd;
> >  		ct->timeout = c->timeout;
> > +		ct->waited_for = false;
> >  		uatomic_add(&ct->holders, 1);
> >  		uatomic_set(&ct->running, 1);
> >  		tur_set_async_timeout(c);