[PATCH 0/9] Extent buffer locking cleanups

[PATCH 0/9] Extent buffer locking cleanups

[David Sterba]

The series moves several atomic counters under CONFIG_BTRFS_DEBUG. The
selected counters are not essential for the extent buffer locking to
work. There's some space saving (4x 4B at least) and the cachelines are
less stressed on non-debugging builds.

The final size is 264 from 280, getting to 256 would be nice but hard or
making the code unreadable. I have ideas to shave 7 more bytes but
that's for another patchset.

David Sterba (9):
  btrfs: add assertion helpers for spinning writers
  btrfs: use assertion helpers for spinning writers
  btrfs: add assertion helpers for spinning readers
  btrfs: use assertion helpers for spinning readers
  btrfs: add assertion helpers for extent buffer read lock counters
  btrfs: use assertion helpers for extent buffer read lock counters
  btrfs: add assertion helpers for extent buffer write lock counters
  btrfs: use assertion helpers for extent buffer write lock counters
  btrfs: switch extent_buffer::lock_nested to bool

 fs/btrfs/extent_io.c |  13 +++--
 fs/btrfs/extent_io.h |  11 ++--
 fs/btrfs/locking.c   | 133 ++++++++++++++++++++++++++++++-------------
 3 files changed, 107 insertions(+), 50 deletions(-)

-- 
2.20.1

[PATCH 1/9] btrfs: add assertion helpers for spinning writers

[David Sterba]

Add helpers for conditional DEBUG build to assert that the extent buffer
spinning_writers constraints are met. Will be used in followup patches.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/locking.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/fs/btrfs/locking.c b/fs/btrfs/locking.c
index 82b84e4daad1..13ef1decdea6 100644
--- a/fs/btrfs/locking.c
+++ b/fs/btrfs/locking.c
@@ -14,6 +14,30 @@
 
 static void btrfs_assert_tree_read_locked(struct extent_buffer *eb);
 
+#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);
+}
+
+static void btrfs_assert_spinning_writers_put(struct extent_buffer *eb)
+{
+	WARN_ON(atomic_read(&eb->spinning_writers) != 1);
+	atomic_dec(&eb->spinning_writers);
+}
+
+static void btrfs_assert_no_spinning_writers(struct extent_buffer *eb)
+{
+	WARN_ON(atomic_read(&eb->spinning_writers));
+}
+
+#else
+static void btrfs_assert_spinning_writers_get(struct extent_buffer *eb) { }
+static void btrfs_assert_spinning_writers_put(struct extent_buffer *eb) { }
+static void btrfs_assert_no_spinning_writers(struct extent_buffer *eb) { }
+#endif
+
 void btrfs_set_lock_blocking_read(struct extent_buffer *eb)
 {
 	/*
-- 
2.20.1

[PATCH 2/9] btrfs: use assertion helpers for spinning writers

[David Sterba]

Use the helpers where open coded. On non-debug builds, the warnings will
not trigger and extent_buffer::spining_writers become unused and can be
moved to the appropriate section, saving a few bytes.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/extent_io.c |  5 ++++-
 fs/btrfs/extent_io.h |  2 +-
 fs/btrfs/locking.c   | 16 ++++++----------
 3 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index ca259c75bbcd..11a95919bb3e 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4673,7 +4673,6 @@ __alloc_extent_buffer(struct btrfs_fs_info *fs_info, u64 start,
 	atomic_set(&eb->blocking_readers, 0);
 	atomic_set(&eb->blocking_writers, 0);
 	atomic_set(&eb->spinning_readers, 0);
-	atomic_set(&eb->spinning_writers, 0);
 	eb->lock_nested = 0;
 	init_waitqueue_head(&eb->write_lock_wq);
 	init_waitqueue_head(&eb->read_lock_wq);
@@ -4691,6 +4690,10 @@ __alloc_extent_buffer(struct btrfs_fs_info *fs_info, u64 start,
 		> MAX_INLINE_EXTENT_BUFFER_SIZE);
 	BUG_ON(len > MAX_INLINE_EXTENT_BUFFER_SIZE);
 
+#ifdef CONFIG_BTRFS_DEBUG
+	atomic_set(&eb->spinning_writers, 0);
+#endif
+
 	return eb;
 }
 
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index 08749e0b9c32..c5eb261c1ea9 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -152,7 +152,6 @@ struct extent_buffer {
 	atomic_t blocking_writers;
 	atomic_t blocking_readers;
 	atomic_t spinning_readers;
-	atomic_t spinning_writers;
 	short lock_nested;
 	/* >= 0 if eb belongs to a log tree, -1 otherwise */
 	short log_index;
@@ -171,6 +170,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;
 	struct list_head leak_list;
 #endif
 };
diff --git a/fs/btrfs/locking.c b/fs/btrfs/locking.c
index 13ef1decdea6..a5a3c5118f61 100644
--- a/fs/btrfs/locking.c
+++ b/fs/btrfs/locking.c
@@ -64,8 +64,7 @@ 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) {
-		WARN_ON(atomic_read(&eb->spinning_writers) != 1);
-		atomic_dec(&eb->spinning_writers);
+		btrfs_assert_spinning_writers_put(eb);
 		btrfs_assert_tree_locked(eb);
 		atomic_inc(&eb->blocking_writers);
 		write_unlock(&eb->lock);
@@ -101,8 +100,7 @@ void btrfs_clear_lock_blocking_write(struct extent_buffer *eb)
 		return;
 	BUG_ON(atomic_read(&eb->blocking_writers) != 1);
 	write_lock(&eb->lock);
-	WARN_ON(atomic_read(&eb->spinning_writers));
-	atomic_inc(&eb->spinning_writers);
+	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);
@@ -200,7 +198,7 @@ int btrfs_try_tree_write_lock(struct extent_buffer *eb)
 		return 0;
 	}
 	atomic_inc(&eb->write_locks);
-	atomic_inc(&eb->spinning_writers);
+	btrfs_assert_spinning_writers_get(eb);
 	eb->lock_owner = current->pid;
 	return 1;
 }
@@ -266,8 +264,7 @@ void btrfs_tree_lock(struct extent_buffer *eb)
 		write_unlock(&eb->lock);
 		goto again;
 	}
-	WARN_ON(atomic_read(&eb->spinning_writers));
-	atomic_inc(&eb->spinning_writers);
+	btrfs_assert_spinning_writers_get(eb);
 	atomic_inc(&eb->write_locks);
 	eb->lock_owner = current->pid;
 }
@@ -286,14 +283,13 @@ void btrfs_tree_unlock(struct extent_buffer *eb)
 	atomic_dec(&eb->write_locks);
 
 	if (blockers) {
-		WARN_ON(atomic_read(&eb->spinning_writers));
+		btrfs_assert_no_spinning_writers(eb);
 		atomic_dec(&eb->blocking_writers);
 		/* Use the lighter barrier after atomic */
 		smp_mb__after_atomic();
 		cond_wake_up_nomb(&eb->write_lock_wq);
 	} else {
-		WARN_ON(atomic_read(&eb->spinning_writers) != 1);
-		atomic_dec(&eb->spinning_writers);
+		btrfs_assert_spinning_writers_put(eb);
 		write_unlock(&eb->lock);
 	}
 }
-- 
2.20.1

[PATCH 3/9] btrfs: add assertion helpers for spinning readers

[David Sterba]

Add helpers for conditional DEBUG build to assert that the extent buffer
spinning_readers constraints are met. Will be used in followup patches.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/locking.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/fs/btrfs/locking.c b/fs/btrfs/locking.c
index a5a3c5118f61..2dd3ae524aa3 100644
--- a/fs/btrfs/locking.c
+++ b/fs/btrfs/locking.c
@@ -32,10 +32,23 @@ static void btrfs_assert_no_spinning_writers(struct extent_buffer *eb)
 	WARN_ON(atomic_read(&eb->spinning_writers));
 }
 
+static void btrfs_assert_spinning_readers_get(struct extent_buffer *eb)
+{
+	atomic_inc(&eb->spinning_readers);
+}
+
+static void btrfs_assert_spinning_readers_put(struct extent_buffer *eb)
+{
+	WARN_ON(atomic_read(&eb->spinning_readers) == 0);
+	atomic_dec(&eb->spinning_readers);
+}
+
 #else
 static void btrfs_assert_spinning_writers_get(struct extent_buffer *eb) { }
 static void btrfs_assert_spinning_writers_put(struct extent_buffer *eb) { }
 static void btrfs_assert_no_spinning_writers(struct extent_buffer *eb) { }
+static void btrfs_assert_spinning_readers_put(struct extent_buffer *eb) { }
+static void btrfs_assert_spinning_readers_get(struct extent_buffer *eb) { }
 #endif
 
 void btrfs_set_lock_blocking_read(struct extent_buffer *eb)
-- 
2.20.1

[PATCH 4/9] btrfs: use assertion helpers for spinning readers

[David Sterba]

Use the helpers where open coded. On non-debug builds, the warnings will
not trigger and extent_buffer::spining_readers become unused and can be
moved to the appropriate section, saving a few bytes.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/extent_io.c |  2 +-
 fs/btrfs/extent_io.h |  2 +-
 fs/btrfs/locking.c   | 12 +++++-------
 3 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 11a95919bb3e..30363cb97625 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4672,7 +4672,6 @@ __alloc_extent_buffer(struct btrfs_fs_info *fs_info, u64 start,
 	atomic_set(&eb->read_locks, 0);
 	atomic_set(&eb->blocking_readers, 0);
 	atomic_set(&eb->blocking_writers, 0);
-	atomic_set(&eb->spinning_readers, 0);
 	eb->lock_nested = 0;
 	init_waitqueue_head(&eb->write_lock_wq);
 	init_waitqueue_head(&eb->read_lock_wq);
@@ -4692,6 +4691,7 @@ __alloc_extent_buffer(struct btrfs_fs_info *fs_info, u64 start,
 
 #ifdef CONFIG_BTRFS_DEBUG
 	atomic_set(&eb->spinning_writers, 0);
+	atomic_set(&eb->spinning_readers, 0);
 #endif
 
 	return eb;
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index c5eb261c1ea9..e49310d2caf4 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -151,7 +151,6 @@ struct extent_buffer {
 	atomic_t read_locks;
 	atomic_t blocking_writers;
 	atomic_t blocking_readers;
-	atomic_t spinning_readers;
 	short lock_nested;
 	/* >= 0 if eb belongs to a log tree, -1 otherwise */
 	short log_index;
@@ -171,6 +170,7 @@ struct extent_buffer {
 	struct page *pages[INLINE_EXTENT_BUFFER_PAGES];
 #ifdef CONFIG_BTRFS_DEBUG
 	atomic_t spinning_writers;
+	atomic_t spinning_readers;
 	struct list_head leak_list;
 #endif
 };
diff --git a/fs/btrfs/locking.c b/fs/btrfs/locking.c
index 2dd3ae524aa3..ce9dc985ce05 100644
--- a/fs/btrfs/locking.c
+++ b/fs/btrfs/locking.c
@@ -62,8 +62,7 @@ void btrfs_set_lock_blocking_read(struct extent_buffer *eb)
 		return;
 	btrfs_assert_tree_read_locked(eb);
 	atomic_inc(&eb->blocking_readers);
-	WARN_ON(atomic_read(&eb->spinning_readers) == 0);
-	atomic_dec(&eb->spinning_readers);
+	btrfs_assert_spinning_readers_put(eb);
 	read_unlock(&eb->lock);
 }
 
@@ -150,7 +149,7 @@ void btrfs_tree_read_lock(struct extent_buffer *eb)
 		goto again;
 	}
 	atomic_inc(&eb->read_locks);
-	atomic_inc(&eb->spinning_readers);
+	btrfs_assert_spinning_readers_get(eb);
 }
 
 /*
@@ -169,7 +168,7 @@ int btrfs_tree_read_lock_atomic(struct extent_buffer *eb)
 		return 0;
 	}
 	atomic_inc(&eb->read_locks);
-	atomic_inc(&eb->spinning_readers);
+	btrfs_assert_spinning_readers_get(eb);
 	return 1;
 }
 
@@ -190,7 +189,7 @@ int btrfs_try_tree_read_lock(struct extent_buffer *eb)
 		return 0;
 	}
 	atomic_inc(&eb->read_locks);
-	atomic_inc(&eb->spinning_readers);
+	btrfs_assert_spinning_readers_get(eb);
 	return 1;
 }
 
@@ -232,8 +231,7 @@ void btrfs_tree_read_unlock(struct extent_buffer *eb)
 		return;
 	}
 	btrfs_assert_tree_read_locked(eb);
-	WARN_ON(atomic_read(&eb->spinning_readers) == 0);
-	atomic_dec(&eb->spinning_readers);
+	btrfs_assert_spinning_readers_put(eb);
 	atomic_dec(&eb->read_locks);
 	read_unlock(&eb->lock);
 }
-- 
2.20.1

[PATCH 5/9] btrfs: add assertion helpers for extent buffer read lock counters

[David Sterba]

The read_locks are a simple counter to track locking balance and used to
assert tree locks.  Add helpers to make it conditionally work only in
DEBUG builds.  Will be used in followup patches.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/locking.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/locking.c b/fs/btrfs/locking.c
index ce9dc985ce05..947c66c01e34 100644
--- a/fs/btrfs/locking.c
+++ b/fs/btrfs/locking.c
@@ -12,8 +12,6 @@
 #include "extent_io.h"
 #include "locking.h"
 
-static void btrfs_assert_tree_read_locked(struct extent_buffer *eb);
-
 #ifdef CONFIG_BTRFS_DEBUG
 static void btrfs_assert_spinning_writers_get(struct extent_buffer *eb)
 {
@@ -43,12 +41,30 @@ static void btrfs_assert_spinning_readers_put(struct extent_buffer *eb)
 	atomic_dec(&eb->spinning_readers);
 }
 
+static void btrfs_assert_tree_read_locks_get(struct extent_buffer *eb)
+{
+	atomic_inc(&eb->read_locks);
+}
+
+static void btrfs_assert_tree_read_locks_put(struct extent_buffer *eb)
+{
+	atomic_dec(&eb->read_locks);
+}
+
+static void btrfs_assert_tree_read_locked(struct extent_buffer *eb)
+{
+	BUG_ON(!atomic_read(&eb->read_locks));
+}
+
 #else
 static void btrfs_assert_spinning_writers_get(struct extent_buffer *eb) { }
 static void btrfs_assert_spinning_writers_put(struct extent_buffer *eb) { }
 static void btrfs_assert_no_spinning_writers(struct extent_buffer *eb) { }
 static void btrfs_assert_spinning_readers_put(struct extent_buffer *eb) { }
 static void btrfs_assert_spinning_readers_get(struct extent_buffer *eb) { }
+static void btrfs_assert_tree_read_locked(struct extent_buffer *eb) { }
+static void btrfs_assert_tree_read_locks_get(struct extent_buffer *eb) { }
+static void btrfs_assert_tree_read_locks_put(struct extent_buffer *eb) { }
 #endif
 
 void btrfs_set_lock_blocking_read(struct extent_buffer *eb)
@@ -309,8 +325,3 @@ void btrfs_assert_tree_locked(struct extent_buffer *eb)
 {
 	BUG_ON(!atomic_read(&eb->write_locks));
 }
-
-static void btrfs_assert_tree_read_locked(struct extent_buffer *eb)
-{
-	BUG_ON(!atomic_read(&eb->read_locks));
-}
-- 
2.20.1

[PATCH 6/9] btrfs: use assertion helpers for extent buffer read lock counters

[David Sterba]

Use the helpers where open coded. On non-debug builds, the warnings will
not trigger and extent_buffer::read_locks become unused and can be
moved to the appropriate section, saving a few bytes.

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 +++++-----
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 30363cb97625..a74495bdb552 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4669,7 +4669,6 @@ __alloc_extent_buffer(struct btrfs_fs_info *fs_info, u64 start,
 	eb->bflags = 0;
 	rwlock_init(&eb->lock);
 	atomic_set(&eb->write_locks, 0);
-	atomic_set(&eb->read_locks, 0);
 	atomic_set(&eb->blocking_readers, 0);
 	atomic_set(&eb->blocking_writers, 0);
 	eb->lock_nested = 0;
@@ -4692,6 +4691,7 @@ __alloc_extent_buffer(struct btrfs_fs_info *fs_info, u64 start,
 #ifdef CONFIG_BTRFS_DEBUG
 	atomic_set(&eb->spinning_writers, 0);
 	atomic_set(&eb->spinning_readers, 0);
+	atomic_set(&eb->read_locks, 0);
 #endif
 
 	return eb;
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index e49310d2caf4..fee8c8c463b8 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -148,7 +148,6 @@ struct extent_buffer {
 
 	/* count of read lock holders on the extent buffer */
 	atomic_t write_locks;
-	atomic_t read_locks;
 	atomic_t blocking_writers;
 	atomic_t blocking_readers;
 	short lock_nested;
@@ -171,6 +170,7 @@ struct extent_buffer {
 #ifdef CONFIG_BTRFS_DEBUG
 	atomic_t spinning_writers;
 	atomic_t spinning_readers;
+	atomic_t read_locks;
 	struct list_head leak_list;
 #endif
 };
diff --git a/fs/btrfs/locking.c b/fs/btrfs/locking.c
index 947c66c01e34..e88a0a3d286f 100644
--- a/fs/btrfs/locking.c
+++ b/fs/btrfs/locking.c
@@ -164,7 +164,7 @@ void btrfs_tree_read_lock(struct extent_buffer *eb)
 			   atomic_read(&eb->blocking_writers) == 0);
 		goto again;
 	}
-	atomic_inc(&eb->read_locks);
+	btrfs_assert_tree_read_locks_get(eb);
 	btrfs_assert_spinning_readers_get(eb);
 }
 
@@ -183,7 +183,7 @@ int btrfs_tree_read_lock_atomic(struct extent_buffer *eb)
 		read_unlock(&eb->lock);
 		return 0;
 	}
-	atomic_inc(&eb->read_locks);
+	btrfs_assert_tree_read_locks_get(eb);
 	btrfs_assert_spinning_readers_get(eb);
 	return 1;
 }
@@ -204,7 +204,7 @@ int btrfs_try_tree_read_lock(struct extent_buffer *eb)
 		read_unlock(&eb->lock);
 		return 0;
 	}
-	atomic_inc(&eb->read_locks);
+	btrfs_assert_tree_read_locks_get(eb);
 	btrfs_assert_spinning_readers_get(eb);
 	return 1;
 }
@@ -248,7 +248,7 @@ void btrfs_tree_read_unlock(struct extent_buffer *eb)
 	}
 	btrfs_assert_tree_read_locked(eb);
 	btrfs_assert_spinning_readers_put(eb);
-	atomic_dec(&eb->read_locks);
+	btrfs_assert_tree_read_locks_put(eb);
 	read_unlock(&eb->lock);
 }
 
@@ -272,7 +272,7 @@ void btrfs_tree_read_unlock_blocking(struct extent_buffer *eb)
 	/* 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);
+	btrfs_assert_tree_read_locks_put(eb);
 }
 
 /*
-- 
2.20.1

[PATCH 7/9] btrfs: add assertion helpers for extent buffer write lock counters

[David Sterba]

The write_locks are a simple counter to track locking balance and used
to assert tree locks.  Add helpers to make it conditionally work only in
DEBUG builds.  Will be used in followup patches.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/locking.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/locking.c b/fs/btrfs/locking.c
index e88a0a3d286f..ebde1d943e55 100644
--- a/fs/btrfs/locking.c
+++ b/fs/btrfs/locking.c
@@ -56,6 +56,21 @@ static void btrfs_assert_tree_read_locked(struct extent_buffer *eb)
 	BUG_ON(!atomic_read(&eb->read_locks));
 }
 
+static void btrfs_assert_tree_write_locks_get(struct extent_buffer *eb)
+{
+	atomic_inc(&eb->write_locks);
+}
+
+static void btrfs_assert_tree_write_locks_put(struct extent_buffer *eb)
+{
+	atomic_dec(&eb->write_locks);
+}
+
+void btrfs_assert_tree_locked(struct extent_buffer *eb)
+{
+	BUG_ON(!atomic_read(&eb->write_locks));
+}
+
 #else
 static void btrfs_assert_spinning_writers_get(struct extent_buffer *eb) { }
 static void btrfs_assert_spinning_writers_put(struct extent_buffer *eb) { }
@@ -65,6 +80,9 @@ static void btrfs_assert_spinning_readers_get(struct extent_buffer *eb) { }
 static void btrfs_assert_tree_read_locked(struct extent_buffer *eb) { }
 static void btrfs_assert_tree_read_locks_get(struct extent_buffer *eb) { }
 static void btrfs_assert_tree_read_locks_put(struct extent_buffer *eb) { }
+void btrfs_assert_tree_locked(struct extent_buffer *eb) { }
+static void btrfs_assert_tree_write_locks_get(struct extent_buffer *eb) { }
+static void btrfs_assert_tree_write_locks_put(struct extent_buffer *eb) { }
 #endif
 
 void btrfs_set_lock_blocking_read(struct extent_buffer *eb)
@@ -320,8 +338,3 @@ void btrfs_tree_unlock(struct extent_buffer *eb)
 		write_unlock(&eb->lock);
 	}
 }
-
-void btrfs_assert_tree_locked(struct extent_buffer *eb)
-{
-	BUG_ON(!atomic_read(&eb->write_locks));
-}
-- 
2.20.1

[PATCH 8/9] btrfs: use assertion helpers for extent buffer write lock counters

[David Sterba]

Use the helpers where open coded. On non-debug builds, the warnings will
not trigger and extent_buffer::write_locks become unused and can be
moved to the appropriate section, saving a few bytes.

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

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index a74495bdb552..a9ae476c7502 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4668,7 +4668,6 @@ __alloc_extent_buffer(struct btrfs_fs_info *fs_info, u64 start,
 	eb->fs_info = fs_info;
 	eb->bflags = 0;
 	rwlock_init(&eb->lock);
-	atomic_set(&eb->write_locks, 0);
 	atomic_set(&eb->blocking_readers, 0);
 	atomic_set(&eb->blocking_writers, 0);
 	eb->lock_nested = 0;
@@ -4692,6 +4691,7 @@ __alloc_extent_buffer(struct btrfs_fs_info *fs_info, u64 start,
 	atomic_set(&eb->spinning_writers, 0);
 	atomic_set(&eb->spinning_readers, 0);
 	atomic_set(&eb->read_locks, 0);
+	atomic_set(&eb->write_locks, 0);
 #endif
 
 	return eb;
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index fee8c8c463b8..b2640db8821e 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -146,8 +146,6 @@ struct extent_buffer {
 	struct rcu_head rcu_head;
 	pid_t lock_owner;
 
-	/* count of read lock holders on the extent buffer */
-	atomic_t write_locks;
 	atomic_t blocking_writers;
 	atomic_t blocking_readers;
 	short lock_nested;
@@ -171,6 +169,7 @@ struct extent_buffer {
 	atomic_t spinning_writers;
 	atomic_t spinning_readers;
 	atomic_t read_locks;
+	atomic_t write_locks;
 	struct list_head leak_list;
 #endif
 };
diff --git a/fs/btrfs/locking.c b/fs/btrfs/locking.c
index ebde1d943e55..e595e6541770 100644
--- a/fs/btrfs/locking.c
+++ b/fs/btrfs/locking.c
@@ -243,7 +243,7 @@ int btrfs_try_tree_write_lock(struct extent_buffer *eb)
 		write_unlock(&eb->lock);
 		return 0;
 	}
-	atomic_inc(&eb->write_locks);
+	btrfs_assert_tree_write_locks_get(eb);
 	btrfs_assert_spinning_writers_get(eb);
 	eb->lock_owner = current->pid;
 	return 1;
@@ -310,7 +310,7 @@ void btrfs_tree_lock(struct extent_buffer *eb)
 		goto again;
 	}
 	btrfs_assert_spinning_writers_get(eb);
-	atomic_inc(&eb->write_locks);
+	btrfs_assert_tree_write_locks_get(eb);
 	eb->lock_owner = current->pid;
 }
 
@@ -325,7 +325,7 @@ void btrfs_tree_unlock(struct extent_buffer *eb)
 
 	btrfs_assert_tree_locked(eb);
 	eb->lock_owner = 0;
-	atomic_dec(&eb->write_locks);
+	btrfs_assert_tree_write_locks_put(eb);
 
 	if (blockers) {
 		btrfs_assert_no_spinning_writers(eb);
-- 
2.20.1

[PATCH 9/9] btrfs: switch extent_buffer::lock_nested to bool

[David Sterba]

The member is tracking simple status of the lock, we can use bool for
that and make some room for further space reduction in the structure.

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 +++---
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index a9ae476c7502..388466b8cde9 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4670,7 +4670,7 @@ __alloc_extent_buffer(struct btrfs_fs_info *fs_info, u64 start,
 	rwlock_init(&eb->lock);
 	atomic_set(&eb->blocking_readers, 0);
 	atomic_set(&eb->blocking_writers, 0);
-	eb->lock_nested = 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 b2640db8821e..0f2aa7e5adc1 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -148,7 +148,7 @@ struct extent_buffer {
 
 	atomic_t blocking_writers;
 	atomic_t blocking_readers;
-	short lock_nested;
+	bool lock_nested;
 	/* >= 0 if eb belongs to a log tree, -1 otherwise */
 	short log_index;
 
diff --git a/fs/btrfs/locking.c b/fs/btrfs/locking.c
index e595e6541770..1b65b7ba9df8 100644
--- a/fs/btrfs/locking.c
+++ b/fs/btrfs/locking.c
@@ -172,7 +172,7 @@ void btrfs_tree_read_lock(struct extent_buffer *eb)
 		 * called on a partly (write-)locked tree.
 		 */
 		BUG_ON(eb->lock_nested);
-		eb->lock_nested = 1;
+		eb->lock_nested = true;
 		read_unlock(&eb->lock);
 		return;
 	}
@@ -261,7 +261,7 @@ void btrfs_tree_read_unlock(struct extent_buffer *eb)
 	 * field only matters to the lock owner.
 	 */
 	if (eb->lock_nested && current->pid == eb->lock_owner) {
-		eb->lock_nested = 0;
+		eb->lock_nested = false;
 		return;
 	}
 	btrfs_assert_tree_read_locked(eb);
@@ -282,7 +282,7 @@ void btrfs_tree_read_unlock_blocking(struct extent_buffer *eb)
 	 * field only matters to the lock owner.
 	 */
 	if (eb->lock_nested && current->pid == eb->lock_owner) {
-		eb->lock_nested = 0;
+		eb->lock_nested = false;
 		return;
 	}
 	btrfs_assert_tree_read_locked(eb);
-- 
2.20.1

Re: [PATCH 0/9] Extent buffer locking cleanups

[Nikolay Borisov]

On 13.03.19 г. 17:46 ч., David Sterba wrote:
> The series moves several atomic counters under CONFIG_BTRFS_DEBUG. The
> selected counters are not essential for the extent buffer locking to
> work. There's some space saving (4x 4B at least) and the cachelines are
> less stressed on non-debugging builds.
> 
> The final size is 264 from 280, getting to 256 would be nice but hard or
> making the code unreadable. I have ideas to shave 7 more bytes but
> that's for another patchset.
> 
> David Sterba (9):
>   btrfs: add assertion helpers for spinning writers
>   btrfs: use assertion helpers for spinning writers
>   btrfs: add assertion helpers for spinning readers
>   btrfs: use assertion helpers for spinning readers
>   btrfs: add assertion helpers for extent buffer read lock counters
>   btrfs: use assertion helpers for extent buffer read lock counters
>   btrfs: add assertion helpers for extent buffer write lock counters
>   btrfs: use assertion helpers for extent buffer write lock counters
>   btrfs: switch extent_buffer::lock_nested to bool
> 
>  fs/btrfs/extent_io.c |  13 +++--
>  fs/btrfs/extent_io.h |  11 ++--
>  fs/btrfs/locking.c   | 133 ++++++++++++++++++++++++++++++-------------
>  3 files changed, 107 insertions(+), 50 deletions(-)

Overall the series looks good, one thing I wonder is wouldn't it be
better if you convert those WARN_ON to assert so that we fail fast in
case the locking invariant are broken?

In any case:

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


> 

Re: [PATCH 0/9] Extent buffer locking cleanups

[Johannes Thumshirn]

For the whole series:
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>

-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH 4/9] btrfs: use assertion helpers for spinning readers

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2731 bytes --]

Hi David,

I love your patch! Yet something to improve:

[auto build test ERROR on kdave/for-next]
[also build test ERROR on next-20190306]
[cannot apply to v5.0]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/David-Sterba/Extent-buffer-locking-cleanups/20190315-231346
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: i386-randconfig-x011-201910 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   fs/btrfs/locking.c: In function 'btrfs_clear_lock_blocking_read':
>> fs/btrfs/locking.c:97:18: error: 'struct extent_buffer' has no member named 'spinning_readers'; did you mean 'blocking_readers'?
     atomic_inc(&eb->spinning_readers);
                     ^~~~~~~~~~~~~~~~
                     blocking_readers

vim +97 fs/btrfs/locking.c

b4ce94de Chris Mason  2009-02-04   85  
aa12c027 David Sterba 2018-04-04   86  void btrfs_clear_lock_blocking_read(struct extent_buffer *eb)
aa12c027 David Sterba 2018-04-04   87  {
b4ce94de Chris Mason  2009-02-04   88  	/*
aa12c027 David Sterba 2018-04-04   89  	 * No lock is required.  The lock owner may change if we have a read
aa12c027 David Sterba 2018-04-04   90  	 * lock, but it won't change to or away from us.  If we have the write
aa12c027 David Sterba 2018-04-04   91  	 * lock, we are the owner and it'll never change.
b4ce94de Chris Mason  2009-02-04   92  	 */
aa12c027 David Sterba 2018-04-04   93  	if (eb->lock_nested && current->pid == eb->lock_owner)
aa12c027 David Sterba 2018-04-04   94  		return;
aa12c027 David Sterba 2018-04-04   95  	BUG_ON(atomic_read(&eb->blocking_readers) == 0);
aa12c027 David Sterba 2018-04-04   96  	read_lock(&eb->lock);
aa12c027 David Sterba 2018-04-04  @97  	atomic_inc(&eb->spinning_readers);
aa12c027 David Sterba 2018-04-04   98  	/* atomic_dec_and_test implies a barrier */
aa12c027 David Sterba 2018-04-04   99  	if (atomic_dec_and_test(&eb->blocking_readers))
aa12c027 David Sterba 2018-04-04  100  		cond_wake_up_nomb(&eb->read_lock_wq);
aa12c027 David Sterba 2018-04-04  101  }
aa12c027 David Sterba 2018-04-04  102  

:::::: The code at line 97 was first introduced by commit
:::::: aa12c02778a9719283fc3c32cfe5cffb902a7685 btrfs: split btrfs_clear_lock_blocking_rw to read and write helpers

:::::: TO: David Sterba <dsterba@suse.com>
:::::: CC: David Sterba <dsterba@suse.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 34008 bytes --]

Re: [PATCH 4/9] btrfs: use assertion helpers for spinning readers

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2654 bytes --]

Hi David,

I love your patch! Yet something to improve:

[auto build test ERROR on kdave/for-next]
[also build test ERROR on next-20190306]
[cannot apply to v5.0]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/David-Sterba/Extent-buffer-locking-cleanups/20190315-231346
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: i386-randconfig-l1-03152056 (attached as .config)
compiler: gcc-5 (Debian 5.5.0-3) 5.4.1 20171010
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   fs//btrfs/locking.c: In function 'btrfs_clear_lock_blocking_read':
>> fs//btrfs/locking.c:97:16: error: 'struct extent_buffer' has no member named 'spinning_readers'
     atomic_inc(&eb->spinning_readers);
                   ^

vim +97 fs//btrfs/locking.c

b4ce94de Chris Mason  2009-02-04   85  
aa12c027 David Sterba 2018-04-04   86  void btrfs_clear_lock_blocking_read(struct extent_buffer *eb)
aa12c027 David Sterba 2018-04-04   87  {
b4ce94de Chris Mason  2009-02-04   88  	/*
aa12c027 David Sterba 2018-04-04   89  	 * No lock is required.  The lock owner may change if we have a read
aa12c027 David Sterba 2018-04-04   90  	 * lock, but it won't change to or away from us.  If we have the write
aa12c027 David Sterba 2018-04-04   91  	 * lock, we are the owner and it'll never change.
b4ce94de Chris Mason  2009-02-04   92  	 */
aa12c027 David Sterba 2018-04-04   93  	if (eb->lock_nested && current->pid == eb->lock_owner)
aa12c027 David Sterba 2018-04-04   94  		return;
aa12c027 David Sterba 2018-04-04   95  	BUG_ON(atomic_read(&eb->blocking_readers) == 0);
aa12c027 David Sterba 2018-04-04   96  	read_lock(&eb->lock);
aa12c027 David Sterba 2018-04-04  @97  	atomic_inc(&eb->spinning_readers);
aa12c027 David Sterba 2018-04-04   98  	/* atomic_dec_and_test implies a barrier */
aa12c027 David Sterba 2018-04-04   99  	if (atomic_dec_and_test(&eb->blocking_readers))
aa12c027 David Sterba 2018-04-04  100  		cond_wake_up_nomb(&eb->read_lock_wq);
aa12c027 David Sterba 2018-04-04  101  }
aa12c027 David Sterba 2018-04-04  102  

:::::: The code at line 97 was first introduced by commit
:::::: aa12c02778a9719283fc3c32cfe5cffb902a7685 btrfs: split btrfs_clear_lock_blocking_rw to read and write helpers

:::::: TO: David Sterba <dsterba@suse.com>
:::::: CC: David Sterba <dsterba@suse.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 38417 bytes --]

Re: [PATCH 4/9] btrfs: use assertion helpers for spinning readers

[David Sterba]

On Sat, Mar 16, 2019 at 12:18:30AM +0800, kbuild test robot wrote:
> Hi David,
> 
> I love your patch! Yet something to improve:
> 
> [auto build test ERROR on kdave/for-next]
> [also build test ERROR on next-20190306]
> [cannot apply to v5.0]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/David-Sterba/Extent-buffer-locking-cleanups/20190315-231346
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
> config: i386-randconfig-x011-201910 (attached as .config)
> compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=i386 
> 
> All errors (new ones prefixed by >>):
> 
>    fs/btrfs/locking.c: In function 'btrfs_clear_lock_blocking_read':
> >> fs/btrfs/locking.c:97:18: error: 'struct extent_buffer' has no member named 'spinning_readers'; did you mean 'blocking_readers'?
>      atomic_inc(&eb->spinning_readers);
>                      ^~~~~~~~~~~~~~~~
>                      blocking_readers
> 
> vim +97 fs/btrfs/locking.c
> 
> b4ce94de Chris Mason  2009-02-04   85  
> aa12c027 David Sterba 2018-04-04   86  void btrfs_clear_lock_blocking_read(struct extent_buffer *eb)
> aa12c027 David Sterba 2018-04-04   87  {
> b4ce94de Chris Mason  2009-02-04   88  	/*
> aa12c027 David Sterba 2018-04-04   89  	 * No lock is required.  The lock owner may change if we have a read
> aa12c027 David Sterba 2018-04-04   90  	 * lock, but it won't change to or away from us.  If we have the write
> aa12c027 David Sterba 2018-04-04   91  	 * lock, we are the owner and it'll never change.
> b4ce94de Chris Mason  2009-02-04   92  	 */
> aa12c027 David Sterba 2018-04-04   93  	if (eb->lock_nested && current->pid == eb->lock_owner)
> aa12c027 David Sterba 2018-04-04   94  		return;
> aa12c027 David Sterba 2018-04-04   95  	BUG_ON(atomic_read(&eb->blocking_readers) == 0);
> aa12c027 David Sterba 2018-04-04   96  	read_lock(&eb->lock);
> aa12c027 David Sterba 2018-04-04  @97  	atomic_inc(&eb->spinning_readers);

Thanks for the report.  There's the right code in my branch that
compiles (and also got tested), with and without the config option, so I
must have sent branch with the uncommited fixup.

Re: [PATCH 0/9] Extent buffer locking cleanups

[David Sterba]

On Thu, Mar 14, 2019 at 09:26:37AM +0200, Nikolay Borisov wrote:
> 
> 
> On 13.03.19 г. 17:46 ч., David Sterba wrote:
> > The series moves several atomic counters under CONFIG_BTRFS_DEBUG. The
> > selected counters are not essential for the extent buffer locking to
> > work. There's some space saving (4x 4B at least) and the cachelines are
> > less stressed on non-debugging builds.
> > 
> > The final size is 264 from 280, getting to 256 would be nice but hard or
> > making the code unreadable. I have ideas to shave 7 more bytes but
> > that's for another patchset.
> > 
> > David Sterba (9):
> >   btrfs: add assertion helpers for spinning writers
> >   btrfs: use assertion helpers for spinning writers
> >   btrfs: add assertion helpers for spinning readers
> >   btrfs: use assertion helpers for spinning readers
> >   btrfs: add assertion helpers for extent buffer read lock counters
> >   btrfs: use assertion helpers for extent buffer read lock counters
> >   btrfs: add assertion helpers for extent buffer write lock counters
> >   btrfs: use assertion helpers for extent buffer write lock counters
> >   btrfs: switch extent_buffer::lock_nested to bool
> > 
> >  fs/btrfs/extent_io.c |  13 +++--
> >  fs/btrfs/extent_io.h |  11 ++--
> >  fs/btrfs/locking.c   | 133 ++++++++++++++++++++++++++++++-------------
> >  3 files changed, 107 insertions(+), 50 deletions(-)
> 
> Overall the series looks good, one thing I wonder is wouldn't it be
> better if you convert those WARN_ON to assert so that we fail fast in
> case the locking invariant are broken?

This probably makes sense, the patches copy the existing code without
other updates. As it is entirely under debug ifdef, BUG_ON would work
too so it's not dependent on asserts compiled in or not.

Re: [PATCH 0/9] Extent buffer locking cleanups

[Nikolay Borisov]

On 18.03.19 г. 21:29 ч., David Sterba wrote:
> On Thu, Mar 14, 2019 at 09:26:37AM +0200, Nikolay Borisov wrote:
>>
>>
>> On 13.03.19 г. 17:46 ч., David Sterba wrote:
>>> The series moves several atomic counters under CONFIG_BTRFS_DEBUG. The
>>> selected counters are not essential for the extent buffer locking to
>>> work. There's some space saving (4x 4B at least) and the cachelines are
>>> less stressed on non-debugging builds.
>>>
>>> The final size is 264 from 280, getting to 256 would be nice but hard or
>>> making the code unreadable. I have ideas to shave 7 more bytes but
>>> that's for another patchset.
>>>
>>> David Sterba (9):
>>>   btrfs: add assertion helpers for spinning writers
>>>   btrfs: use assertion helpers for spinning writers
>>>   btrfs: add assertion helpers for spinning readers
>>>   btrfs: use assertion helpers for spinning readers
>>>   btrfs: add assertion helpers for extent buffer read lock counters
>>>   btrfs: use assertion helpers for extent buffer read lock counters
>>>   btrfs: add assertion helpers for extent buffer write lock counters
>>>   btrfs: use assertion helpers for extent buffer write lock counters
>>>   btrfs: switch extent_buffer::lock_nested to bool
>>>
>>>  fs/btrfs/extent_io.c |  13 +++--
>>>  fs/btrfs/extent_io.h |  11 ++--
>>>  fs/btrfs/locking.c   | 133 ++++++++++++++++++++++++++++++-------------
>>>  3 files changed, 107 insertions(+), 50 deletions(-)
>>
>> Overall the series looks good, one thing I wonder is wouldn't it be
>> better if you convert those WARN_ON to assert so that we fail fast in
>> case the locking invariant are broken?
> 
> This probably makes sense, the patches copy the existing code without
> other updates. As it is entirely under debug ifdef, BUG_ON would work
> too so it's not dependent on asserts compiled in or not.

I'm fine with turning it into a BUG_ON as well.

>