On Mon, Oct 01, 2018 at 10:09:41PM +0200, Martin Wilck wrote: > On Fri, 2018-09-21 at 18:05 -0500, Benjamin Marzinski wrote: > > tur_devt() locks ct->lock. However, it is ocassionally called while > > ct->lock is already locked. In reality, there is no reason why we > > need > > to lock all the accesses to ct->devt. The tur checker only needs to > > write to this variable one time, when it first gets the file > > descripter > > that it is checking. It also never uses ct->devt directly. Instead, > > it > > always graps the major and minor, and turns them into a string. This > > patch changes ct->devt into that string, and sets it in > > libcheck_init() > > when it is first initializing the checker context. After that, ct- > > >devt > > is only ever read. > > I like the lock removal a lot, but not so much the conversion into a > string. Why not keep the dev_t?
Because we will simply convert it into a string every time we use it, instead of doing the work one time. It's 24 more bytes in the tur_checker_context, but the code is easier to read, and we're not doing the same work again and again. > > Or maybe even easier, the other way around: why don't we make it a > char* and simply set checker->dev_t = &pp->dev_t? The whole reason we have tur_checker_context->holders is that it's possible for a path to be removed (or orphaned) while the thread is still running. The tur_checker_context needs to keep all its own storage, so that it never as to worry about pointing to freed memory. -Ben > Regards, > Martin > > > > > Cc: Bart Van Assche <[email protected]>4 > > Signed-off-by: Benjamin Marzinski <[email protected]> > > --- > > libmultipath/checkers/tur.c | 55 +++++++++++++-------------------- > > ------------ > > 1 file changed, 16 insertions(+), 39 deletions(-) > > > > diff --git a/libmultipath/checkers/tur.c > > b/libmultipath/checkers/tur.c > > index 275541f..d173648 100644 > > --- a/libmultipath/checkers/tur.c > > +++ b/libmultipath/checkers/tur.c > > @@ -37,36 +37,24 @@ > > #define MSG_TUR_FAILED "tur checker failed to initialize" > > > > struct tur_checker_context { > > - dev_t devt; > > + char devt[32]; > > int state; > > - int running; > > + int running; /* uatomic access only */ > > int fd; > > unsigned int timeout; > > time_t time; > > pthread_t thread; > > pthread_mutex_t lock; > > pthread_cond_t active; > > - int holders; > > + int holders; /* uatomic access only */ > > char message[CHECKER_MSG_LEN]; > > }; > > > > -static const char *tur_devt(char *devt_buf, int size, > > - struct tur_checker_context *ct) > > -{ > > - dev_t devt; > > - > > - pthread_mutex_lock(&ct->lock); > > - devt = ct->devt; > > - pthread_mutex_unlock(&ct->lock); > > - > > - snprintf(devt_buf, size, "%d:%d", major(devt), minor(devt)); > > - return devt_buf; > > -} > > - > > 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)); > > if (!ct) > > @@ -81,6 +69,9 @@ int libcheck_init (struct checker * c) > > pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); > > pthread_mutex_init(&ct->lock, &attr); > > pthread_mutexattr_destroy(&attr); > > + if (fstat(c->fd, &sb) == 0) > > + snprintf(ct->devt, sizeof(ct->devt), "%d:%d", > > major(sb.st_rdev), > > + minor(sb.st_rdev)); > > c->context = ct; > > > > return 0; > > @@ -232,14 +223,12 @@ static void *tur_thread(void *ctx) > > { > > struct tur_checker_context *ct = ctx; > > int state, running; > > - char devt[32]; > > > > /* This thread can be canceled, so setup clean up */ > > tur_thread_cleanup_push(ct); > > rcu_register_thread(); > > > > - condlog(3, "%s: tur checker starting up", > > - tur_devt(devt, sizeof(devt), ct)); > > + condlog(3, "%s: tur checker starting up", ct->devt); > > > > /* TUR checker start up */ > > pthread_mutex_lock(&ct->lock); > > @@ -256,8 +245,8 @@ static void *tur_thread(void *ctx) > > pthread_cond_signal(&ct->active); > > pthread_mutex_unlock(&ct->lock); > > > > - condlog(3, "%s: tur checker finished, state %s", > > - tur_devt(devt, sizeof(devt), ct), > > checker_state_name(state)); > > + condlog(3, "%s: tur checker finished, state %s", ct->devt, > > + checker_state_name(state)); > > > > running = uatomic_xchg(&ct->running, 0); > > if (!running) > > @@ -305,20 +294,12 @@ int libcheck_check(struct checker * c) > > { > > struct tur_checker_context *ct = c->context; > > struct timespec tsp; > > - struct stat sb; > > pthread_attr_t attr; > > int tur_status, r; > > - char devt[32]; > > > > if (!ct) > > return PATH_UNCHECKED; > > > > - if (fstat(c->fd, &sb) == 0) { > > - pthread_mutex_lock(&ct->lock); > > - ct->devt = sb.st_rdev; > > - pthread_mutex_unlock(&ct->lock); > > - } > > - > > if (c->sync) > > return tur_check(c->fd, c->timeout, > > copy_msg_to_checker, c); > > > > @@ -327,8 +308,7 @@ int libcheck_check(struct checker * c) > > */ > > r = pthread_mutex_lock(&ct->lock); > > if (r != 0) { > > - condlog(2, "%s: tur mutex lock failed with %d", > > - tur_devt(devt, sizeof(devt), ct), r); > > + condlog(2, "%s: tur mutex lock failed with %d", ct- > > >devt, r); > > MSG(c, MSG_TUR_FAILED); > > return PATH_WILD; > > } > > @@ -338,14 +318,12 @@ int libcheck_check(struct checker * c) > > int running = uatomic_xchg(&ct->running, 0); > > if (running) > > pthread_cancel(ct->thread); > > - condlog(3, "%s: tur checker timeout", > > - tur_devt(devt, sizeof(devt), ct)); > > + condlog(3, "%s: tur checker timeout", ct- > > >devt); > > ct->thread = 0; > > MSG(c, MSG_TUR_TIMEOUT); > > tur_status = PATH_TIMEOUT; > > } else if (uatomic_read(&ct->running) != 0) { > > - condlog(3, "%s: tur checker not finished", > > - tur_devt(devt, sizeof(devt), > > ct)); > > + condlog(3, "%s: tur checker not finished", ct- > > >devt); > > tur_status = PATH_PENDING; > > } else { > > /* TUR checker done */ > > @@ -361,7 +339,7 @@ int libcheck_check(struct checker * c) > > if (ct->state == PATH_PENDING) { > > pthread_mutex_unlock(&ct->lock); > > condlog(3, "%s: tur thread not > > responding", > > - tur_devt(devt, sizeof(devt), > > ct)); > > + ct->devt); > > return PATH_TIMEOUT; > > } > > } > > @@ -381,7 +359,7 @@ int libcheck_check(struct checker * c) > > ct->thread = 0; > > pthread_mutex_unlock(&ct->lock); > > condlog(3, "%s: failed to start tur thread, > > using" > > - " sync mode", tur_devt(devt, > > sizeof(devt), ct)); > > + " sync mode", ct->devt); > > return tur_check(c->fd, c->timeout, > > copy_msg_to_checker, c); > > } > > @@ -392,8 +370,7 @@ int libcheck_check(struct checker * c) > > pthread_mutex_unlock(&ct->lock); > > if (uatomic_read(&ct->running) != 0 && > > (tur_status == PATH_PENDING || tur_status == > > PATH_UNCHECKED)) { > > - condlog(3, "%s: tur checker still running", > > - tur_devt(devt, sizeof(devt), ct)); > > + condlog(3, "%s: tur checker still running", ct- > > >devt); > > tur_status = PATH_PENDING; > > } else { > > int running = uatomic_xchg(&ct->running, 0); > > -- > 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
