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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 <bmarzins@xxxxxxxxxx>
> ---
>  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;






[Index of Archives]     [DM Crypt]     [Fedora Desktop]     [ATA RAID]     [Fedora Marketing]     [Fedora Packaging]     [Fedora SELinux]     [Yosemite Discussion]     [KDE Users]     [Fedora Docs]

  Powered by Linux