All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] Extent buffer lock cleanups
@ 2019-05-30 16:30 David Sterba
  2019-05-30 16:31 ` [PATCH 1/3] btrfs: switch extent_buffer blocking_writers from atomic to int David Sterba
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: David Sterba @ 2019-05-30 16:30 UTC (permalink / raw)
  To: linux-btrfs; +Cc: David Sterba

There are 3 atomics that don't need to be, all related to writes where
the exclusivity is guaranteed by the lock.

David Sterba (3):
  btrfs: switch extent_buffer blocking_writers from atomic to int
  btrfs: switch extent_buffer spinning_writers from atomic to int
  btrfs: switch extent_buffer write_locks from atomic to int

 fs/btrfs/extent_io.c  |  6 ++---
 fs/btrfs/extent_io.h  |  6 ++---
 fs/btrfs/locking.c    | 62 +++++++++++++++++++------------------------
 fs/btrfs/print-tree.c |  6 ++---
 4 files changed, 37 insertions(+), 43 deletions(-)

-- 
2.21.0


^ permalink raw reply	[flat|nested] 9+ messages in thread
* [PATCH 1/3] btrfs: switch extent_buffer blocking_writers from atomic to int
  2019-05-30 16:30 [PATCH 0/3] Extent buffer lock cleanups David Sterba
@ 2019-05-30 16:31 ` David Sterba
  2019-05-31  8:29   ` Nikolay Borisov
  2019-05-30 16:31 ` [PATCH 2/3] btrfs: switch extent_buffer spinning_writers " David Sterba
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: David Sterba @ 2019-05-30 16:31 UTC (permalink / raw)
  To: linux-btrfs; +Cc: David Sterba

The blocking_writers is either 0 or 1 and always updated under the lock,
so we don't need the atomic_t semantics.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/extent_io.c  |  2 +-
 fs/btrfs/extent_io.h  |  2 +-
 fs/btrfs/locking.c    | 46 +++++++++++++++++++------------------------
 fs/btrfs/print-tree.c |  2 +-
 4 files changed, 23 insertions(+), 29 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 2f38c10d2bfb..57b6de9df7c4 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4823,7 +4823,7 @@ __alloc_extent_buffer(struct btrfs_fs_info *fs_info, u64 start,
 	eb->bflags = 0;
 	rwlock_init(&eb->lock);
 	atomic_set(&eb->blocking_readers, 0);
-	atomic_set(&eb->blocking_writers, 0);
+	eb->blocking_writers = 0;
 	eb->lock_nested = false;
 	init_waitqueue_head(&eb->write_lock_wq);
 	init_waitqueue_head(&eb->read_lock_wq);
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index aa18a16a6ed7..201da61dfc21 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -167,7 +167,7 @@ struct extent_buffer {
 	struct rcu_head rcu_head;
 	pid_t lock_owner;
 
-	atomic_t blocking_writers;
+	int blocking_writers;
 	atomic_t blocking_readers;
 	bool lock_nested;
 	/* >= 0 if eb belongs to a log tree, -1 otherwise */
diff --git a/fs/btrfs/locking.c b/fs/btrfs/locking.c
index 2f6c3c7851ed..5feb01147e19 100644
--- a/fs/btrfs/locking.c
+++ b/fs/btrfs/locking.c
@@ -111,10 +111,10 @@ void btrfs_set_lock_blocking_write(struct extent_buffer *eb)
 	 */
 	if (eb->lock_nested && current->pid == eb->lock_owner)
 		return;
-	if (atomic_read(&eb->blocking_writers) == 0) {
+	if (eb->blocking_writers == 0) {
 		btrfs_assert_spinning_writers_put(eb);
 		btrfs_assert_tree_locked(eb);
-		atomic_inc(&eb->blocking_writers);
+		eb->blocking_writers++;
 		write_unlock(&eb->lock);
 	}
 }
@@ -148,12 +148,11 @@ void btrfs_clear_lock_blocking_write(struct extent_buffer *eb)
 	 */
 	if (eb->lock_nested && current->pid == eb->lock_owner)
 		return;
-	BUG_ON(atomic_read(&eb->blocking_writers) != 1);
 	write_lock(&eb->lock);
+	BUG_ON(eb->blocking_writers != 1);
 	btrfs_assert_spinning_writers_get(eb);
-	/* atomic_dec_and_test implies a barrier */
-	if (atomic_dec_and_test(&eb->blocking_writers))
-		cond_wake_up_nomb(&eb->write_lock_wq);
+	if (--eb->blocking_writers == 0)
+		cond_wake_up(&eb->write_lock_wq);
 }
 
 /*
@@ -167,12 +166,10 @@ void btrfs_tree_read_lock(struct extent_buffer *eb)
 	if (trace_btrfs_tree_read_lock_enabled())
 		start_ns = ktime_get_ns();
 again:
-	BUG_ON(!atomic_read(&eb->blocking_writers) &&
-	       current->pid == eb->lock_owner);
-
 	read_lock(&eb->lock);
-	if (atomic_read(&eb->blocking_writers) &&
-	    current->pid == eb->lock_owner) {
+	BUG_ON(eb->blocking_writers == 0 &&
+	       current->pid == eb->lock_owner);
+	if (eb->blocking_writers && current->pid == eb->lock_owner) {
 		/*
 		 * This extent is already write-locked by our thread. We allow
 		 * an additional read lock to be added because it's for the same
@@ -185,10 +182,10 @@ void btrfs_tree_read_lock(struct extent_buffer *eb)
 		trace_btrfs_tree_read_lock(eb, start_ns);
 		return;
 	}
-	if (atomic_read(&eb->blocking_writers)) {
+	if (eb->blocking_writers) {
 		read_unlock(&eb->lock);
 		wait_event(eb->write_lock_wq,
-			   atomic_read(&eb->blocking_writers) == 0);
+			   eb->blocking_writers == 0);
 		goto again;
 	}
 	btrfs_assert_tree_read_locks_get(eb);
@@ -203,11 +200,11 @@ void btrfs_tree_read_lock(struct extent_buffer *eb)
  */
 int btrfs_tree_read_lock_atomic(struct extent_buffer *eb)
 {
-	if (atomic_read(&eb->blocking_writers))
+	if (eb->blocking_writers)
 		return 0;
 
 	read_lock(&eb->lock);
-	if (atomic_read(&eb->blocking_writers)) {
+	if (eb->blocking_writers) {
 		read_unlock(&eb->lock);
 		return 0;
 	}
@@ -223,13 +220,13 @@ int btrfs_tree_read_lock_atomic(struct extent_buffer *eb)
  */
 int btrfs_try_tree_read_lock(struct extent_buffer *eb)
 {
-	if (atomic_read(&eb->blocking_writers))
+	if (eb->blocking_writers)
 		return 0;
 
 	if (!read_trylock(&eb->lock))
 		return 0;
 
-	if (atomic_read(&eb->blocking_writers)) {
+	if (eb->blocking_writers) {
 		read_unlock(&eb->lock);
 		return 0;
 	}
@@ -245,13 +242,11 @@ int btrfs_try_tree_read_lock(struct extent_buffer *eb)
  */
 int btrfs_try_tree_write_lock(struct extent_buffer *eb)
 {
-	if (atomic_read(&eb->blocking_writers) ||
-	    atomic_read(&eb->blocking_readers))
+	if (eb->blocking_writers || atomic_read(&eb->blocking_readers))
 		return 0;
 
 	write_lock(&eb->lock);
-	if (atomic_read(&eb->blocking_writers) ||
-	    atomic_read(&eb->blocking_readers)) {
+	if (eb->blocking_writers || atomic_read(&eb->blocking_readers)) {
 		write_unlock(&eb->lock);
 		return 0;
 	}
@@ -322,10 +317,9 @@ void btrfs_tree_lock(struct extent_buffer *eb)
 	WARN_ON(eb->lock_owner == current->pid);
 again:
 	wait_event(eb->read_lock_wq, atomic_read(&eb->blocking_readers) == 0);
-	wait_event(eb->write_lock_wq, atomic_read(&eb->blocking_writers) == 0);
+	wait_event(eb->write_lock_wq, eb->blocking_writers == 0);
 	write_lock(&eb->lock);
-	if (atomic_read(&eb->blocking_readers) ||
-	    atomic_read(&eb->blocking_writers)) {
+	if (atomic_read(&eb->blocking_readers) || eb->blocking_writers) {
 		write_unlock(&eb->lock);
 		goto again;
 	}
@@ -340,7 +334,7 @@ void btrfs_tree_lock(struct extent_buffer *eb)
  */
 void btrfs_tree_unlock(struct extent_buffer *eb)
 {
-	int blockers = atomic_read(&eb->blocking_writers);
+	int blockers = eb->blocking_writers;
 
 	BUG_ON(blockers > 1);
 
@@ -351,7 +345,7 @@ void btrfs_tree_unlock(struct extent_buffer *eb)
 
 	if (blockers) {
 		btrfs_assert_no_spinning_writers(eb);
-		atomic_dec(&eb->blocking_writers);
+		eb->blocking_writers--;
 		/* Use the lighter barrier after atomic */
 		smp_mb__after_atomic();
 		cond_wake_up_nomb(&eb->write_lock_wq);
diff --git a/fs/btrfs/print-tree.c b/fs/btrfs/print-tree.c
index 1141ca5fae6a..7cb4f1fbe043 100644
--- a/fs/btrfs/print-tree.c
+++ b/fs/btrfs/print-tree.c
@@ -155,7 +155,7 @@ static void print_eb_refs_lock(struct extent_buffer *eb)
 "refs %u lock (w:%d r:%d bw:%d br:%d sw:%d sr:%d) lock_owner %u current %u",
 		   atomic_read(&eb->refs), atomic_read(&eb->write_locks),
 		   atomic_read(&eb->read_locks),
-		   atomic_read(&eb->blocking_writers),
+		   eb->blocking_writers,
 		   atomic_read(&eb->blocking_readers),
 		   atomic_read(&eb->spinning_writers),
 		   atomic_read(&eb->spinning_readers),
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread
* [PATCH 2/3] btrfs: switch extent_buffer spinning_writers from atomic to int
  2019-05-30 16:30 [PATCH 0/3] Extent buffer lock cleanups David Sterba
  2019-05-30 16:31 ` [PATCH 1/3] btrfs: switch extent_buffer blocking_writers from atomic to int David Sterba
@ 2019-05-30 16:31 ` David Sterba
  2019-05-31  9:19   ` Nikolay Borisov
  2019-05-30 16:31 ` [PATCH 3/3] btrfs: switch extent_buffer write_locks " David Sterba
  2019-06-07 13:31 ` [PATCH 0/3] Extent buffer lock cleanups David Sterba
  3 siblings, 1 reply; 9+ messages in thread
From: David Sterba @ 2019-05-30 16:31 UTC (permalink / raw)
  To: linux-btrfs; +Cc: David Sterba

The spinning_writers is either 0 or 1 and always updated under the lock,
so we don't need the atomic_t semantics.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/extent_io.c  |  2 +-
 fs/btrfs/extent_io.h  |  2 +-
 fs/btrfs/locking.c    | 10 +++++-----
 fs/btrfs/print-tree.c |  2 +-
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 57b6de9df7c4..71ee9e976307 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4842,7 +4842,7 @@ __alloc_extent_buffer(struct btrfs_fs_info *fs_info, u64 start,
 	BUG_ON(len > MAX_INLINE_EXTENT_BUFFER_SIZE);
 
 #ifdef CONFIG_BTRFS_DEBUG
-	atomic_set(&eb->spinning_writers, 0);
+	eb->spinning_writers = 0;
 	atomic_set(&eb->spinning_readers, 0);
 	atomic_set(&eb->read_locks, 0);
 	atomic_set(&eb->write_locks, 0);
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index 201da61dfc21..5616b96c365d 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -187,7 +187,7 @@ struct extent_buffer {
 	wait_queue_head_t read_lock_wq;
 	struct page *pages[INLINE_EXTENT_BUFFER_PAGES];
 #ifdef CONFIG_BTRFS_DEBUG
-	atomic_t spinning_writers;
+	int spinning_writers;
 	atomic_t spinning_readers;
 	atomic_t read_locks;
 	atomic_t write_locks;
diff --git a/fs/btrfs/locking.c b/fs/btrfs/locking.c
index 5feb01147e19..270667627977 100644
--- a/fs/btrfs/locking.c
+++ b/fs/btrfs/locking.c
@@ -15,19 +15,19 @@
 #ifdef CONFIG_BTRFS_DEBUG
 static void btrfs_assert_spinning_writers_get(struct extent_buffer *eb)
 {
-	WARN_ON(atomic_read(&eb->spinning_writers));
-	atomic_inc(&eb->spinning_writers);
+	WARN_ON(eb->spinning_writers);
+	eb->spinning_writers++;
 }
 
 static void btrfs_assert_spinning_writers_put(struct extent_buffer *eb)
 {
-	WARN_ON(atomic_read(&eb->spinning_writers) != 1);
-	atomic_dec(&eb->spinning_writers);
+	WARN_ON(eb->spinning_writers != 1);
+	eb->spinning_writers--;
 }
 
 static void btrfs_assert_no_spinning_writers(struct extent_buffer *eb)
 {
-	WARN_ON(atomic_read(&eb->spinning_writers));
+	WARN_ON(eb->spinning_writers);
 }
 
 static void btrfs_assert_spinning_readers_get(struct extent_buffer *eb)
diff --git a/fs/btrfs/print-tree.c b/fs/btrfs/print-tree.c
index 7cb4f1fbe043..c5cc435ed39a 100644
--- a/fs/btrfs/print-tree.c
+++ b/fs/btrfs/print-tree.c
@@ -157,7 +157,7 @@ static void print_eb_refs_lock(struct extent_buffer *eb)
 		   atomic_read(&eb->read_locks),
 		   eb->blocking_writers,
 		   atomic_read(&eb->blocking_readers),
-		   atomic_read(&eb->spinning_writers),
+		   eb->spinning_writers,
 		   atomic_read(&eb->spinning_readers),
 		   eb->lock_owner, current->pid);
 #endif
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread
* [PATCH 3/3] btrfs: switch extent_buffer write_locks from atomic to int
  2019-05-30 16:30 [PATCH 0/3] Extent buffer lock cleanups David Sterba
  2019-05-30 16:31 ` [PATCH 1/3] btrfs: switch extent_buffer blocking_writers from atomic to int David Sterba
  2019-05-30 16:31 ` [PATCH 2/3] btrfs: switch extent_buffer spinning_writers " David Sterba
@ 2019-05-30 16:31 ` David Sterba
  2019-05-31  9:20   ` Nikolay Borisov
  2019-06-07 13:31 ` [PATCH 0/3] Extent buffer lock cleanups David Sterba
  3 siblings, 1 reply; 9+ messages in thread
From: David Sterba @ 2019-05-30 16:31 UTC (permalink / raw)
  To: linux-btrfs; +Cc: David Sterba

The write_locks is either 0 or 1 and always updated under the lock,
so we don't need the atomic_t semantics.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/extent_io.c  | 2 +-
 fs/btrfs/extent_io.h  | 2 +-
 fs/btrfs/locking.c    | 6 +++---
 fs/btrfs/print-tree.c | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 71ee9e976307..6d75d4dcf473 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4845,7 +4845,7 @@ __alloc_extent_buffer(struct btrfs_fs_info *fs_info, u64 start,
 	eb->spinning_writers = 0;
 	atomic_set(&eb->spinning_readers, 0);
 	atomic_set(&eb->read_locks, 0);
-	atomic_set(&eb->write_locks, 0);
+	eb->write_locks = 0;
 #endif
 
 	return eb;
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index 5616b96c365d..844e595cde5b 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -190,7 +190,7 @@ struct extent_buffer {
 	int spinning_writers;
 	atomic_t spinning_readers;
 	atomic_t read_locks;
-	atomic_t write_locks;
+	int write_locks;
 	struct list_head leak_list;
 #endif
 };
diff --git a/fs/btrfs/locking.c b/fs/btrfs/locking.c
index 270667627977..98fccce4208c 100644
--- a/fs/btrfs/locking.c
+++ b/fs/btrfs/locking.c
@@ -58,17 +58,17 @@ static void btrfs_assert_tree_read_locked(struct extent_buffer *eb)
 
 static void btrfs_assert_tree_write_locks_get(struct extent_buffer *eb)
 {
-	atomic_inc(&eb->write_locks);
+	eb->write_locks++;
 }
 
 static void btrfs_assert_tree_write_locks_put(struct extent_buffer *eb)
 {
-	atomic_dec(&eb->write_locks);
+	eb->write_locks--;
 }
 
 void btrfs_assert_tree_locked(struct extent_buffer *eb)
 {
-	BUG_ON(!atomic_read(&eb->write_locks));
+	BUG_ON(!eb->write_locks);
 }
 
 #else
diff --git a/fs/btrfs/print-tree.c b/fs/btrfs/print-tree.c
index c5cc435ed39a..9cb50577d982 100644
--- a/fs/btrfs/print-tree.c
+++ b/fs/btrfs/print-tree.c
@@ -153,7 +153,7 @@ static void print_eb_refs_lock(struct extent_buffer *eb)
 #ifdef CONFIG_BTRFS_DEBUG
 	btrfs_info(eb->fs_info,
 "refs %u lock (w:%d r:%d bw:%d br:%d sw:%d sr:%d) lock_owner %u current %u",
-		   atomic_read(&eb->refs), atomic_read(&eb->write_locks),
+		   atomic_read(&eb->refs), eb->write_locks,
 		   atomic_read(&eb->read_locks),
 		   eb->blocking_writers,
 		   atomic_read(&eb->blocking_readers),
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] btrfs: switch extent_buffer blocking_writers from atomic to int
  2019-05-30 16:31 ` [PATCH 1/3] btrfs: switch extent_buffer blocking_writers from atomic to int David Sterba
@ 2019-05-31  8:29   ` Nikolay Borisov
  0 siblings, 0 replies; 9+ messages in thread
From: Nikolay Borisov @ 2019-05-31  8:29 UTC (permalink / raw)
  To: David Sterba, linux-btrfs



On 30.05.19 г. 19:31 ч., David Sterba wrote:
> The blocking_writers is either 0 or 1 and always updated under the lock,
> so we don't need the atomic_t semantics.>
> Signed-off-by: David Sterba <dsterba@suse.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> ---
>  fs/btrfs/extent_io.c  |  2 +-
>  fs/btrfs/extent_io.h  |  2 +-
>  fs/btrfs/locking.c    | 46 +++++++++++++++++++------------------------
>  fs/btrfs/print-tree.c |  2 +-
>  4 files changed, 23 insertions(+), 29 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 2f38c10d2bfb..57b6de9df7c4 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -4823,7 +4823,7 @@ __alloc_extent_buffer(struct btrfs_fs_info *fs_info, u64 start,
>  	eb->bflags = 0;
>  	rwlock_init(&eb->lock);
>  	atomic_set(&eb->blocking_readers, 0);
> -	atomic_set(&eb->blocking_writers, 0);
> +	eb->blocking_writers = 0;
>  	eb->lock_nested = false;
>  	init_waitqueue_head(&eb->write_lock_wq);
>  	init_waitqueue_head(&eb->read_lock_wq);
> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
> index aa18a16a6ed7..201da61dfc21 100644
> --- a/fs/btrfs/extent_io.h
> +++ b/fs/btrfs/extent_io.h
> @@ -167,7 +167,7 @@ struct extent_buffer {
>  	struct rcu_head rcu_head;
>  	pid_t lock_owner;
>  
> -	atomic_t blocking_writers;
> +	int blocking_writers;
>  	atomic_t blocking_readers;
>  	bool lock_nested;
>  	/* >= 0 if eb belongs to a log tree, -1 otherwise */
> diff --git a/fs/btrfs/locking.c b/fs/btrfs/locking.c
> index 2f6c3c7851ed..5feb01147e19 100644
> --- a/fs/btrfs/locking.c
> +++ b/fs/btrfs/locking.c
> @@ -111,10 +111,10 @@ void btrfs_set_lock_blocking_write(struct extent_buffer *eb)
>  	 */
>  	if (eb->lock_nested && current->pid == eb->lock_owner)
>  		return;
> -	if (atomic_read(&eb->blocking_writers) == 0) {
> +	if (eb->blocking_writers == 0) {
>  		btrfs_assert_spinning_writers_put(eb);
>  		btrfs_assert_tree_locked(eb);
> -		atomic_inc(&eb->blocking_writers);
> +		eb->blocking_writers++;
>  		write_unlock(&eb->lock);
>  	}
>  }
> @@ -148,12 +148,11 @@ void btrfs_clear_lock_blocking_write(struct extent_buffer *eb)
>  	 */
>  	if (eb->lock_nested && current->pid == eb->lock_owner)
>  		return;
> -	BUG_ON(atomic_read(&eb->blocking_writers) != 1);
>  	write_lock(&eb->lock);
> +	BUG_ON(eb->blocking_writers != 1);
>  	btrfs_assert_spinning_writers_get(eb);
> -	/* atomic_dec_and_test implies a barrier */
> -	if (atomic_dec_and_test(&eb->blocking_writers))
> -		cond_wake_up_nomb(&eb->write_lock_wq);
> +	if (--eb->blocking_writers == 0)
> +		cond_wake_up(&eb->write_lock_wq);
>  }
>  
>  /*
> @@ -167,12 +166,10 @@ void btrfs_tree_read_lock(struct extent_buffer *eb)
>  	if (trace_btrfs_tree_read_lock_enabled())
>  		start_ns = ktime_get_ns();
>  again:
> -	BUG_ON(!atomic_read(&eb->blocking_writers) &&
> -	       current->pid == eb->lock_owner);
> -
>  	read_lock(&eb->lock);
> -	if (atomic_read(&eb->blocking_writers) &&
> -	    current->pid == eb->lock_owner) {
> +	BUG_ON(eb->blocking_writers == 0 &&
> +	       current->pid == eb->lock_owner);
> +	if (eb->blocking_writers && current->pid == eb->lock_owner) {
>  		/*
>  		 * This extent is already write-locked by our thread. We allow
>  		 * an additional read lock to be added because it's for the same
> @@ -185,10 +182,10 @@ void btrfs_tree_read_lock(struct extent_buffer *eb)
>  		trace_btrfs_tree_read_lock(eb, start_ns);
>  		return;
>  	}
> -	if (atomic_read(&eb->blocking_writers)) {
> +	if (eb->blocking_writers) {
>  		read_unlock(&eb->lock);
>  		wait_event(eb->write_lock_wq,
> -			   atomic_read(&eb->blocking_writers) == 0);
> +			   eb->blocking_writers == 0);
>  		goto again;
>  	}
>  	btrfs_assert_tree_read_locks_get(eb);
> @@ -203,11 +200,11 @@ void btrfs_tree_read_lock(struct extent_buffer *eb)
>   */
>  int btrfs_tree_read_lock_atomic(struct extent_buffer *eb)
>  {
> -	if (atomic_read(&eb->blocking_writers))
> +	if (eb->blocking_writers)
>  		return 0;
>  
>  	read_lock(&eb->lock);
> -	if (atomic_read(&eb->blocking_writers)) {
> +	if (eb->blocking_writers) {
>  		read_unlock(&eb->lock);
>  		return 0;
>  	}
> @@ -223,13 +220,13 @@ int btrfs_tree_read_lock_atomic(struct extent_buffer *eb)
>   */
>  int btrfs_try_tree_read_lock(struct extent_buffer *eb)
>  {
> -	if (atomic_read(&eb->blocking_writers))
> +	if (eb->blocking_writers)
>  		return 0;
>  
>  	if (!read_trylock(&eb->lock))
>  		return 0;
>  
> -	if (atomic_read(&eb->blocking_writers)) {
> +	if (eb->blocking_writers) {
>  		read_unlock(&eb->lock);
>  		return 0;
>  	}
> @@ -245,13 +242,11 @@ int btrfs_try_tree_read_lock(struct extent_buffer *eb)
>   */
>  int btrfs_try_tree_write_lock(struct extent_buffer *eb)
>  {
> -	if (atomic_read(&eb->blocking_writers) ||
> -	    atomic_read(&eb->blocking_readers))
> +	if (eb->blocking_writers || atomic_read(&eb->blocking_readers))
>  		return 0;
>  
>  	write_lock(&eb->lock);
> -	if (atomic_read(&eb->blocking_writers) ||
> -	    atomic_read(&eb->blocking_readers)) {
> +	if (eb->blocking_writers || atomic_read(&eb->blocking_readers)) {
>  		write_unlock(&eb->lock);
>  		return 0;
>  	}
> @@ -322,10 +317,9 @@ void btrfs_tree_lock(struct extent_buffer *eb)
>  	WARN_ON(eb->lock_owner == current->pid);
>  again:
>  	wait_event(eb->read_lock_wq, atomic_read(&eb->blocking_readers) == 0);
> -	wait_event(eb->write_lock_wq, atomic_read(&eb->blocking_writers) == 0);
> +	wait_event(eb->write_lock_wq, eb->blocking_writers == 0);
>  	write_lock(&eb->lock);
> -	if (atomic_read(&eb->blocking_readers) ||
> -	    atomic_read(&eb->blocking_writers)) {
> +	if (atomic_read(&eb->blocking_readers) || eb->blocking_writers) {
>  		write_unlock(&eb->lock);
>  		goto again;
>  	}
> @@ -340,7 +334,7 @@ void btrfs_tree_lock(struct extent_buffer *eb)
>   */
>  void btrfs_tree_unlock(struct extent_buffer *eb)
>  {
> -	int blockers = atomic_read(&eb->blocking_writers);
> +	int blockers = eb->blocking_writers;
>  
>  	BUG_ON(blockers > 1);
>  
> @@ -351,7 +345,7 @@ void btrfs_tree_unlock(struct extent_buffer *eb)
>  
>  	if (blockers) {
>  		btrfs_assert_no_spinning_writers(eb);
> -		atomic_dec(&eb->blocking_writers);
> +		eb->blocking_writers--;
>  		/* Use the lighter barrier after atomic */
>  		smp_mb__after_atomic();
>  		cond_wake_up_nomb(&eb->write_lock_wq);
> diff --git a/fs/btrfs/print-tree.c b/fs/btrfs/print-tree.c
> index 1141ca5fae6a..7cb4f1fbe043 100644
> --- a/fs/btrfs/print-tree.c
> +++ b/fs/btrfs/print-tree.c
> @@ -155,7 +155,7 @@ static void print_eb_refs_lock(struct extent_buffer *eb)
>  "refs %u lock (w:%d r:%d bw:%d br:%d sw:%d sr:%d) lock_owner %u current %u",
>  		   atomic_read(&eb->refs), atomic_read(&eb->write_locks),
>  		   atomic_read(&eb->read_locks),
> -		   atomic_read(&eb->blocking_writers),
> +		   eb->blocking_writers,
>  		   atomic_read(&eb->blocking_readers),
>  		   atomic_read(&eb->spinning_writers),
>  		   atomic_read(&eb->spinning_readers),
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] btrfs: switch extent_buffer spinning_writers from atomic to int
  2019-05-30 16:31 ` [PATCH 2/3] btrfs: switch extent_buffer spinning_writers " David Sterba
@ 2019-05-31  9:19   ` Nikolay Borisov
  2019-05-31 11:28     ` David Sterba
  0 siblings, 1 reply; 9+ messages in thread
From: Nikolay Borisov @ 2019-05-31  9:19 UTC (permalink / raw)
  To: David Sterba, linux-btrfs



On 30.05.19 г. 19:31 ч., David Sterba wrote:
> The spinning_writers is either 0 or 1 and always updated under the lock,
> so we don't need the atomic_t semantics.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/extent_io.c  |  2 +-
>  fs/btrfs/extent_io.h  |  2 +-
>  fs/btrfs/locking.c    | 10 +++++-----
>  fs/btrfs/print-tree.c |  2 +-
>  4 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 57b6de9df7c4..71ee9e976307 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -4842,7 +4842,7 @@ __alloc_extent_buffer(struct btrfs_fs_info *fs_info, u64 start,
>  	BUG_ON(len > MAX_INLINE_EXTENT_BUFFER_SIZE);
>  
>  #ifdef CONFIG_BTRFS_DEBUG
> -	atomic_set(&eb->spinning_writers, 0);
> +	eb->spinning_writers = 0;
>  	atomic_set(&eb->spinning_readers, 0);
>  	atomic_set(&eb->read_locks, 0);
>  	atomic_set(&eb->write_locks, 0);
> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
> index 201da61dfc21..5616b96c365d 100644
> --- a/fs/btrfs/extent_io.h
> +++ b/fs/btrfs/extent_io.h
> @@ -187,7 +187,7 @@ struct extent_buffer {
>  	wait_queue_head_t read_lock_wq;
>  	struct page *pages[INLINE_EXTENT_BUFFER_PAGES];
>  #ifdef CONFIG_BTRFS_DEBUG
> -	atomic_t spinning_writers;
> +	int spinning_writers;
>  	atomic_t spinning_readers;
>  	atomic_t read_locks;
>  	atomic_t write_locks;
> diff --git a/fs/btrfs/locking.c b/fs/btrfs/locking.c
> index 5feb01147e19..270667627977 100644
> --- a/fs/btrfs/locking.c
> +++ b/fs/btrfs/locking.c
> @@ -15,19 +15,19 @@
>  #ifdef CONFIG_BTRFS_DEBUG
>  static void btrfs_assert_spinning_writers_get(struct extent_buffer *eb)
>  {
> -	WARN_ON(atomic_read(&eb->spinning_writers));
> -	atomic_inc(&eb->spinning_writers);
> +	WARN_ON(eb->spinning_writers);
> +	eb->spinning_writers++;
>  }
>  
>  static void btrfs_assert_spinning_writers_put(struct extent_buffer *eb)
>  {
> -	WARN_ON(atomic_read(&eb->spinning_writers) != 1);
> -	atomic_dec(&eb->spinning_writers);
> +	WARN_ON(eb->spinning_writers != 1);
> +	eb->spinning_writers--;
>  }
>  
>  static void btrfs_assert_no_spinning_writers(struct extent_buffer *eb)
>  {
> -	WARN_ON(atomic_read(&eb->spinning_writers));
> +	WARN_ON(eb->spinning_writers);
>  }

IMO longterm  it will be good if those debug functions contained
lockdep_assert_held_exclusive/read macros for posterity.

>  
>  static void btrfs_assert_spinning_readers_get(struct extent_buffer *eb)
> diff --git a/fs/btrfs/print-tree.c b/fs/btrfs/print-tree.c
> index 7cb4f1fbe043..c5cc435ed39a 100644
> --- a/fs/btrfs/print-tree.c
> +++ b/fs/btrfs/print-tree.c
> @@ -157,7 +157,7 @@ static void print_eb_refs_lock(struct extent_buffer *eb)
>  		   atomic_read(&eb->read_locks),
>  		   eb->blocking_writers,
>  		   atomic_read(&eb->blocking_readers),
> -		   atomic_read(&eb->spinning_writers),
> +		   eb->spinning_writers,
>  		   atomic_read(&eb->spinning_readers),
>  		   eb->lock_owner, current->pid);
>  #endif
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] btrfs: switch extent_buffer write_locks from atomic to int
  2019-05-30 16:31 ` [PATCH 3/3] btrfs: switch extent_buffer write_locks " David Sterba
@ 2019-05-31  9:20   ` Nikolay Borisov
  0 siblings, 0 replies; 9+ messages in thread
From: Nikolay Borisov @ 2019-05-31  9:20 UTC (permalink / raw)
  To: David Sterba, linux-btrfs



On 30.05.19 г. 19:31 ч., David Sterba wrote:
> The write_locks is either 0 or 1 and always updated under the lock,
> so we don't need the atomic_t semantics.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>

Generally looks good, though my remark for patch2 remains.

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> ---
>  fs/btrfs/extent_io.c  | 2 +-
>  fs/btrfs/extent_io.h  | 2 +-
>  fs/btrfs/locking.c    | 6 +++---
>  fs/btrfs/print-tree.c | 2 +-
>  4 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 71ee9e976307..6d75d4dcf473 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -4845,7 +4845,7 @@ __alloc_extent_buffer(struct btrfs_fs_info *fs_info, u64 start,
>  	eb->spinning_writers = 0;
>  	atomic_set(&eb->spinning_readers, 0);
>  	atomic_set(&eb->read_locks, 0);
> -	atomic_set(&eb->write_locks, 0);
> +	eb->write_locks = 0;
>  #endif
>  
>  	return eb;
> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
> index 5616b96c365d..844e595cde5b 100644
> --- a/fs/btrfs/extent_io.h
> +++ b/fs/btrfs/extent_io.h
> @@ -190,7 +190,7 @@ struct extent_buffer {
>  	int spinning_writers;
>  	atomic_t spinning_readers;
>  	atomic_t read_locks;
> -	atomic_t write_locks;
> +	int write_locks;
>  	struct list_head leak_list;
>  #endif
>  };
> diff --git a/fs/btrfs/locking.c b/fs/btrfs/locking.c
> index 270667627977..98fccce4208c 100644
> --- a/fs/btrfs/locking.c
> +++ b/fs/btrfs/locking.c
> @@ -58,17 +58,17 @@ static void btrfs_assert_tree_read_locked(struct extent_buffer *eb)
>  
>  static void btrfs_assert_tree_write_locks_get(struct extent_buffer *eb)
>  {
> -	atomic_inc(&eb->write_locks);
> +	eb->write_locks++;
>  }
>  
>  static void btrfs_assert_tree_write_locks_put(struct extent_buffer *eb)
>  {
> -	atomic_dec(&eb->write_locks);
> +	eb->write_locks--;
>  }
>  
>  void btrfs_assert_tree_locked(struct extent_buffer *eb)
>  {
> -	BUG_ON(!atomic_read(&eb->write_locks));
> +	BUG_ON(!eb->write_locks);
>  }
>  
>  #else
> diff --git a/fs/btrfs/print-tree.c b/fs/btrfs/print-tree.c
> index c5cc435ed39a..9cb50577d982 100644
> --- a/fs/btrfs/print-tree.c
> +++ b/fs/btrfs/print-tree.c
> @@ -153,7 +153,7 @@ static void print_eb_refs_lock(struct extent_buffer *eb)
>  #ifdef CONFIG_BTRFS_DEBUG
>  	btrfs_info(eb->fs_info,
>  "refs %u lock (w:%d r:%d bw:%d br:%d sw:%d sr:%d) lock_owner %u current %u",
> -		   atomic_read(&eb->refs), atomic_read(&eb->write_locks),
> +		   atomic_read(&eb->refs), eb->write_locks,
>  		   atomic_read(&eb->read_locks),
>  		   eb->blocking_writers,
>  		   atomic_read(&eb->blocking_readers),
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] btrfs: switch extent_buffer spinning_writers from atomic to int
  2019-05-31  9:19   ` Nikolay Borisov
@ 2019-05-31 11:28     ` David Sterba
  0 siblings, 0 replies; 9+ messages in thread
From: David Sterba @ 2019-05-31 11:28 UTC (permalink / raw)
  To: Nikolay Borisov; +Cc: David Sterba, linux-btrfs

On Fri, May 31, 2019 at 12:19:15PM +0300, Nikolay Borisov wrote:
> >  {
> > -	WARN_ON(atomic_read(&eb->spinning_writers));
> > -	atomic_inc(&eb->spinning_writers);
> > +	WARN_ON(eb->spinning_writers);
> > +	eb->spinning_writers++;
> >  }
> >  
> >  static void btrfs_assert_spinning_writers_put(struct extent_buffer *eb)
> >  {
> > -	WARN_ON(atomic_read(&eb->spinning_writers) != 1);
> > -	atomic_dec(&eb->spinning_writers);
> > +	WARN_ON(eb->spinning_writers != 1);
> > +	eb->spinning_writers--;
> >  }
> >  
> >  static void btrfs_assert_no_spinning_writers(struct extent_buffer *eb)
> >  {
> > -	WARN_ON(atomic_read(&eb->spinning_writers));
> > +	WARN_ON(eb->spinning_writers);
> >  }
> 
> IMO longterm  it will be good if those debug functions contained
> lockdep_assert_held_exclusive/read macros for posterity.

The functions are not public and used only inside implementation of
locks, so the chances of wrong use are low so I don't see much value
adding it.

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH 0/3] Extent buffer lock cleanups
  2019-05-30 16:30 [PATCH 0/3] Extent buffer lock cleanups David Sterba
                   ` (2 preceding siblings ...)
  2019-05-30 16:31 ` [PATCH 3/3] btrfs: switch extent_buffer write_locks " David Sterba
@ 2019-06-07 13:31 ` David Sterba
  3 siblings, 0 replies; 9+ messages in thread
From: David Sterba @ 2019-06-07 13:31 UTC (permalink / raw)
  To: David Sterba; +Cc: linux-btrfs

On Thu, May 30, 2019 at 06:30:57PM +0200, David Sterba wrote:
> There are 3 atomics that don't need to be, all related to writes where
> the exclusivity is guaranteed by the lock.
> 
> David Sterba (3):
>   btrfs: switch extent_buffer blocking_writers from atomic to int
>   btrfs: switch extent_buffer spinning_writers from atomic to int
>   btrfs: switch extent_buffer write_locks from atomic to int
> 
>  fs/btrfs/extent_io.c  |  6 ++---
>  fs/btrfs/extent_io.h  |  6 ++---
>  fs/btrfs/locking.c    | 62 +++++++++++++++++++------------------------
>  fs/btrfs/print-tree.c |  6 ++---
>  4 files changed, 37 insertions(+), 43 deletions(-)

Added to misc-next.

^ permalink raw reply	[flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-06-07 13:30 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-30 16:30 [PATCH 0/3] Extent buffer lock cleanups David Sterba
2019-05-30 16:31 ` [PATCH 1/3] btrfs: switch extent_buffer blocking_writers from atomic to int David Sterba
2019-05-31  8:29   ` Nikolay Borisov
2019-05-30 16:31 ` [PATCH 2/3] btrfs: switch extent_buffer spinning_writers " David Sterba
2019-05-31  9:19   ` Nikolay Borisov
2019-05-31 11:28     ` David Sterba
2019-05-30 16:31 ` [PATCH 3/3] btrfs: switch extent_buffer write_locks " David Sterba
2019-05-31  9:20   ` Nikolay Borisov
2019-06-07 13:31 ` [PATCH 0/3] Extent buffer lock cleanups David Sterba

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.