Skip to contentSkip to conversation

Barriers + waitqueue_active fixes

GIT PULL In review David Sterba 2 participants 6 messages Feb 24, 2015 – Mar 25, 2015 Btrfs file system

lore
Share & tools
lore

David Sterba

author
#0 lore raw

Hi,

I've noticed some missing smp_mb() in recent code before waitqueue_active and did a broader audit. Fixed and documented the rest.

The first patch is a stable candidate but does not apply as-is due to changes in 4.0-rc1.

---

The following changes since commit e57cf21e9787c081db4db6afa02e6e70112ee410:

Btrfs: fix allocation size calculations in alloc_btrfs_bio (2015-02-20 06:55:15 -0800)

are available in the git repository at:

git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git fix/waitqueue-barriers

for you to fetch changes up to 1b127581ed4eb64dc941d2ecaba3174802870c07:

btrfs: comment the rest of implicit barriers before waitqueue_active (2015-02-20 19:06:39 +0100)

----------------------------------------------------------------
David Sterba (4):
btrfs: add missing barriers before waitqueue_active
btrfs: add comments to barriers before waitqueue_active
btrfs: remove extra barrier before waitqueue_active
btrfs: comment the rest of implicit barriers before waitqueue_active

 fs/btrfs/compression.c   |  3 +++
 fs/btrfs/delayed-inode.c |  4 ++++
 fs/btrfs/dev-replace.c   |  9 ++++++++-
 fs/btrfs/disk-io.c       |  3 +++
 fs/btrfs/extent-tree.c   |  3 +--
 fs/btrfs/inode.c         |  3 +++
 fs/btrfs/locking.c       | 12 ++++++++++++
 fs/btrfs/ordered-data.c  |  6 ++++++
 fs/btrfs/raid56.c        | 17 ++++++++++++-----
 fs/btrfs/transaction.c   |  8 +++++++-
 fs/btrfs/tree-log.c      | 16 ++++++++++++++--
 fs/btrfs/volumes.c       |  3 +++
 12 files changed, 76 insertions(+), 11 deletions(-)
author
#1 lore raw

PATCH 1/4 btrfs: add missing barriers before waitqueue_active

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 <[email protected]>
---
 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(-)
fs/btrfs/dev-replace.c +8 -1
index 5ec03d999c37..30b8668396e0 100644
@@ -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);
 }
fs/btrfs/raid56.c +12 -5
index 5264858ed768..b460e193f324 100644
@@ -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:
fs/btrfs/transaction.c +4 -1
index 038fcf6051e0..90ba0c3c3d0d 100644
@@ -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);
fs/btrfs/tree-log.c +8 -0
index f96996a1b70c..121df0fe5128 100644
@@ -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;
Signature · 1 line
1.8.4.5
author
#3 lore raw

PATCH 2/4 btrfs: add comments to barriers before waitqueue_active

In reply to David Sterba

Reduce number of undocumented barriers out there.

Signed-off-by David Sterba <[email protected]>
---
 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(-)
fs/btrfs/compression.c +3 -0
index e9df8862012c..38ffb5526b1b 100644
@@ -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);
fs/btrfs/extent-tree.c +1 -2
index 28ce5c8004d4..46ca10ab243f 100644
@@ -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))
fs/btrfs/locking.c +3 -0
index f8229ef1b46d..ec69baf28692 100644
@@ -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);
fs/btrfs/ordered-data.c +6 -0
index 534544e08f76..09dc73e7df7b 100644
@@ -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 {
fs/btrfs/transaction.c +3 -0
index 90ba0c3c3d0d..6302939345a8 100644
@@ -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);
Signature · 1 line
1.8.4.5
author
#4 lore raw

PATCH 3/4 btrfs: remove extra barrier before waitqueue_active

In reply to 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 <[email protected]>
---
 fs/btrfs/tree-log.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
fs/btrfs/tree-log.c +6 -2
index 121df0fe5128..07ed248ef0e7 100644
@@ -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);
 	}
Signature · 1 line
1.8.4.5
author
#5 lore raw

PATCH 4/4 btrfs: comment the rest of implicit barriers before waitqueue_active

In reply to David Sterba

There are atomic operations that imply the barrier for waitqueue_active mixed in an if-condition.

Signed-off-by David Sterba <[email protected]>
---
 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(+)
fs/btrfs/delayed-inode.c +4 -0
index 82f0c7c95474..913d8295cb3f 100644
@@ -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))
fs/btrfs/disk-io.c +3 -0
index 41b320e235d7..7c29d81bd4b1 100644
@@ -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);
fs/btrfs/inode.c +3 -0
index 8564d8ce03de..11c7866184c6 100644
@@ -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))
fs/btrfs/locking.c +9 -0
index ec69baf28692..fe6de74bacda 100644
@@ -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);
fs/btrfs/volumes.c +3 -0
index 8222f6f74147..8e34332c5f2e 100644
@@ -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);
Signature · 1 line
1.8.4.5
#2 lore raw

Re: [PATCH 1/4] btrfs: add missing barriers before waitqueue_active

In reply to David Sterba
Quoted 2 lines · David Sterba

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:

Quoted 24 lines
Signed-off-by David Sterba <[email protected]>
---
 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(-)
fs/btrfs/dev-replace.c +4 -0
index 5ec03d999c37..30b8668396e0 100644
@@ -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))
Quoted 1 line

wake_up(&fs_info->replace_wait);

This one isn't performance critical, lets just use wake_up directly.

Quoted 14 lines

}

fs/btrfs/dev-replace.c +4 -1
@@ -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);
Quoted 1 line

}

This is performance critical. Can we do it only when a replace is actually running?

Quoted 29 lines
fs/btrfs/raid56.c +12 -5
index 5264858ed768..b460e193f324 100644
@@ -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;
+			}
 		}
 	}
Quoted 1 line

done:

This one is already protected by the h->lock, we can't miss wakeups.

Quoted 18 lines
fs/btrfs/transaction.c +4 -1
index 038fcf6051e0..90ba0c3c3d0d 100644
@@ -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()) {
Quoted 1 line

spin_unlock(&tree->lock);

Maybe just one before the while loop? This shouldn't be changing once clear_btree_io_tree is called

Quoted 6 lines
fs/btrfs/tree-log.c +0 -0
index f96996a1b70c..121df0fe5128 100644

fs/btrfs/tree-log.c out_wake_log_root: · line 2748

Quoted 10 lines
fs/btrfs/tree-log.c +4 -0
@@ -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

fs/btrfs/tree-log.c out: · line 2763

Quoted 12 lines
fs/btrfs/tree-log.c +4 -0
@@ -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