Barriers around waitqueue_active (V2)

GIT PULL New David Sterba 1 participant 7 messages Oct 11, 2015 Btrfs file system

Share & tools

David Sterba

author maintainer

2015-10-11 17:51 UTC #0 lore raw

Hi,

I've updated the main patch according to your comments in
https://www.mail-archive.com/linux-btrfs%40vger.kernel.org/msg42551.html

though with one exception, the barrier in btrfs_bio_counter_sub which seems to be in performance sensitive context but I did not find a good way how determine that dev-replace is running. It's protected by the shared satus of 'mutually_exclusive_operation_running' and we'd have to do some other checks that would also imply some performance drop. I hope it's ok to skipt that one for now, the remaining new barriers look safe and the rest is documenting the existing ones.

Please consider pulling this for the 4.4 cycle. Thanks.

----------------------------------------------------------------
The following changes since commit 9ffecb10283508260936b96022d4ee43a7798b4c:

Linux 4.3-rc3 (2015-09-27 07:50:08 -0400)

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 ee86395458072760d62e66aad10a5e9e8902b8cf:

btrfs: comment the rest of implicit barriers before waitqueue_active (2015-10-10 18:42:00 +0200)

----------------------------------------------------------------
David Sterba (6):
btrfs: remove waitqueue_active check from btrfs_rm_dev_replace_unblocked
btrfs: add barrier for waitqueue_active in clear_btree_io_tree
btrfs: comment waitqueue_active implied by locks
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   |  3 +--
 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        |  6 +++++-
 fs/btrfs/transaction.c   |  9 +++++++++
 fs/btrfs/tree-log.c      | 14 ++++++++++++--
 fs/btrfs/volumes.c       |  3 +++
 12 files changed, 62 insertions(+), 7 deletions(-)

David Sterba

author maintainer

2015-10-11 17:51 UTC #1 lore raw

PATCH 1/6 btrfs: remove waitqueue_active check from btrfs_rm_dev_replace_unblocked

Normally the waitqueue_active would need a barrier, but this is not necessary here because it's not a performance sensitive context and we can call wake_up directly.

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/dev-replace.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

fs/btrfs/dev-replace.c +1 -2

index e54dd5905cee..733ff75b620e 100644
@@ -454,8 +454,7 @@ 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);
-	if (waitqueue_active(&fs_info->replace_wait))
-		wake_up(&fs_info->replace_wait);
+	wake_up(&fs_info->replace_wait);
 }
 
 static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,

Signature · 1 line

2.1.3

David Sterba

author maintainer

2015-10-11 17:51 UTC #2 lore raw

PATCH 2/6 btrfs: add barrier for waitqueue_active in clear_btree_io_tree

In reply to David Sterba

waitqueue_active should be preceded by a barrier, in this function we don't need to call it all the time.

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/transaction.c | 6 ++++++
 1 file changed, 6 insertions(+)

fs/btrfs/transaction.c +6 -0

index 74bc3338418b..e8e5b5a10719 100644
@@ -82,6 +82,12 @@ void btrfs_put_transaction(struct btrfs_transaction *transaction)
 static void clear_btree_io_tree(struct extent_io_tree *tree)
 {
 	spin_lock(&tree->lock);
+	/*
+	 * Do a single barrier for the waitqueue_active check here, the state
+	 * of the waitqueue should not change once clear_btree_io_tree is
+	 * called.
+	 */
+	smp_mb();
 	while (!RB_EMPTY_ROOT(&tree->state)) {
 		struct rb_node *node;
 		struct extent_state *state;

Signature · 1 line

2.1.3

David Sterba

author maintainer

2015-10-11 17:51 UTC #3 lore raw

PATCH 3/6 btrfs: comment waitqueue_active implied by locks

In reply to David Sterba

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/raid56.c   | 6 +++++-
 fs/btrfs/tree-log.c | 6 ++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

fs/btrfs/raid56.c +5 -1

index fcf7265ca46f..1a33d3eb36de 100644
@@ -810,7 +810,11 @@ static noinline void unlock_stripe(struct btrfs_raid_bio *rbio)
 			}
 
 			goto done_nolock;
-		} else  if (waitqueue_active(&h->wait)) {
+			/*
+			 * The barrier for this waitqueue_active is not needed,
+			 * we're protected by h->lock and can't miss a wakeup.
+			 */
+		} else if (waitqueue_active(&h->wait)) {
 			spin_unlock(&rbio->bio_list_lock);
 			spin_unlock_irqrestore(&h->lock, flags);
 			wake_up(&h->wait);

fs/btrfs/tree-log.c +6 -0

index 1bbaace73383..d0deb4643502 100644
@@ -2950,6 +2950,9 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
 	atomic_set(&log_root_tree->log_commit[index2], 0);
 	mutex_unlock(&log_root_tree->log_mutex);
 
+	/*
+	 * The barrier before waitqueue_active is implied by mutex_unlock
+	 */
 	if (waitqueue_active(&log_root_tree->log_commit_wait[index2]))
 		wake_up(&log_root_tree->log_commit_wait[index2]);
 out:

@@ -2961,6 +2964,9 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
 	atomic_set(&root->log_commit[index1], 0);
 	mutex_unlock(&root->log_mutex);
 
+	/*
+	 * The barrier before waitqueue_active is implied by mutex_unlock
+	 */
 	if (waitqueue_active(&root->log_commit_wait[index1]))
 		wake_up(&root->log_commit_wait[index1]);
 	return ret;

Signature · 1 line

2.1.3

David Sterba

author maintainer

2015-10-11 17:51 UTC #4 lore raw

PATCH 4/6 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 57ee8ca29b06..3a9317ce67f8 100644
@@ -839,6 +839,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 9f9604201333..59eb92f65c62 100644
@@ -10363,8 +10363,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 d7e6baf1b205..03f8630dbaf2 100644
@@ -280,6 +280,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 52170cf1757e..071005f008c1 100644
@@ -345,6 +345,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 {

@@ -409,6 +412,9 @@ int btrfs_dec_test_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 {

fs/btrfs/transaction.c +3 -0

index e8e5b5a10719..3fd70f797b7d 100644
@@ -861,6 +861,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

2.1.3

David Sterba

author maintainer

2015-10-11 17:51 UTC #5 lore raw

PATCH 5/6 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 d0deb4643502..63275594debd 100644
@@ -229,7 +229,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);
 	}

@@ -2820,7 +2822,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

2.1.3

David Sterba

author maintainer

2015-10-11 17:51 UTC #6 lore raw

PATCH 6/6 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 a2ae42720a6a..e0941fbb913c 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 295795aebe0b..379526ffd84d 100644
@@ -802,6 +802,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 611b66d73e80..7be4abe25e06 100644
@@ -1096,6 +1096,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 03f8630dbaf2..8077461fc56a 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 6fc735869c18..ff3527192409 100644
@@ -345,6 +345,9 @@ static noinline void run_scheduled_bios(struct btrfs_device *device)
 		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

2.1.3