]>
Commit | Line | Data |
---|---|---|
a65d4bac GKH |
1 | From foo@baz Sat Jul 28 10:25:26 CEST 2018 |
2 | From: David Sterba <dsterba@suse.com> | |
3 | Date: Tue, 24 Apr 2018 14:53:56 +0200 | |
4 | Subject: btrfs: add barriers to btrfs_sync_log before log_commit_wait wakeups | |
5 | ||
6 | From: David Sterba <dsterba@suse.com> | |
7 | ||
8 | [ Upstream commit 3d3a2e610ea5e7c6d4f9481ecce5d8e2d8317843 ] | |
9 | ||
10 | Currently the code assumes that there's an implied barrier by the | |
11 | sequence of code preceding the wakeup, namely the mutex unlock. | |
12 | ||
13 | As Nikolay pointed out: | |
14 | ||
15 | I think this is wrong (not your code) but the original assumption that | |
16 | the RELEASE semantics provided by mutex_unlock is sufficient. | |
17 | According to memory-barriers.txt: | |
18 | ||
19 | Section 'LOCK ACQUISITION FUNCTIONS' states: | |
20 | ||
21 | (2) RELEASE operation implication: | |
22 | ||
23 | Memory operations issued before the RELEASE will be completed before the | |
24 | RELEASE operation has completed. | |
25 | ||
26 | Memory operations issued after the RELEASE *may* be completed before the | |
27 | RELEASE operation has completed. | |
28 | ||
29 | (I've bolded the may portion) | |
30 | ||
31 | The example given there: | |
32 | ||
33 | As an example, consider the following: | |
34 | ||
35 | *A = a; | |
36 | *B = b; | |
37 | ACQUIRE | |
38 | *C = c; | |
39 | *D = d; | |
40 | RELEASE | |
41 | *E = e; | |
42 | *F = f; | |
43 | ||
44 | The following sequence of events is acceptable: | |
45 | ||
46 | ACQUIRE, {*F,*A}, *E, {*C,*D}, *B, RELEASE | |
47 | ||
48 | So if we assume that *C is modifying the flag which the waitqueue is checking, | |
49 | and *E is the actual wakeup, then those accesses can be re-ordered... | |
50 | ||
51 | IMHO this code should be considered broken... | |
52 | Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> | |
53 | --- | |
54 | ||
55 | To be on the safe side, add the barriers. The synchronization logic | |
56 | around log using the mutexes and several other threads does not make it | |
57 | easy to reason for/against the barrier. | |
58 | ||
59 | CC: Nikolay Borisov <nborisov@suse.com> | |
60 | Link: https://lkml.kernel.org/r/6ee068d8-1a69-3728-00d1-d86293d43c9f@suse.com | |
61 | Reviewed-by: Nikolay Borisov <nborisov@suse.com> | |
62 | Signed-off-by: David Sterba <dsterba@suse.com> | |
63 | ||
64 | Signed-off-by: Sasha Levin <alexander.levin@microsoft.com> | |
65 | Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> | |
66 | --- | |
67 | fs/btrfs/tree-log.c | 10 ++++++++-- | |
68 | 1 file changed, 8 insertions(+), 2 deletions(-) | |
69 | ||
70 | --- a/fs/btrfs/tree-log.c | |
71 | +++ b/fs/btrfs/tree-log.c | |
72 | @@ -3041,8 +3041,11 @@ out_wake_log_root: | |
73 | mutex_unlock(&log_root_tree->log_mutex); | |
74 | ||
75 | /* | |
76 | - * The barrier before waitqueue_active is implied by mutex_unlock | |
77 | + * The barrier before waitqueue_active is needed so all the updates | |
78 | + * above are seen by the woken threads. It might not be necessary, but | |
79 | + * proving that seems to be hard. | |
80 | */ | |
81 | + smp_mb(); | |
82 | if (waitqueue_active(&log_root_tree->log_commit_wait[index2])) | |
83 | wake_up(&log_root_tree->log_commit_wait[index2]); | |
84 | out: | |
85 | @@ -3053,8 +3056,11 @@ out: | |
86 | mutex_unlock(&root->log_mutex); | |
87 | ||
88 | /* | |
89 | - * The barrier before waitqueue_active is implied by mutex_unlock | |
90 | + * The barrier before waitqueue_active is needed so all the updates | |
91 | + * above are seen by the woken threads. It might not be necessary, but | |
92 | + * proving that seems to be hard. | |
93 | */ | |
94 | + smp_mb(); | |
95 | if (waitqueue_active(&root->log_commit_wait[index1])) | |
96 | wake_up(&root->log_commit_wait[index1]); | |
97 | return ret; |