On Fri, 2018-09-21 at 18:05 -0500, Benjamin Marzinski wrote: > There are only three variables whose access needs to be synchronized > between the tur thread and the path checker itself: state, message, > and > active. The rest of the variables are either only written when the > tur > thread isn't running, or they aren't accessed by the tur thread, or > they > are atomics that are themselves used to synchronize things. > > This patch limits the amount of code that is covered by ct->lock to > only what needs to be locked. It also makes ct->lock no longer a > recursive lock. To make this simpler, tur_thread now only sets the > state and message one time, instead of twice, since PATH_UNCHECKED > was never able to be returned anyway. > > One benefit of this is that the tur checker thread gets more time to > call tur_check() and return before libcheck_check() gives up and > return PATH_PENDING. > > Signed-off-by: Benjamin Marzinski <[email protected]>
Reviewed-by: Martin Wilck <[email protected]> > --- > libmultipath/checkers/tur.c | 49 ++++++++++++++++++----------------- > ---------- > 1 file changed, 20 insertions(+), 29 deletions(-) > > diff --git a/libmultipath/checkers/tur.c > b/libmultipath/checkers/tur.c > index abda162..9f6ef51 100644 > --- a/libmultipath/checkers/tur.c > +++ b/libmultipath/checkers/tur.c > @@ -53,7 +53,6 @@ struct tur_checker_context { > int libcheck_init (struct checker * c) > { > struct tur_checker_context *ct; > - pthread_mutexattr_t attr; > struct stat sb; > > ct = malloc(sizeof(struct tur_checker_context)); > @@ -65,10 +64,7 @@ int libcheck_init (struct checker * c) > ct->fd = -1; > uatomic_set(&ct->holders, 1); > pthread_cond_init_mono(&ct->active); > - pthread_mutexattr_init(&attr); > - pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); > - pthread_mutex_init(&ct->lock, &attr); > - pthread_mutexattr_destroy(&attr); > + pthread_mutex_init(&ct->lock, NULL); > if (fstat(c->fd, &sb) == 0) > snprintf(ct->devt, sizeof(ct->devt), "%d:%d", > major(sb.st_rdev), > minor(sb.st_rdev)); > @@ -213,12 +209,6 @@ static void *tur_thread(void *ctx) > > condlog(3, "%s: tur checker starting up", ct->devt); > > - /* TUR checker start up */ > - pthread_mutex_lock(&ct->lock); > - ct->state = PATH_PENDING; > - ct->message[0] = '\0'; > - pthread_mutex_unlock(&ct->lock); > - > state = tur_check(ct->fd, ct->timeout, msg); > pthread_testcancel(); > > @@ -283,13 +273,6 @@ int libcheck_check(struct checker * c) > /* > * Async mode > */ > - r = pthread_mutex_lock(&ct->lock); > - if (r != 0) { > - condlog(2, "%s: tur mutex lock failed with %d", ct- > >devt, r); > - MSG(c, MSG_TUR_FAILED); > - return PATH_WILD; > - } > - > if (ct->thread) { > if (tur_check_async_timeout(c)) { > int running = uatomic_xchg(&ct->running, 0); > @@ -305,23 +288,29 @@ int libcheck_check(struct checker * c) > } else { > /* TUR checker done */ > ct->thread = 0; > + pthread_mutex_lock(&ct->lock); > tur_status = ct->state; > strlcpy(c->message, ct->message, sizeof(c- > >message)); > + pthread_mutex_unlock(&ct->lock); > } > - pthread_mutex_unlock(&ct->lock); > } else { > if (uatomic_read(&ct->holders) > 1) { > /* pthread cancel failed. If it didn't get the > path > state already, timeout */ > - if (ct->state == PATH_PENDING) { > - pthread_mutex_unlock(&ct->lock); > + pthread_mutex_lock(&ct->lock); > + tur_status = ct->state; > + pthread_mutex_unlock(&ct->lock); > + if (tur_status == PATH_PENDING) { > condlog(3, "%s: tur thread not > responding", > ct->devt); > return PATH_TIMEOUT; > } > } > /* Start new TUR checker */ > - ct->state = PATH_UNCHECKED; > + pthread_mutex_lock(&ct->lock); > + tur_status = ct->state = PATH_PENDING; > + ct->message[0] = '\0'; > + pthread_mutex_unlock(&ct->lock); > ct->fd = c->fd; > ct->timeout = c->timeout; > uatomic_add(&ct->holders, 1); > @@ -334,20 +323,22 @@ int libcheck_check(struct checker * c) > uatomic_sub(&ct->holders, 1); > uatomic_set(&ct->running, 0); > ct->thread = 0; > - pthread_mutex_unlock(&ct->lock); > condlog(3, "%s: failed to start tur thread, > using" > " sync mode", ct->devt); > return tur_check(c->fd, c->timeout, c- > >message); > } > tur_timeout(&tsp); > - r = pthread_cond_timedwait(&ct->active, &ct->lock, > &tsp); > - tur_status = ct->state; > - strlcpy(c->message, ct->message, sizeof(c->message)); > + pthread_mutex_lock(&ct->lock); > + if (ct->state == PATH_PENDING) > + r = pthread_cond_timedwait(&ct->active, &ct- > >lock, > + &tsp); > + if (!r) { > + tur_status = ct->state; > + strlcpy(c->message, ct->message, sizeof(c- > >message)); > + } > pthread_mutex_unlock(&ct->lock); > - if (uatomic_read(&ct->running) != 0 && > - (tur_status == PATH_PENDING || tur_status == > PATH_UNCHECKED)) { > + if (tur_status == PATH_PENDING) { > condlog(3, "%s: tur checker still running", ct- > >devt); > - tur_status = PATH_PENDING; > } else { > int running = uatomic_xchg(&ct->running, 0); > if (running) -- Dr. Martin Wilck <[email protected]>, Tel. +49 (0)911 74053 2107 SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) -- dm-devel mailing list [email protected] https://www.redhat.com/mailman/listinfo/dm-devel
