On Tue, 2024-10-08 at 15:33 -0400, Benjamin Marzinski wrote:
> 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 <bmarz...@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).
That holds for this patch. But right in the next patch, the code is
changed such that libcheck_pending doesn't actually wait at all. So
"ct->waited_for" is a really confusing name. It just means that we have
polled for the state once. In theory that can happen very quickly after
starting the checker.
Regards
Martin
