From: Fan Yong <fan.yong@intel.com>
Date: Fri, 4 Nov 2016 01:04:39 +0000 (+0800)
Subject: LU-8760 lib: avoid unexpected out of order execution
X-Git-Tag: 2.10.51~36
X-Git-Url: https://git.whamcloud.com/?p=fs%2Flustre-release.git;a=commitdiff_plain;h=c2b6030e9217e54e7153c0a33cce0c2ea4afa54c;ds=sidebyside

LU-8760 lib: avoid unexpected out of order execution

There is race condtion in __l_wait_event() because of the
out-of-order execution between changing thread state and
checking condition. It may block the thread (to be waken)
for ever. Consider the following real execution order:

1. Thread1 checks condition on CPU1, gets false.
2. Thread2 sets condition on CPU2.
3. Thread2 calls wake_up() on CPU2 to wake the threads with
   state TASK_INTERRUPTIBLE | TASK_UNINTERRUPTIBLE. But the
   Thread1'sstate is TASK_RUNNING at that time.
4. Thread1 sets its state as TASK_INTERRUPTIBLE on CPU1,
   then schedule.

If the '__timeout' variable is zero, the Thread1 will have
no chance to check the condition again.

Generally, the interval between out-of-ordered step1 and step4
is very tiny, as to above step2 and step3 cannot happen. On some
degree, it can explain why we seldom hit related trouble. But
such race really exists, especially consider that the step1 and
step4 can be interruptible.

The patch adds barrier between changing thread's state and
checking condition to avoid out-of-order execution.

Signed-off-by: Fan Yong <fan.yong@intel.com>
Change-Id: I32caee6b332f037d864419ea8728112da563cce0
Reviewed-on: https://review.whamcloud.com/23564
Tested-by: Jenkins
Tested-by: Maloo <hpdd-maloo@intel.com>
Reviewed-by: Yang Sheng <yang.sheng@intel.com>
Reviewed-by: Dmitry Eremin <dmitry.eremin@intel.com>
Reviewed-by: Oleg Drokin <oleg.drokin@intel.com>
---

diff --git a/lustre/include/lustre_lib.h b/lustre/include/lustre_lib.h
index 5d1d4a1..325fc9b 100644
--- a/lustre/include/lustre_lib.h
+++ b/lustre/include/lustre_lib.h
@@ -274,6 +274,30 @@ do {                                                                           \
 	for (;;) {                                                             \
 		set_current_state(TASK_INTERRUPTIBLE);			       \
 									       \
+		/* To guarantee that the condition check will be done */       \
+		/* after setting the thread state as TASK_INTERRUPTIBLE. */    \
+		/* Otherwise, out-of-order execution may cause some race. */   \
+		/* Consider the following real execution order: */	       \
+									       \
+		/* 1. Thread1 checks condition on CPU1, gets false. */	       \
+		/* 2. Thread2 sets condition on CPU2. */		       \
+		/* 3. Thread2 calls wake_up() on CPU2 to wake the threads */   \
+		/*    with state TASK_INTERRUPTIBLE | TASK_UNINTERRUPTIBLE. */ \
+		/*    But the Thread1's state is TASK_RUNNING at that time. */ \
+		/* 4. Thread1 sets its state as TASK_INTERRUPTIBLE on CPU1, */ \
+		/*    then schedule. */					       \
+									       \
+		/* If the '__timeout' variable is zero, the Thread1 will */    \
+		/* have no chance to check the condition again. */	       \
+									       \
+		/* Generally, the interval between out-of-ordered step1 and */ \
+		/* step4 is very tiny, as to above step2 and step3 cannot */   \
+		/* happen. On some degree, it can explain why we seldom hit */ \
+		/* related trouble. But such race really exists, especially */ \
+		/* consider that the step1 and step4 can be interruptible. */  \
+		/* So add barrier to avoid Thread1 out-of-order execution. */  \
+		smp_mb();						       \
+									       \
 		if (condition)                                                 \
 			break;                                                 \
 									       \