[PATCH 0/2] Misc locking fixes

[PATCH 0/2] Misc locking fixes

[Nikolay Borisov]

Here are two patches cleaning up unused functions and fixing a newly introduced
deadlock due to memory barriers. Especially the memory barrier one needs to be 
sent merged ASAP. 

Nikolay Borisov (2):
  btrfs: Remove unused locking functions
  btrfs: Fix deadlock caused by missing memory barrier

 fs/btrfs/locking.c           | 45 +++++-------------------------------
 fs/btrfs/locking.h           |  2 --
 include/trace/events/btrfs.h |  2 --
 3 files changed, 6 insertions(+), 43 deletions(-)

-- 
2.17.1

[PATCH 1/2] btrfs: Remove unused locking functions

[Nikolay Borisov]

Those were split out of btrfs_clear_lock_blocking_rw by
aa12c02778a9 ("btrfs: split btrfs_clear_lock_blocking_rw to read and write helpers")
however at that time this function was unused due to commit
523983401644 ("Btrfs: kill btrfs_clear_path_blocking"). Put the final
nail in the coffin of those 2 functions.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/locking.c           | 36 ------------------------------------
 fs/btrfs/locking.h           |  2 --
 include/trace/events/btrfs.h |  2 --
 3 files changed, 40 deletions(-)

diff --git a/fs/btrfs/locking.c b/fs/btrfs/locking.c
index fe21bfc4e787..ceb23690b89b 100644
--- a/fs/btrfs/locking.c
+++ b/fs/btrfs/locking.c
@@ -119,42 +119,6 @@ void btrfs_set_lock_blocking_write(struct extent_buffer *eb)
 	}
 }
 
-void btrfs_clear_lock_blocking_read(struct extent_buffer *eb)
-{
-	trace_btrfs_clear_lock_blocking_read(eb);
-	/*
-	 * No lock is required.  The lock owner may change if we have a read
-	 * lock, but it won't change to or away from us.  If we have the write
-	 * lock, we are the owner and it'll never change.
-	 */
-	if (eb->lock_nested && current->pid == eb->lock_owner)
-		return;
-	BUG_ON(atomic_read(&eb->blocking_readers) == 0);
-	read_lock(&eb->lock);
-	btrfs_assert_spinning_readers_get(eb);
-	/* atomic_dec_and_test implies a barrier */
-	if (atomic_dec_and_test(&eb->blocking_readers))
-		cond_wake_up_nomb(&eb->read_lock_wq);
-}
-
-void btrfs_clear_lock_blocking_write(struct extent_buffer *eb)
-{
-	trace_btrfs_clear_lock_blocking_write(eb);
-	/*
-	 * no lock is required.  The lock owner may change if
-	 * we have a read lock, but it won't change to or away
-	 * from us.  If we have the write lock, we are the owner
-	 * and it'll never change.
-	 */
-	if (eb->lock_nested && current->pid == eb->lock_owner)
-		return;
-	write_lock(&eb->lock);
-	BUG_ON(eb->blocking_writers != 1);
-	btrfs_assert_spinning_writers_get(eb);
-	if (--eb->blocking_writers == 0)
-		cond_wake_up(&eb->write_lock_wq);
-}
-
 /*
  * take a spinning read lock.  This will wait for any blocking
  * writers
diff --git a/fs/btrfs/locking.h b/fs/btrfs/locking.h
index 27627d4fd3a9..6ee384027b94 100644
--- a/fs/btrfs/locking.h
+++ b/fs/btrfs/locking.h
@@ -24,8 +24,6 @@ void btrfs_tree_read_unlock(struct extent_buffer *eb);
 void btrfs_tree_read_unlock_blocking(struct extent_buffer *eb);
 void btrfs_set_lock_blocking_read(struct extent_buffer *eb);
 void btrfs_set_lock_blocking_write(struct extent_buffer *eb);
-void btrfs_clear_lock_blocking_read(struct extent_buffer *eb);
-void btrfs_clear_lock_blocking_write(struct extent_buffer *eb);
 void btrfs_assert_tree_locked(struct extent_buffer *eb);
 int btrfs_try_tree_read_lock(struct extent_buffer *eb);
 int btrfs_try_tree_write_lock(struct extent_buffer *eb);
diff --git a/include/trace/events/btrfs.h b/include/trace/events/btrfs.h
index 2f6a669408bb..5cb95646b94e 100644
--- a/include/trace/events/btrfs.h
+++ b/include/trace/events/btrfs.h
@@ -2086,8 +2086,6 @@ DEFINE_BTRFS_LOCK_EVENT(btrfs_tree_read_unlock);
 DEFINE_BTRFS_LOCK_EVENT(btrfs_tree_read_unlock_blocking);
 DEFINE_BTRFS_LOCK_EVENT(btrfs_set_lock_blocking_read);
 DEFINE_BTRFS_LOCK_EVENT(btrfs_set_lock_blocking_write);
-DEFINE_BTRFS_LOCK_EVENT(btrfs_clear_lock_blocking_read);
-DEFINE_BTRFS_LOCK_EVENT(btrfs_clear_lock_blocking_write);
 DEFINE_BTRFS_LOCK_EVENT(btrfs_try_tree_read_lock);
 DEFINE_BTRFS_LOCK_EVENT(btrfs_try_tree_write_lock);
 DEFINE_BTRFS_LOCK_EVENT(btrfs_tree_read_lock_atomic);
-- 
2.17.1

[PATCH 2/2] btrfs: Fix deadlock caused by missing memory barrier

[Nikolay Borisov]

Commit 06297d8cefca ("btrfs: switch extent_buffer blocking_writers from atomic to int")
changed the type of blocking_writers but forgot to adjust relevant code
in btrfs_tree_unlock by converting the smp_mb__after_atomic to smp_mb.
This opened up the possibility of a deadlock due to re-ordering of
setting blocking_writers and checking/waking up the  waiter. This
particular lockup is explained in a comment above waitqueue_active()
function.

Fix it by converting the memory barrier to a full smp_mb, accounting
for the fact that blocking_writers is a simple integer.

Fixes: 06297d8cefca ("btrfs: switch extent_buffer blocking_writers from atomic to int")
Signed-off-by: Nikolay Borisov <nborisov@suse.com>
Tested-by: Johannes Thumshirn <jthumshirn@suse.com>
---
 fs/btrfs/locking.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/locking.c b/fs/btrfs/locking.c
index ceb23690b89b..5e867720df88 100644
--- a/fs/btrfs/locking.c
+++ b/fs/btrfs/locking.c
@@ -310,9 +310,12 @@ void btrfs_tree_unlock(struct extent_buffer *eb)
 	if (blockers) {
 		btrfs_assert_no_spinning_writers(eb);
 		eb->blocking_writers--;
-		/* Use the lighter barrier after atomic */
-		smp_mb__after_atomic();
-		cond_wake_up_nomb(&eb->write_lock_wq);
+		/*
+		 * We need to order modifying blocking_writers above with
+		 * actually waking up the sleepers to ensure they see the
+		 * updated value of blocking_writers
+		 */
+		cond_wake_up(&eb->write_lock_wq);
 	} else {
 		btrfs_assert_spinning_writers_put(eb);
 		write_unlock(&eb->lock);
-- 
2.17.1

Re: [PATCH 2/2] btrfs: Fix deadlock caused by missing memory barrier

[David Sterba]

On Thu, Jul 25, 2019 at 11:27:29AM +0300, Nikolay Borisov wrote:
> Commit 06297d8cefca ("btrfs: switch extent_buffer blocking_writers from atomic to int")
> changed the type of blocking_writers but forgot to adjust relevant code
> in btrfs_tree_unlock by converting the smp_mb__after_atomic to smp_mb.
> This opened up the possibility of a deadlock due to re-ordering of
> setting blocking_writers and checking/waking up the  waiter. This
> particular lockup is explained in a comment above waitqueue_active()
> function.
> 
> Fix it by converting the memory barrier to a full smp_mb, accounting
> for the fact that blocking_writers is a simple integer.
> 
> Fixes: 06297d8cefca ("btrfs: switch extent_buffer blocking_writers from atomic to int")
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> Tested-by: Johannes Thumshirn <jthumshirn@suse.com>

Reviewed-by: David Sterba <dsterba@suse.com>