Cleanup waitqueue_active and barriers

Add convenience wrappers for the waitqueue management that involves memory barriers to prevent deadlocks. The helpers will let us remove barriers and the necessary comments in several places.

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

---
 fs/btrfs/ctree.h | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

fs/btrfs/ctree.h +23 -0

index bf545e7552b8..c9827a1b676d 100644
@@ -3748,4 +3748,27 @@ static inline int btrfs_is_testing(struct btrfs_fs_info *fs_info)
 #endif
 	return 0;
 }
+
+static inline void cond_wake_up(struct wait_queue_head *wq)
+{
+	/*
+	 * This implies a full smp_mb barrier, see comments for
+	 * waitqueue_active why.
+	 */
+	if (wq_has_sleeper(wq))
+		wake_up(wq);
+}
+
+static inline void cond_wake_up_nomb(struct wait_queue_head *wq)
+{
+	/*
+	 * Special case for conditional wakeup where the barrier required for
+	 * waitqueue_active is implied by some of the preceding code. Eg. one
+	 * of such atomic operations (atomic_dec_and_return, ...), or a
+	 * unlock/lock sequence, etc.
+	 */
+	if (waitqueue_active(wq))
+		wake_up(wq);
+}
+
 #endif

Use the wrappers and reduce the amount of low-level details about the waitqueue management.

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

---
 fs/btrfs/compression.c   |  7 +------
 fs/btrfs/delayed-inode.c |  9 +++------
 fs/btrfs/dev-replace.c   | 10 ++++------
 fs/btrfs/extent-tree.c   |  7 +------
 fs/btrfs/inode.c         |  9 +++------
 fs/btrfs/locking.c       | 34 +++++++++++-----------------------
 fs/btrfs/ordered-data.c  | 14 ++++----------
 fs/btrfs/transaction.c   |  7 +------
 fs/btrfs/tree-log.c      | 28 ++++++++--------------------
 9 files changed, 36 insertions(+), 89 deletions(-)

fs/btrfs/compression.c +1 -6

index 562c3e633403..2d2d7380d381 100644
@@ -1003,12 +1003,7 @@ static void __free_workspace(int type, struct list_head *workspace,
 		btrfs_compress_op[idx]->free_workspace(workspace);
 	atomic_dec(total_ws);
 wake:
-	/*
-	 * Make sure counter is updated before we wake up waiters.
-	 */
-	smp_mb();
-	if (waitqueue_active(ws_wait))
-		wake_up(ws_wait);
+	cond_wake_up(ws_wait);
 }
 
 static void free_workspace(int type, struct list_head *ws)

fs/btrfs/delayed-inode.c +3 -6

index d06bef16ebd5..3e7f5f26ff0f 100644
@@ -472,13 +472,10 @@ 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
-	 */
+	/* 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))
-		wake_up(&delayed_root->wait);
+	    BTRFS_DELAYED_BACKGROUND || seq % BTRFS_DELAYED_BATCH == 0))
+		cond_wake_up_nomb(&delayed_root->wait);
 }
 
 static void __btrfs_remove_delayed_item(struct btrfs_delayed_item *delayed_item)

fs/btrfs/dev-replace.c +3 -3

index e279f04b3388..f498572155f1 100644
@@ -928,9 +928,9 @@ void btrfs_dev_replace_clear_lock_blocking(
 	ASSERT(atomic_read(&dev_replace->read_locks) > 0);
 	ASSERT(atomic_read(&dev_replace->blocking_readers) > 0);
 	read_lock(&dev_replace->lock);
-	if (atomic_dec_and_test(&dev_replace->blocking_readers) &&
-	    waitqueue_active(&dev_replace->read_lock_wq))
-		wake_up(&dev_replace->read_lock_wq);
+	/* Barrier implied by atomic_dec_and_test */
+	if (atomic_dec_and_test(&dev_replace->blocking_readers))
+		cond_wake_up_nomb(&dev_replace->read_lock_wq);
 }
 
 void btrfs_bio_counter_inc_noblocked(struct btrfs_fs_info *fs_info)

fs/btrfs/dev-replace.c +0 -0

 }
 
 void btrfs_bio_counter_inc_blocked(struct btrfs_fs_info *fs_info)

fs/btrfs/extent-tree.c +1 -6

index 2760292e1175..d57801711884 100644
@@ -10999,12 +10999,7 @@ int btrfs_trim_fs(struct btrfs_fs_info *fs_info, struct fstrim_range *range)
 void btrfs_end_write_no_snapshotting(struct btrfs_root *root)
 {
 	percpu_counter_dec(&root->subv_writers->counter);
-	/*
-	 * Make sure counter is updated before we wake up waiters.
-	 */
-	smp_mb();
-	if (waitqueue_active(&root->subv_writers->wait))
-		wake_up(&root->subv_writers->wait);
+	cond_wake_up(&root->subv_writers->wait);
 }
 
 int btrfs_start_write_no_snapshotting(struct btrfs_root *root)

fs/btrfs/inode.c +3 -6

index fc5b7d82b842..b963b5b4734e 100644
@@ -1168,13 +1168,10 @@ static noinline void async_cow_submit(struct btrfs_work *work)
 	nr_pages = (async_cow->end - async_cow->start + PAGE_SIZE) >>
 		PAGE_SHIFT;
 
-	/*
-	 * atomic_sub_return implies a barrier for waitqueue_active
-	 */
+	/* atomic_sub_return implies a barrier */
 	if (atomic_sub_return(nr_pages, &fs_info->async_delalloc_pages) <
-	    5 * SZ_1M &&
-	    waitqueue_active(&fs_info->async_submit_wait))
-		wake_up(&fs_info->async_submit_wait);
+	    5 * SZ_1M)
+		cond_wake_up_nomb(&fs_info->async_submit_wait);
 
 	if (async_cow->inode)
 		submit_compressed_extents(async_cow->inode, async_cow);

fs/btrfs/locking.c +11 -23

index 621083f8932c..cce666cd104e 100644
@@ -78,22 +78,16 @@ 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);
+		/* atomic_dec_and_test implies a barrier */
+		if (atomic_dec_and_test(&eb->blocking_writers))
+			cond_wake_up_nomb(&eb->write_lock_wq);
 	} else if (rw == BTRFS_READ_LOCK_BLOCKING) {
 		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);
+		/* atomic_dec_and_test implies a barrier */
+		if (atomic_dec_and_test(&eb->blocking_readers))
+			cond_wake_up_nomb(&eb->read_lock_wq);
 	}
 }
 

@@ -233,12 +227,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);
+	/* atomic_dec_and_test implies a barrier */
+	if (atomic_dec_and_test(&eb->blocking_readers))
+		cond_wake_up_nomb(&eb->read_lock_wq);
 	atomic_dec(&eb->read_locks);
 }
 

@@ -287,12 +278,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.
-		 */
+		/* Use the lighter barrier after atomic */
 		smp_mb__after_atomic();
-		if (waitqueue_active(&eb->write_lock_wq))
-			wake_up(&eb->write_lock_wq);
+		cond_wake_up_nomb(&eb->write_lock_wq);
 	} else {
 		WARN_ON(atomic_read(&eb->spinning_writers) != 1);
 		atomic_dec(&eb->spinning_writers);

fs/btrfs/ordered-data.c +4 -10

index 9be98e42cfb6..66ff1419e2e0 100644
@@ -356,11 +356,8 @@ 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);
+		/* test_and_set_bit implies a barrier */
+		cond_wake_up_nomb(&entry->wait);
 	} else {
 		ret = 1;
 	}

@@ -423,11 +420,8 @@ 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);
+		/* test_and_set_bit implies a barrier */
+		cond_wake_up_nomb(&entry->wait);
 	} else {
 		ret = 1;
 	}

fs/btrfs/transaction.c +1 -6

index eab15777ba88..f431223196a0 100644
@@ -909,12 +909,7 @@ 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);
+	cond_wake_up(&cur_trans->writer_wait);
 	btrfs_put_transaction(cur_trans);
 
 	if (current->journal_info == trans)

fs/btrfs/tree-log.c +4 -10

index 7b8fee45b29e..979fc02214d4 100644
@@ -235,11 +235,8 @@ 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)) {
-		/*
-		 * Implicit memory barrier after atomic_dec_and_test
-		 */
-		if (waitqueue_active(&root->log_writer_wait))
-			wake_up(&root->log_writer_wait);
+		/* atomic_dec_and_test implies a barrier */
+		cond_wake_up_nomb(&root->log_writer_wait);
 	}
 }
 

@@ -2965,11 +2962,8 @@ 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)) {
-		/*
-		 * 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);
+		/* atomic_dec_and_test implies a barrier */
+		cond_wake_up_nomb(&log_root_tree->log_writer_wait);
 	}
 
 	if (ret) {

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

 out:
 	mutex_lock(&root->log_mutex);
 	btrfs_remove_all_log_ctxs(root, index1, ret);

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

 	return ret;
 }
 

fs/btrfs/dev-replace.c void btrfs_bio_counter_inc_noblocked(struct btrfs_fs_info *fs_info) · line 944

Quoted 8 lines

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

@@ -941,9 +941,7 @@ 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);
-
-	if (waitqueue_active(&fs_info->replace_wait))
-		wake_up(&fs_info->replace_wait);
+	cond_wake_up_nomb(&fs_info->replace_wait);

nit/offtopic:

I think here the code requires comments since we have 2 types of waiters for fs_info->replace_wait. One is dependent on the percpu_counter_sum (i.e. the btrfs_rm_dev_replace_blocked). And then there is another condition on the same wait entry - the btrfs_bio_counter_inc_blocked i.e:

wait_event(fs_info->replace_wait,

                           !test_bit(BTRFS_FS_STATE_DEV_REPLACING,              
                                     &fs_info->fs_state)); 

geez, who would come up with such synchronization ...

Quoted 7 lines

fs/btrfs/dev-replace.c +0 -0

 }
 
 void btrfs_bio_counter_inc_blocked(struct btrfs_fs_info *fs_info)

@@ -3092,11 +3086,8 @@ 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);
 

fs/btrfs/tree-log.c int btrfs_sync_log(struct btrfs_trans_handle *trans, · line 3090

Quoted 7 lines

fs/btrfs/tree-log.c +2 -5

-	/*
-	 * 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]);
+	/* The barrier is implied by mutex_unlock */
+	cond_wake_up_nomb(&log_root_tree->log_commit_wait[index2]);

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

Quoted 6 lines

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

 out:
 	mutex_lock(&root->log_mutex);
 	btrfs_remove_all_log_ctxs(root, index1, ret);

fs/btrfs/tree-log.c int btrfs_sync_log(struct btrfs_trans_handle *trans, · line 3099

Quoted 10 lines

fs/btrfs/tree-log.c +2 -5

@@ -3104,11 +3095,8 @@ 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]);
+	/* The barrier is implied by mutex_unlock */
+	cond_wake_up_nomb(&root->log_commit_wait[index1]);

ditto.

Quoted 3 lines

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

 	return ret;
 }
 

Quoted 17 lines · Nikolay Borisov

fs/btrfs/dev-replace.c void btrfs_bio_counter_inc_noblocked(struct btrfs_fs_info *fs_info) · line 944

Quoted 8 lines

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

@@ -941,9 +941,7 @@ 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);
-
-	if (waitqueue_active(&fs_info->replace_wait))
-		wake_up(&fs_info->replace_wait);
+	cond_wake_up_nomb(&fs_info->replace_wait);

nit/offtopic:

I think here the code requires comments since we have 2 types of waiters for fs_info->replace_wait. One is dependent on the percpu_counter_sum (i.e. the btrfs_rm_dev_replace_blocked). And then there is another condition on the same wait entry - the btrfs_bio_counter_inc_blocked i.e:

Quoted 5 lines

wait_event(fs_info->replace_wait,

                           !test_bit(BTRFS_FS_STATE_DEV_REPLACING,              
                                     &fs_info->fs_state)); 

geez, who would come up with such synchronization ...

'git blame' is the right tool, I'm not a fan of the dev-replace locking either.

Quoted 7 lines

Quoted 7 lines

fs/btrfs/dev-replace.c +0 -0

 }
 
 void btrfs_bio_counter_inc_blocked(struct btrfs_fs_info *fs_info)

@@ -3092,11 +3086,8 @@ 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);
 

Quoted 10 lines

fs/btrfs/tree-log.c int btrfs_sync_log(struct btrfs_trans_handle *trans, · line 3090

Quoted 7 lines

fs/btrfs/tree-log.c +2 -5

-	/*
-	 * 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]);
+	/* The barrier is implied by mutex_unlock */
+	cond_wake_up_nomb(&log_root_tree->log_commit_wait[index2]);

I think this is wrong (not your code) but the original assumption that the RELEASE semantics provided by mutex_unlock is sufficient.

That was added in 33a9eca7e4a4c2c17ae by me.

Quoted 28 lines

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;                                                                     

Quoted 8 lines

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

Maybe, but the code has to be taken as a whole, the log_mutex and waiting does not follow a simple pattern that could be matched on the barriers example. In this case I think that the log_commit_wait is partially synchronized by the log_mutex and the bad scenario of missed wakeup will not happen. We'll have either empty waiter queue, or the waiters are blocked on the mutex (ie. the waitqueu is active and we'll always have someone to wake). But this is an observation made after a short time reading the code so there actually might be a tiny window where some of the assumptions do not hold.

Point of the patch is to do the transformation to the helpers. If there are bugs that could be obscured by the patch, I can postpone it until the bugs are fixed.