[dm-devel] [PATCH 19/32] libmultipath: cleanup tur locking

[Benjamin Marzinski]

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.

Cc: Bart Van Assche <bart.vanassche at wdc.com>
Signed-off-by: Benjamin Marzinski <bmarzins at redhat.com>
---
 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 a02c445..4a3e39c 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;
 
 	ct = malloc(sizeof(struct tur_checker_context));
 	if (!ct)
@@ -64,10 +63,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);
 	c->context = ct;
 
 	return 0;
@@ -209,12 +205,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();
 
@@ -286,13 +276,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);
@@ -308,23 +291,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);
@@ -337,20 +326,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)
-- 
2.7.4