On Wed, 2024-08-28 at 18:17 -0400, Benjamin Marzinski wrote:
> This patch adds a new optional symbol for the dynamic path checker
> libraries, libcheck_pending. This is currently unused, but can be
> called
> on pending checkers to check if they've completed and return their
> value.
> The "tur" and "directio" checkers are the only ones which can return
> PATH_PENDING. They now implement libcheck_pending() as a wrapper
> around
> the code that libcheck_check uses to wait for pending paths, which
> has
> been broken out into its own function.
>
> Signed-off-by: Benjamin Marzinski <bmarz...@redhat.com>
> ---
> libmultipath/checkers.c | 4 +-
> libmultipath/checkers.h | 1 +
> libmultipath/checkers/directio.c | 86 ++++++++++++++++++++++--------
> --
> libmultipath/checkers/tur.c | 65 ++++++++++++++++--------
> 4 files changed, 109 insertions(+), 47 deletions(-)
>
> diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c
> index c4918d28..298aec78 100644
> --- a/libmultipath/checkers.c
> +++ b/libmultipath/checkers.c
> @@ -26,6 +26,7 @@ struct checker_class {
> void (*free)(struct checker *); /* to free the context
> */
> void (*reset)(void); /* to reset the global
> variables */
> void *(*thread)(void *); /* async thread entry
> point */
> + int (*pending)(struct checker *); /* to recheck pending
> paths */
> const char **msgtable;
> short msgtable_size;
> };
> @@ -180,7 +181,8 @@ static struct checker_class
> *add_checker_class(const char *name)
> c->mp_init = (int (*)(struct checker *)) dlsym(c->handle,
> "libcheck_mp_init");
> c->reset = (void (*)(void)) dlsym(c->handle,
> "libcheck_reset");
> c->thread = (void *(*)(void*)) dlsym(c->handle,
> "libcheck_thread");
> - /* These 3 functions can be NULL. call dlerror() to clear
> out any
> + c->pending = (int (*)(struct checker *)) dlsym(c->handle,
> "libcheck_pending");
> + /* These 4 functions can be NULL. call dlerror() to clear
> out any
> * error string */
> dlerror();
>
> diff --git a/libmultipath/checkers.h b/libmultipath/checkers.h
> index fb1160af..b2342a1b 100644
> --- a/libmultipath/checkers.h
> +++ b/libmultipath/checkers.h
> @@ -190,6 +190,7 @@ void libcheck_free(struct checker *);
> void *libcheck_thread(struct checker_context *ctx);
> void libcheck_reset(void);
> int libcheck_mp_init(struct checker *);
> +int libcheck_pending(struct checker *c);
>
>
> /*
> diff --git a/libmultipath/checkers/directio.c
> b/libmultipath/checkers/directio.c
> index 8e87878b..3f88b40d 100644
> --- a/libmultipath/checkers/directio.c
> +++ b/libmultipath/checkers/directio.c
> @@ -288,14 +288,36 @@ get_events(struct aio_group *aio_grp, struct
> timespec *timeout)
> return got_events;
> }
>
> +static void
> +check_pending(struct directio_context *ct, struct timespec endtime)
> +{
> + int r;
> + struct timespec currtime, timeout;
> +
> + while(1) {
> + get_monotonic_time(&currtime);
> + timespecsub(&endtime, &currtime, &timeout);
> + if (timeout.tv_sec < 0)
> + timeout.tv_sec = timeout.tv_nsec = 0;
> +
> + r = get_events(ct->aio_grp, &timeout);
> +
> + if (ct->req->state != PATH_PENDING) {
> + ct->running = 0;
> + return;
> + } else if (r == 0 ||
> + (timeout.tv_sec == 0 && timeout.tv_nsec
> == 0))
> + return;
> + }
> +}
> +
> static int
> check_state(int fd, struct directio_context *ct, int sync, int
> timeout_secs)
> {
> struct timespec timeout = { .tv_nsec = 1000 };
> struct stat sb;
> int rc;
> - long r;
> - struct timespec currtime, endtime;
> + struct timespec endtime;
>
> if (fstat(fd, &sb) == 0) {
> LOG(4, "called for %x", (unsigned) sb.st_rdev);
> @@ -330,21 +352,11 @@ check_state(int fd, struct directio_context
> *ct, int sync, int timeout_secs)
> endtime.tv_sec += timeout.tv_sec;
> endtime.tv_nsec += timeout.tv_nsec;
> normalize_timespec(&endtime);
> - while(1) {
> - r = get_events(ct->aio_grp, &timeout);
>
> - if (ct->req->state != PATH_PENDING) {
> - ct->running = 0;
> - return ct->req->state;
> - } else if (r == 0 ||
> - (timeout.tv_sec == 0 && timeout.tv_nsec
> == 0))
> - break;
> + check_pending(ct, endtime);
> + if (ct->req->state != PATH_PENDING)
> + return ct->req->state;
>
> - get_monotonic_time(&currtime);
> - timespecsub(&endtime, &currtime, &timeout);
> - if (timeout.tv_sec < 0)
> - timeout.tv_sec = timeout.tv_nsec = 0;
> - }
> if (ct->running > timeout_secs || sync) {
> struct io_event event;
>
> @@ -360,17 +372,9 @@ check_state(int fd, struct directio_context *ct,
> int sync, int timeout_secs)
> return rc;
> }
>
> -int libcheck_check (struct checker * c)
> +static void set_msgid(struct checker *c, int state)
> {
> - int ret;
> - struct directio_context * ct = (struct directio_context *)c-
> >context;
> -
> - if (!ct)
> - return PATH_UNCHECKED;
> -
> - ret = check_state(c->fd, ct, checker_is_sync(c), c-
> >timeout);
> -
> - switch (ret)
> + switch (state)
> {
> case PATH_UNCHECKED:
> c->msgid = MSG_DIRECTIO_UNKNOWN;
> @@ -387,5 +391,37 @@ int libcheck_check (struct checker * c)
> default:
> break;
> }
> +}
> +
> +int libcheck_pending(struct checker *c)
> +{
> + struct timespec endtime;
> + struct directio_context *ct = (struct directio_context *)c-
> >context;
> +
> + /* The if path checker isn't running, just return the
> exiting value. */
> + if (!ct || !ct->running)
> + return c->path_state;
> +
> + if (ct->req->state == PATH_PENDING) {
> + get_monotonic_time(&endtime);
> + check_pending(ct, endtime);
> + } else
> + ct->running = 0;
> + set_msgid(c, ct->req->state);
> +
> + return ct->req->state;
> +}
> +
> +int libcheck_check (struct checker * c)
> +{
> + int ret;
> + struct directio_context * ct = (struct directio_context *)c-
> >context;
> +
> + if (!ct)
> + return PATH_UNCHECKED;
> +
> + ret = check_state(c->fd, ct, checker_is_sync(c), c-
> >timeout);
> + set_msgid(c, ret);
> +
> return ret;
> }
> 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?
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;
