From 1787debc1b6640e15a02ccac4699b92affb2bb14 Mon Sep 17 00:00:00 2001 From: Krishnan Parthasarathi Date: Mon, 15 Apr 2013 15:55:28 +0530 Subject: syncenv: be robust against spurious wake()s In the current implementation, when the callers of synctasks perform a spurious wake() of a sleeping synctask (i.e, an extra wake() soon after a wake() which already woke up a yielded synctask), there is now a possibility of two sync threacs picking up the same synctask. This can result in a crash. The fix is to change ->slept = 0|1 and membership of synctask in runqueue atomically. Today we dequeue a task from the runqueue in syncenv_task(), but reset ->slept = 0 much later in synctask_switchto() in an unlocked manner -- which is safe, when there are no spurious wake()s. However, this opens a race window where, if a second wake() happens after the dequeue, but before setting ->slept = 0, it results in queueing the same synctask in the runqueue once again, and get picked up by a different synctask. This is has been diagnosed to be the crashes in the regression tests of http://review.gluster.org/4784. However that patch still has a spurious wake() [the trigger for this bug] which is yet to be fixed. BUG: 948686 Change-Id: I51858e887cad2680e46fb973629f8465f4429363 Original-author: Anand Avati Signed-off-by: Krishnan Parthasarathi Reviewed-on: http://review.gluster.org/4833 Reviewed-by: Vijay Bellur Tested-by: Gluster Build System --- libglusterfs/src/syncop.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) (limited to 'libglusterfs') diff --git a/libglusterfs/src/syncop.c b/libglusterfs/src/syncop.c index d610a2fca03..e2a049271d5 100644 --- a/libglusterfs/src/syncop.c +++ b/libglusterfs/src/syncop.c @@ -143,10 +143,10 @@ synctask_wake (struct synctask *task) if (task->slept && task->woken >= task->waitfor) __run (task); + + pthread_cond_broadcast (&env->cond); } pthread_mutex_unlock (&env->mutex); - - pthread_cond_broadcast (&env->cond); } void @@ -320,12 +320,12 @@ err: struct synctask * syncenv_task (struct syncproc *proc) { - struct syncenv *env = NULL; + struct syncenv *env = NULL; struct synctask *task = NULL; struct timespec sleep_till = {0, }; int ret = 0; - env = proc->env; + env = proc->env; pthread_mutex_lock (&env->mutex); { @@ -347,9 +347,13 @@ syncenv_task (struct syncproc *proc) task = list_entry (env->runq.next, struct synctask, all_tasks); list_del_init (&task->all_tasks); - env->runcount--; + env->runcount--; + + task->woken = 0; + task->slept = 0; + task->waitfor = 0; - task->proc = proc; + task->proc = proc; } unlock: pthread_mutex_unlock (&env->mutex); @@ -368,10 +372,6 @@ synctask_switchto (struct synctask *task) synctask_set (task); THIS = task->xl; - task->woken = 0; - task->slept = 0; - task->waitfor = 0; - #if defined(__NetBSD__) && defined(_UC_TLSBASE) /* Preserve pthread private pointer through swapcontex() */ task->ctx.uc_flags &= ~_UC_TLSBASE; -- cgit