Fix race on thread shutdown causing deadlock

PATCH In review Andy Grover 2 participants 4 messages Apr 29, 2014 – Apr 30, 2014 Kbuild

Share & tools

Andy Grover

author

2014-04-29 01:51 UTC #0 lore raw

PATCH 1/3 Fix race on thread shutdown causing deadlock

This patch and the next are somewhat a revert of 318e9f2, but the previous fix didn't quite close the race. This only happens when we create threads for a backstore that turns out to be invalid, which we then tear down.

See https://bugzilla.redhat.com/show_bug.cgi?id=848585 .

This is occurring because there's still a window where a thread misses seeing info->stop == 1 but is not yet in cond_wait so it misses the broadcast:

thread_close: thread_worker_fn:

                           info->stop is seen as 0

info->stop = 1
pthread_cond_broadcast -- misses broadcast
pthread_cond_wait
pthread_join (hangs)

I believe the solution is to go back to using pthread_cancel. We can call it before pthread_cond_wait is called (or after) and it will do the right
thing: pop out and exit. The only tricky bit is we need to use the pthread_cleanup_push mechanism to properly release info->pending_lock.

Signed-off-by Andy Grover <[email protected]>

---
 usr/bs.c        | 25 ++++++++++++++-----------
 usr/bs_thread.h |  2 --
 2 files changed, 14 insertions(+), 13 deletions(-)

usr/bs.c +14 ~~-11~~

index 13d3b4e..d81aaee 100644
@@ -213,6 +213,12 @@ static void bs_sig_request_done(int fd, int events, void *data)
 	}
 }
 
+/* Unlock mutex even if thread is cancelled */
+static void mutex_cleanup(void *mutex)
+{
+	pthread_mutex_unlock(mutex);
+}
+
 static void *bs_thread_worker_fn(void *arg)
 {
 	struct bs_thread_info *info = arg;

@@ -226,15 +232,13 @@ static void *bs_thread_worker_fn(void *arg)
 	dprintf("started this thread\n");
 	pthread_mutex_unlock(&info->startup_lock);
 
-	while (!info->stop) {
+	while (1) {
 		pthread_mutex_lock(&info->pending_lock);
+		pthread_cleanup_push(mutex_cleanup, &info->pending_lock);
+
 	retest:
 		if (list_empty(&info->pending_list)) {
 			pthread_cond_wait(&info->pending_cond, &info->pending_lock);
-			if (info->stop) {
-				pthread_mutex_unlock(&info->pending_lock);
-				pthread_exit(NULL);
-			}
 			goto retest;
 		}

@@ -242,7 +246,7 @@ static void *bs_thread_worker_fn(void *arg)
 				       struct scsi_cmd, bs_list);
 
 		list_del(&cmd->bs_list);
-		pthread_mutex_unlock(&info->pending_lock);
+		pthread_cleanup_pop(1); /* Unlock pending_lock mutex */
 
 		info->request_fn(cmd);

@@ -435,10 +439,10 @@ tgtadm_err bs_thread_open(struct bs_thread_info *info, request_func_t *rfn,
 
 	return TGTADM_SUCCESS;
 destroy_threads:
-	info->stop = 1;
 
 	pthread_mutex_unlock(&info->startup_lock);
 	for (; i > 0; i--) {
+		pthread_cancel(info->worker_thread[i - 1]);
 		pthread_join(info->worker_thread[i - 1], NULL);
 		eprintf("stopped the worker thread %d\n", i - 1);
 	}

@@ -455,18 +459,17 @@ void bs_thread_close(struct bs_thread_info *info)
 {
 	int i;
 
-	info->stop = 1;
 	pthread_cond_broadcast(&info->pending_cond);
 
-	for (i = 0; i < info->nr_worker_threads && info->worker_thread[i]; i++)
+	for (i = 0; i < info->nr_worker_threads && info->worker_thread[i]; i++) {
+		pthread_cancel(info->worker_thread[i]);
 		pthread_join(info->worker_thread[i], NULL);
+	}
 
 	pthread_cond_destroy(&info->pending_cond);
 	pthread_mutex_destroy(&info->pending_lock);
 	pthread_mutex_destroy(&info->startup_lock);
 	free(info->worker_thread);
-
-	info->stop = 0;
 }
 
 int bs_thread_cmd_submit(struct scsi_cmd *cmd)

usr/bs_thread.h +0 -2

index a7e4063..a3ac551 100644
@@ -13,8 +13,6 @@ struct bs_thread_info {
 
 	pthread_mutex_t startup_lock;
 
-	int stop;
-
 	request_func_t *request_fn;
 };

Signature · 1 line

1.9.0

Andy Grover

author

2014-04-29 01:51 UTC #1 lore raw

PATCH 2/3 Remove startup_lock

The purpose of the startup lock was to hold threads at the startup_lock so if a thread creation fails, it's still possible to have them end themselves by looking at info->stop. Since we are back to using pthread_cancel instead of info->stop, we don't need this any more. The only wrinkle is we need to only send pthread_cancels to successfully-created threads, so now check this in the destroy_threads reverse-loop.

Signed-off-by Andy Grover <[email protected]>

---
 usr/bs.c        | 17 +++++------------
 usr/bs_thread.h |  2 --
 2 files changed, 5 insertions(+), 14 deletions(-)

usr/bs.c +5 ~~-12~~

index d81aaee..d6b8015 100644
@@ -228,9 +228,6 @@ static void *bs_thread_worker_fn(void *arg)
 	sigfillset(&set);
 	sigprocmask(SIG_BLOCK, &set, NULL);
 
-	pthread_mutex_lock(&info->startup_lock);
-	dprintf("started this thread\n");
-	pthread_mutex_unlock(&info->startup_lock);
 
 	while (1) {
 		pthread_mutex_lock(&info->pending_lock);

@@ -420,9 +417,7 @@ tgtadm_err bs_thread_open(struct bs_thread_info *info, request_func_t *rfn,
 
 	pthread_cond_init(&info->pending_cond, NULL);
 	pthread_mutex_init(&info->pending_lock, NULL);
-	pthread_mutex_init(&info->startup_lock, NULL);
 
-	pthread_mutex_lock(&info->startup_lock);
 	for (i = 0; i < nr_threads; i++) {
 		ret = pthread_create(&info->worker_thread[i], NULL,
 				     bs_thread_worker_fn, info);

@@ -434,22 +429,21 @@ tgtadm_err bs_thread_open(struct bs_thread_info *info, request_func_t *rfn,
 				goto destroy_threads;
 		}
 	}
-	pthread_mutex_unlock(&info->startup_lock);
 	info->nr_worker_threads = nr_threads;
 
 	return TGTADM_SUCCESS;
 destroy_threads:
 
-	pthread_mutex_unlock(&info->startup_lock);
 	for (; i > 0; i--) {
-		pthread_cancel(info->worker_thread[i - 1]);
-		pthread_join(info->worker_thread[i - 1], NULL);
-		eprintf("stopped the worker thread %d\n", i - 1);
+		if (info->worker_thread[i - 1]) {
+			pthread_cancel(info->worker_thread[i - 1]);
+			pthread_join(info->worker_thread[i - 1], NULL);
+			eprintf("stopped the worker thread %d\n", i - 1);
+		}
 	}
 
 	pthread_cond_destroy(&info->pending_cond);
 	pthread_mutex_destroy(&info->pending_lock);
-	pthread_mutex_destroy(&info->startup_lock);
 	free(info->worker_thread);
 
 	return TGTADM_NOMEM;

@@ -468,7 +462,6 @@ void bs_thread_close(struct bs_thread_info *info)
 
 	pthread_cond_destroy(&info->pending_cond);
 	pthread_mutex_destroy(&info->pending_lock);
-	pthread_mutex_destroy(&info->startup_lock);
 	free(info->worker_thread);
 }

usr/bs_thread.h +0 -2

index a3ac551..260f73f 100644
@@ -11,8 +11,6 @@ struct bs_thread_info {
 	/* protected by pending_lock */
 	struct list_head pending_list;
 
-	pthread_mutex_t startup_lock;
-
 	request_func_t *request_fn;
 };

Signature · 1 line

1.9.0

Andy Grover

author

2014-04-29 01:51 UTC #2 lore raw

PATCH 3/3 Replace if()/goto with while()

In reply to Andy Grover

Is equivalent and nicer-looking.

Signed-off-by Andy Grover <[email protected]>

---
 usr/bs.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

usr/bs.c +5 ~~-10~~

index d6b8015..1ab86f7 100644
@@ -122,11 +122,9 @@ retry:
 	}
 
 	pthread_mutex_lock(&finished_lock);
-retest:
-	if (list_empty(&finished_list)) {
+
+	while (list_empty(&finished_list))
 		pthread_cond_wait(&finished_cond, &finished_lock);
-		goto retest;
-	}
 
 	while (!list_empty(&finished_list)) {
 		cmd = list_first_entry(&finished_list,

@@ -228,16 +226,13 @@ static void *bs_thread_worker_fn(void *arg)
 	sigfillset(&set);
 	sigprocmask(SIG_BLOCK, &set, NULL);
 
-
 	while (1) {
 		pthread_mutex_lock(&info->pending_lock);
 		pthread_cleanup_push(mutex_cleanup, &info->pending_lock);
 
-	retest:
-		if (list_empty(&info->pending_list)) {
-			pthread_cond_wait(&info->pending_cond, &info->pending_lock);
-			goto retest;
-		}
+		while (list_empty(&info->pending_list))
+			pthread_cond_wait(&info->pending_cond,
+					  &info->pending_lock);
 
 		cmd = list_first_entry(&info->pending_list,
 				       struct scsi_cmd, bs_list);

Signature · 1 line

1.9.0

FUJITA Tomonori

2014-04-30 14:29 UTC #3 lore raw

In reply to Andy Grover

Quoted 22 lines · On Mon, 28 Apr 2014 18:51:20 -0700 Andy Grover <[email protected]> wrote:

This patch and the next are somewhat a revert of 318e9f2, but the previous fix didn't quite close the race. This only happens when we create threads for a backstore that turns out to be invalid, which we then tear down.
See https://bugzilla.redhat.com/show_bug.cgi?id=848585 .
This is occurring because there's still a window where a thread misses seeing info->stop == 1 but is not yet in cond_wait so it misses the broadcast:
thread_close: thread_worker_fn:
                           info->stop is seen as 0
info->stop = 1
pthread_cond_broadcast -- misses broadcast
pthread_cond_wait
pthread_join (hangs)
I believe the solution is to go back to using pthread_cancel. We can call it before pthread_cond_wait is called (or after) and it will do the right
thing: pop out and exit. The only tricky bit is we need to use the pthread_cleanup_push mechanism to properly release info->pending_lock.

Quoted 5 lines

Signed-off-by Andy Grover <[email protected]>

---
 usr/bs.c        | 25 ++++++++++++++-----------
 usr/bs_thread.h |  2 --
 2 files changed, 14 insertions(+), 13 deletions(-)

Thanks a lot for the fixes and detailed explanation. Surely, looks like there is a race. The whole patchset looks good. Applied, thanks!