[dm-devel] [PATCH v3 02/19] libmultipath: fix tur checker double locking
Martin Wilck
mwilck at suse.com
Mon Oct 1 20:09:41 UTC 2018
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?
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?
Regards,
Martin
>
> Cc: Bart Van Assche <bart.vanassche at wdc.com>4
> Signed-off-by: Benjamin Marzinski <bmarzins at redhat.com>
> ---
> 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 <mwilck at suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
More information about the dm-devel
mailing list