* [PATCH 1/4] btrfs: add missing barriers before waitqueue_active
2015-02-24 16:51 [PULL] [PATCH 0/4] Barriers + waitqueue_active fixes David Sterba
@ 2015-02-24 16:51 ` David Sterba
2015-03-25 18:05 ` Chris Mason
2015-02-24 16:51 ` [PATCH 2/4] btrfs: add comments to " David Sterba
` (2 subsequent siblings)
3 siblings, 1 reply; 6+ messages in thread
From: David Sterba @ 2015-02-24 16:51 UTC (permalink / raw)
To: linux-btrfs; +Cc: David Sterba
The waitqueue might miss a wakeup due to memory ordering issues, the
explicit barrier is required unless there's an implicit one.
Signed-off-by: David Sterba <dsterba@suse.cz>
---
fs/btrfs/dev-replace.c | 9 ++++++++-
fs/btrfs/raid56.c | 17 ++++++++++++-----
fs/btrfs/transaction.c | 5 ++++-
fs/btrfs/tree-log.c | 8 ++++++++
4 files changed, 32 insertions(+), 7 deletions(-)
diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index 5ec03d999c37..30b8668396e0 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -451,6 +451,10 @@ static void btrfs_rm_dev_replace_blocked(struct btrfs_fs_info *fs_info)
static void btrfs_rm_dev_replace_unblocked(struct btrfs_fs_info *fs_info)
{
clear_bit(BTRFS_FS_STATE_DEV_REPLACING, &fs_info->fs_state);
+ /*
+ * Make sure counter is updated before we wake up waiters.
+ */
+ smp_mb();
if (waitqueue_active(&fs_info->replace_wait))
wake_up(&fs_info->replace_wait);
}
@@ -916,7 +920,10 @@ void btrfs_bio_counter_inc_noblocked(struct btrfs_fs_info *fs_info)
void btrfs_bio_counter_sub(struct btrfs_fs_info *fs_info, s64 amount)
{
percpu_counter_sub(&fs_info->bio_counter, amount);
-
+ /*
+ * Make sure counter is updated before we wake up waiters.
+ */
+ smp_mb();
if (waitqueue_active(&fs_info->replace_wait))
wake_up(&fs_info->replace_wait);
}
diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index 5264858ed768..b460e193f324 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -809,11 +809,18 @@ static noinline void unlock_stripe(struct btrfs_raid_bio *rbio)
}
goto done_nolock;
- } else if (waitqueue_active(&h->wait)) {
- spin_unlock(&rbio->bio_list_lock);
- spin_unlock_irqrestore(&h->lock, flags);
- wake_up(&h->wait);
- goto done_nolock;
+ } else {
+ /*
+ * Make sure counter is updated before we wake up
+ * waiters.
+ */
+ smp_mb();
+ if (waitqueue_active(&h->wait)) {
+ spin_unlock(&rbio->bio_list_lock);
+ spin_unlock_irqrestore(&h->lock, flags);
+ wake_up(&h->wait);
+ goto done_nolock;
+ }
}
}
done:
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 038fcf6051e0..90ba0c3c3d0d 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -90,8 +90,11 @@ static void clear_btree_io_tree(struct extent_io_tree *tree)
/*
* btree io trees aren't supposed to have tasks waiting for
* changes in the flags of extent states ever.
+ *
+ * Barrier required to make sure counter is updated before we
+ * wake up waiters.
*/
- ASSERT(!waitqueue_active(&state->wq));
+ ASSERT(({ smp_mb(); !waitqueue_active(&state->wq); }));
free_extent_state(state);
if (need_resched()) {
spin_unlock(&tree->lock);
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index f96996a1b70c..121df0fe5128 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -2739,6 +2739,10 @@ out_wake_log_root:
atomic_set(&log_root_tree->log_commit[index2], 0);
mutex_unlock(&log_root_tree->log_mutex);
+ /*
+ * Make sure counter is updated before we wake up waiters.
+ */
+ smp_mb();
if (waitqueue_active(&log_root_tree->log_commit_wait[index2]))
wake_up(&log_root_tree->log_commit_wait[index2]);
out:
@@ -2750,6 +2754,10 @@ out:
atomic_set(&root->log_commit[index1], 0);
mutex_unlock(&root->log_mutex);
+ /*
+ * Make sure counter is updated before we wake up waiters.
+ */
+ smp_mb();
if (waitqueue_active(&root->log_commit_wait[index1]))
wake_up(&root->log_commit_wait[index1]);
return ret;
--
1.8.4.5
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 1/4] btrfs: add missing barriers before waitqueue_active
2015-02-24 16:51 ` [PATCH 1/4] btrfs: add missing barriers before waitqueue_active David Sterba
@ 2015-03-25 18:05 ` Chris Mason
0 siblings, 0 replies; 6+ messages in thread
From: Chris Mason @ 2015-03-25 18:05 UTC (permalink / raw)
To: David Sterba; +Cc: linux-btrfs
On Tue, Feb 24, 2015 at 11:51 AM, David Sterba <dsterba@suse.cz> wrote:
> The waitqueue might miss a wakeup due to memory ordering issues, the
> explicit barrier is required unless there's an implicit one.
Thanks for going through these Dave, a few comments below:
>
> Signed-off-by: David Sterba <dsterba@suse.cz>
> ---
> fs/btrfs/dev-replace.c | 9 ++++++++-
> fs/btrfs/raid56.c | 17 ++++++++++++-----
> fs/btrfs/transaction.c | 5 ++++-
> fs/btrfs/tree-log.c | 8 ++++++++
> 4 files changed, 32 insertions(+), 7 deletions(-)
>
> diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
> index 5ec03d999c37..30b8668396e0 100644
> --- a/fs/btrfs/dev-replace.c
> +++ b/fs/btrfs/dev-replace.c
> @@ -451,6 +451,10 @@ static void btrfs_rm_dev_replace_blocked(struct
> btrfs_fs_info *fs_info)
> static void btrfs_rm_dev_replace_unblocked(struct btrfs_fs_info
> *fs_info)
> {
> clear_bit(BTRFS_FS_STATE_DEV_REPLACING, &fs_info->fs_state);
> + /*
> + * Make sure counter is updated before we wake up waiters.
> + */
> + smp_mb();
> if (waitqueue_active(&fs_info->replace_wait))
> wake_up(&fs_info->replace_wait);
This one isn't performance critical, lets just use wake_up directly.
>
> }
> @@ -916,7 +920,10 @@ void btrfs_bio_counter_inc_noblocked(struct
> btrfs_fs_info *fs_info)
> void btrfs_bio_counter_sub(struct btrfs_fs_info *fs_info, s64 amount)
> {
> percpu_counter_sub(&fs_info->bio_counter, amount);
> -
> + /*
> + * Make sure counter is updated before we wake up waiters.
> + */
> + smp_mb();
> if (waitqueue_active(&fs_info->replace_wait))
> wake_up(&fs_info->replace_wait);
> }
This is performance critical. Can we do it only when a replace is
actually running?
>
> diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
> index 5264858ed768..b460e193f324 100644
> --- a/fs/btrfs/raid56.c
> +++ b/fs/btrfs/raid56.c
> @@ -809,11 +809,18 @@ static noinline void unlock_stripe(struct
> btrfs_raid_bio *rbio)
> }
>
> goto done_nolock;
> - } else if (waitqueue_active(&h->wait)) {
> - spin_unlock(&rbio->bio_list_lock);
> - spin_unlock_irqrestore(&h->lock, flags);
> - wake_up(&h->wait);
> - goto done_nolock;
> + } else {
> + /*
> + * Make sure counter is updated before we wake up
> + * waiters.
> + */
> + smp_mb();
> + if (waitqueue_active(&h->wait)) {
> + spin_unlock(&rbio->bio_list_lock);
> + spin_unlock_irqrestore(&h->lock, flags);
> + wake_up(&h->wait);
> + goto done_nolock;
> + }
> }
> }
> done:
This one is already protected by the h->lock, we can't miss wakeups.
>
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index 038fcf6051e0..90ba0c3c3d0d 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -90,8 +90,11 @@ static void clear_btree_io_tree(struct
> extent_io_tree *tree)
> /*
> * btree io trees aren't supposed to have tasks waiting for
> * changes in the flags of extent states ever.
> + *
> + * Barrier required to make sure counter is updated before we
> + * wake up waiters.
> */
> - ASSERT(!waitqueue_active(&state->wq));
> + ASSERT(({ smp_mb(); !waitqueue_active(&state->wq); }));
> free_extent_state(state);
> if (need_resched()) {
> spin_unlock(&tree->lock);
Maybe just one before the while loop? This shouldn't be changing once
clear_btree_io_tree is called
>
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index f96996a1b70c..121df0fe5128 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -2739,6 +2739,10 @@ out_wake_log_root:
> atomic_set(&log_root_tree->log_commit[index2], 0);
> mutex_unlock(&log_root_tree->log_mutex);
>
> + /*
> + * Make sure counter is updated before we wake up waiters.
> + */
> + smp_mb();
> if (waitqueue_active(&log_root_tree->log_commit_wait[index2]))
> wake_up(&log_root_tree->log_commit_wait[index2]);
> out:
this one is protected by the log tree mutex
>
> @@ -2750,6 +2754,10 @@ out:
> atomic_set(&root->log_commit[index1], 0);
> mutex_unlock(&root->log_mutex);
>
> + /*
> + * Make sure counter is updated before we wake up waiters.
> + */
> + smp_mb();
> if (waitqueue_active(&root->log_commit_wait[index1]))
> wake_up(&root->log_commit_wait[index1]);
> return ret;
This one we still need, but you'll get the barrier for free from
mutex_unlock
-chris
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/4] btrfs: add comments to barriers before waitqueue_active
2015-02-24 16:51 [PULL] [PATCH 0/4] Barriers + waitqueue_active fixes David Sterba
2015-02-24 16:51 ` [PATCH 1/4] btrfs: add missing barriers before waitqueue_active David Sterba
@ 2015-02-24 16:51 ` David Sterba
2015-02-24 16:51 ` [PATCH 3/4] btrfs: remove extra barrier " David Sterba
2015-02-24 16:51 ` [PATCH 4/4] btrfs: comment the rest of implicit barriers " David Sterba
3 siblings, 0 replies; 6+ messages in thread
From: David Sterba @ 2015-02-24 16:51 UTC (permalink / raw)
To: linux-btrfs; +Cc: David Sterba
Reduce number of undocumented barriers out there.
Signed-off-by: David Sterba <dsterba@suse.cz>
---
fs/btrfs/compression.c | 3 +++
fs/btrfs/extent-tree.c | 3 +--
fs/btrfs/locking.c | 3 +++
fs/btrfs/ordered-data.c | 6 ++++++
fs/btrfs/transaction.c | 3 +++
5 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index e9df8862012c..38ffb5526b1b 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -838,6 +838,9 @@ static void free_workspace(int type, struct list_head *workspace)
btrfs_compress_op[idx]->free_workspace(workspace);
atomic_dec(alloc_workspace);
wake:
+ /*
+ * Make sure counter is updated before we wake up waiters.
+ */
smp_mb();
if (waitqueue_active(workspace_wait))
wake_up(workspace_wait);
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 28ce5c8004d4..46ca10ab243f 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -9732,8 +9732,7 @@ void btrfs_end_write_no_snapshoting(struct btrfs_root *root)
{
percpu_counter_dec(&root->subv_writers->counter);
/*
- * Make sure counter is updated before we wake up
- * waiters.
+ * Make sure counter is updated before we wake up waiters.
*/
smp_mb();
if (waitqueue_active(&root->subv_writers->wait))
diff --git a/fs/btrfs/locking.c b/fs/btrfs/locking.c
index f8229ef1b46d..ec69baf28692 100644
--- a/fs/btrfs/locking.c
+++ b/fs/btrfs/locking.c
@@ -279,6 +279,9 @@ void btrfs_tree_unlock(struct extent_buffer *eb)
if (blockers) {
WARN_ON(atomic_read(&eb->spinning_writers));
atomic_dec(&eb->blocking_writers);
+ /*
+ * Make sure counter is updated before we wake up waiters.
+ */
smp_mb();
if (waitqueue_active(&eb->write_lock_wq))
wake_up(&eb->write_lock_wq);
diff --git a/fs/btrfs/ordered-data.c b/fs/btrfs/ordered-data.c
index 534544e08f76..09dc73e7df7b 100644
--- a/fs/btrfs/ordered-data.c
+++ b/fs/btrfs/ordered-data.c
@@ -352,6 +352,9 @@ int btrfs_dec_test_first_ordered_pending(struct inode *inode,
if (entry->bytes_left == 0) {
ret = test_and_set_bit(BTRFS_ORDERED_IO_DONE, &entry->flags);
+ /*
+ * Implicit memory barrier after test_and_set_bit
+ */
if (waitqueue_active(&entry->wait))
wake_up(&entry->wait);
} else {
@@ -416,6 +419,9 @@ have_entry:
if (entry->bytes_left == 0) {
ret = test_and_set_bit(BTRFS_ORDERED_IO_DONE, &entry->flags);
+ /*
+ * Implicit memory barrier after test_and_set_bit
+ */
if (waitqueue_active(&entry->wait))
wake_up(&entry->wait);
} else {
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 90ba0c3c3d0d..6302939345a8 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -814,6 +814,9 @@ static int __btrfs_end_transaction(struct btrfs_trans_handle *trans,
atomic_dec(&cur_trans->num_writers);
extwriter_counter_dec(cur_trans, trans->type);
+ /*
+ * Make sure counter is updated before we wake up waiters.
+ */
smp_mb();
if (waitqueue_active(&cur_trans->writer_wait))
wake_up(&cur_trans->writer_wait);
--
1.8.4.5
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 3/4] btrfs: remove extra barrier before waitqueue_active
2015-02-24 16:51 [PULL] [PATCH 0/4] Barriers + waitqueue_active fixes David Sterba
2015-02-24 16:51 ` [PATCH 1/4] btrfs: add missing barriers before waitqueue_active David Sterba
2015-02-24 16:51 ` [PATCH 2/4] btrfs: add comments to " David Sterba
@ 2015-02-24 16:51 ` David Sterba
2015-02-24 16:51 ` [PATCH 4/4] btrfs: comment the rest of implicit barriers " David Sterba
3 siblings, 0 replies; 6+ messages in thread
From: David Sterba @ 2015-02-24 16:51 UTC (permalink / raw)
To: linux-btrfs; +Cc: David Sterba
Removing barriers is scary, but a call to atomic_dec_and_test implies
a barrier, so we don't need to issue another one.
Signed-off-by: David Sterba <dsterba@suse.cz>
---
fs/btrfs/tree-log.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index 121df0fe5128..07ed248ef0e7 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -238,7 +238,9 @@ int btrfs_pin_log_trans(struct btrfs_root *root)
void btrfs_end_log_trans(struct btrfs_root *root)
{
if (atomic_dec_and_test(&root->log_writers)) {
- smp_mb();
+ /*
+ * Implicit memory barrier after atomic_dec_and_test
+ */
if (waitqueue_active(&root->log_writer_wait))
wake_up(&root->log_writer_wait);
}
@@ -2610,7 +2612,9 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
mutex_lock(&log_root_tree->log_mutex);
if (atomic_dec_and_test(&log_root_tree->log_writers)) {
- smp_mb();
+ /*
+ * Implicit memory barrier after atomic_dec_and_test
+ */
if (waitqueue_active(&log_root_tree->log_writer_wait))
wake_up(&log_root_tree->log_writer_wait);
}
--
1.8.4.5
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 4/4] btrfs: comment the rest of implicit barriers before waitqueue_active
2015-02-24 16:51 [PULL] [PATCH 0/4] Barriers + waitqueue_active fixes David Sterba
` (2 preceding siblings ...)
2015-02-24 16:51 ` [PATCH 3/4] btrfs: remove extra barrier " David Sterba
@ 2015-02-24 16:51 ` David Sterba
3 siblings, 0 replies; 6+ messages in thread
From: David Sterba @ 2015-02-24 16:51 UTC (permalink / raw)
To: linux-btrfs; +Cc: David Sterba
There are atomic operations that imply the barrier for waitqueue_active
mixed in an if-condition.
Signed-off-by: David Sterba <dsterba@suse.cz>
---
fs/btrfs/delayed-inode.c | 4 ++++
fs/btrfs/disk-io.c | 3 +++
fs/btrfs/inode.c | 3 +++
fs/btrfs/locking.c | 9 +++++++++
fs/btrfs/volumes.c | 3 +++
5 files changed, 22 insertions(+)
diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
index 82f0c7c95474..913d8295cb3f 100644
--- a/fs/btrfs/delayed-inode.c
+++ b/fs/btrfs/delayed-inode.c
@@ -463,6 +463,10 @@ static int __btrfs_add_delayed_deletion_item(struct btrfs_delayed_node *node,
static void finish_one_item(struct btrfs_delayed_root *delayed_root)
{
int seq = atomic_inc_return(&delayed_root->items_seq);
+
+ /*
+ * atomic_dec_return implies a barrier for waitqueue_active
+ */
if ((atomic_dec_return(&delayed_root->items) <
BTRFS_DELAYED_BACKGROUND || seq % BTRFS_DELAYED_BATCH == 0) &&
waitqueue_active(&delayed_root->wait))
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 41b320e235d7..7c29d81bd4b1 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -807,6 +807,9 @@ static void run_one_async_done(struct btrfs_work *work)
limit = btrfs_async_submit_limit(fs_info);
limit = limit * 2 / 3;
+ /*
+ * atomic_dec_return implies a barrier for waitqueue_active
+ */
if (atomic_dec_return(&fs_info->nr_async_submits) < limit &&
waitqueue_active(&fs_info->async_submit_wait))
wake_up(&fs_info->async_submit_wait);
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 8564d8ce03de..11c7866184c6 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -1089,6 +1089,9 @@ static noinline void async_cow_submit(struct btrfs_work *work)
nr_pages = (async_cow->end - async_cow->start + PAGE_CACHE_SIZE) >>
PAGE_CACHE_SHIFT;
+ /*
+ * atomic_sub_return implies a barrier for waitqueue_active
+ */
if (atomic_sub_return(nr_pages, &root->fs_info->async_delalloc_pages) <
5 * 1024 * 1024 &&
waitqueue_active(&root->fs_info->async_submit_wait))
diff --git a/fs/btrfs/locking.c b/fs/btrfs/locking.c
index ec69baf28692..fe6de74bacda 100644
--- a/fs/btrfs/locking.c
+++ b/fs/btrfs/locking.c
@@ -79,6 +79,9 @@ void btrfs_clear_lock_blocking_rw(struct extent_buffer *eb, int rw)
write_lock(&eb->lock);
WARN_ON(atomic_read(&eb->spinning_writers));
atomic_inc(&eb->spinning_writers);
+ /*
+ * atomic_dec_and_test implies a barrier for waitqueue_active
+ */
if (atomic_dec_and_test(&eb->blocking_writers) &&
waitqueue_active(&eb->write_lock_wq))
wake_up(&eb->write_lock_wq);
@@ -86,6 +89,9 @@ void btrfs_clear_lock_blocking_rw(struct extent_buffer *eb, int rw)
BUG_ON(atomic_read(&eb->blocking_readers) == 0);
read_lock(&eb->lock);
atomic_inc(&eb->spinning_readers);
+ /*
+ * atomic_dec_and_test implies a barrier for waitqueue_active
+ */
if (atomic_dec_and_test(&eb->blocking_readers) &&
waitqueue_active(&eb->read_lock_wq))
wake_up(&eb->read_lock_wq);
@@ -229,6 +235,9 @@ void btrfs_tree_read_unlock_blocking(struct extent_buffer *eb)
}
btrfs_assert_tree_read_locked(eb);
WARN_ON(atomic_read(&eb->blocking_readers) == 0);
+ /*
+ * atomic_dec_and_test implies a barrier for waitqueue_active
+ */
if (atomic_dec_and_test(&eb->blocking_readers) &&
waitqueue_active(&eb->read_lock_wq))
wake_up(&eb->read_lock_wq);
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 8222f6f74147..8e34332c5f2e 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -341,6 +341,9 @@ loop_lock:
pending = pending->bi_next;
cur->bi_next = NULL;
+ /*
+ * atomic_dec_return implies a barrier for waitqueue_active
+ */
if (atomic_dec_return(&fs_info->nr_async_bios) < limit &&
waitqueue_active(&fs_info->async_submit_wait))
wake_up(&fs_info->async_submit_wait);
--
1.8.4.5
^ permalink raw reply related [flat|nested] 6+ messages in thread