btrfs: add barriers to btrfs_sync_log before log_commit_wait wakeups

[ Upstream commit 3d3a2e610ea5e7c6d4f9481ecce5d8e2d8317843 ]

Currently the code assumes that there's an implied barrier by the
sequence of code preceding the wakeup, namely the mutex unlock.

As Nikolay pointed out:

I think this is wrong (not your code) but the original assumption that
the RELEASE semantics provided by mutex_unlock is sufficient.
According to memory-barriers.txt:

Section 'LOCK ACQUISITION FUNCTIONS' states:

 (2) RELEASE operation implication:

     Memory operations issued before the RELEASE will be completed before the
     RELEASE operation has completed.

     Memory operations issued after the RELEASE *may* be completed before the
     RELEASE operation has completed.

(I've bolded the may portion)

The example given there:

As an example, consider the following:

    *A = a;
    *B = b;
    ACQUIRE
    *C = c;
    *D = d;
    RELEASE
    *E = e;
    *F = f;

The following sequence of events is acceptable:

    ACQUIRE, {*F,*A}, *E, {*C,*D}, *B, RELEASE

So if we assume that *C is modifying the flag which the waitqueue is checking,
and *E is the actual wakeup, then those accesses can be re-ordered...

IMHO this code should be considered broken...
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index bf4e22df7c9786615bfd0df861cd7dbb6859e922..e1b4a59485dffc85208cad4c1b45084080e878bd 100644 (file)
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -3041,8 +3041,11 @@ out_wake_log_root:
        mutex_unlock(&log_root_tree->log_mutex);
 
        /*
-        * The barrier before waitqueue_active is implied by mutex_unlock
+        * The barrier before waitqueue_active is needed so all the updates
+        * above are seen by the woken threads. It might not be necessary, but
+        * proving that seems to be hard.
         */
+       smp_mb();
        if (waitqueue_active(&log_root_tree->log_commit_wait[index2]))
                wake_up(&log_root_tree->log_commit_wait[index2]);
 out:
@@ -3053,8 +3056,11 @@ out:
        mutex_unlock(&root->log_mutex);
 
        /*
-        * The barrier before waitqueue_active is implied by mutex_unlock
+        * The barrier before waitqueue_active is needed so all the updates
+        * above are seen by the woken threads. It might not be necessary, but
+        * proving that seems to be hard.
         */
+       smp_mb();
        if (waitqueue_active(&root->log_commit_wait[index1]))
                wake_up(&root->log_commit_wait[index1]);
        return ret;