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 <avati@redhat.com>
Signed-off-by: Krishnan Parthasarathi <kparthas@redhat.com>
Reviewed-on: http://review.gluster.org/4833
Reviewed-by: Vijay Bellur <vbellur@redhat.com>
Tested-by: Gluster Build System <jenkins@build.gluster.com>

1 files changed, 10 insertions, 10 deletions

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;