Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

btrfs: add barriers to btrfs_sync_log before log_commit_wait wakeups

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...
---

To be on the safe side, add the barriers. The synchronization logic
around log using the mutexes and several other threads does not make it
easy to reason for/against the barrier.

CC: Nikolay Borisov <nborisov@suse.com>
Link: https://lkml.kernel.org/r/6ee068d8-1a69-3728-00d1-d86293d43c9f@suse.com
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>

+8 -2
+8 -2
fs/btrfs/tree-log.c
··· 3116 3116 mutex_unlock(&log_root_tree->log_mutex); 3117 3117 3118 3118 /* 3119 - * The barrier before waitqueue_active is implied by mutex_unlock 3119 + * The barrier before waitqueue_active is needed so all the updates 3120 + * above are seen by the woken threads. It might not be necessary, but 3121 + * proving that seems to be hard. 3120 3122 */ 3123 + smp_mb(); 3121 3124 if (waitqueue_active(&log_root_tree->log_commit_wait[index2])) 3122 3125 wake_up(&log_root_tree->log_commit_wait[index2]); 3123 3126 out: ··· 3131 3128 mutex_unlock(&root->log_mutex); 3132 3129 3133 3130 /* 3134 - * The barrier before waitqueue_active is implied by mutex_unlock 3131 + * The barrier before waitqueue_active is needed so all the updates 3132 + * above are seen by the woken threads. It might not be necessary, but 3133 + * proving that seems to be hard. 3135 3134 */ 3135 + smp_mb(); 3136 3136 if (waitqueue_active(&root->log_commit_wait[index1])) 3137 3137 wake_up(&root->log_commit_wait[index1]); 3138 3138 return ret;