* [PATCH 1/9] btrfs: add assertion helpers for spinning writers
2019-03-13 15:46 [PATCH 0/9] Extent buffer locking cleanups David Sterba
@ 2019-03-13 15:47 ` David Sterba
2019-03-13 15:47 ` [PATCH 2/9] btrfs: use " David Sterba
` (9 subsequent siblings)
10 siblings, 0 replies; 17+ messages in thread
From: David Sterba @ 2019-03-13 15:47 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 2/9] btrfs: use assertion helpers for spinning writers
2019-03-13 15:46 [PATCH 0/9] Extent buffer locking cleanups David Sterba
2019-03-13 15:47 ` [PATCH 1/9] btrfs: add assertion helpers for spinning writers David Sterba
@ 2019-03-13 15:47 ` David Sterba
2019-03-13 15:47 ` [PATCH 3/9] btrfs: add assertion helpers for spinning readers David Sterba
` (8 subsequent siblings)
10 siblings, 0 replies; 17+ messages in thread
From: David Sterba @ 2019-03-13 15:47 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 3/9] btrfs: add assertion helpers for spinning readers
2019-03-13 15:46 [PATCH 0/9] Extent buffer locking cleanups David Sterba
2019-03-13 15:47 ` [PATCH 1/9] btrfs: add assertion helpers for spinning writers David Sterba
2019-03-13 15:47 ` [PATCH 2/9] btrfs: use " David Sterba
@ 2019-03-13 15:47 ` David Sterba
2019-03-13 15:47 ` [PATCH 4/9] btrfs: use " David Sterba
` (7 subsequent siblings)
10 siblings, 0 replies; 17+ messages in thread
From: David Sterba @ 2019-03-13 15:47 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 4/9] btrfs: use assertion helpers for spinning readers
2019-03-13 15:46 [PATCH 0/9] Extent buffer locking cleanups David Sterba
` (2 preceding siblings ...)
2019-03-13 15:47 ` [PATCH 3/9] btrfs: add assertion helpers for spinning readers David Sterba
@ 2019-03-13 15:47 ` David Sterba
2019-03-15 16:18 ` kbuild test robot
2019-03-15 17:13 ` kbuild test robot
2019-03-13 15:47 ` [PATCH 5/9] btrfs: add assertion helpers for extent buffer read lock counters David Sterba
` (6 subsequent siblings)
10 siblings, 2 replies; 17+ messages in thread
From: David Sterba @ 2019-03-13 15:47 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 4/9] btrfs: use assertion helpers for spinning readers
2019-03-13 15:47 ` [PATCH 4/9] btrfs: use " David Sterba
@ 2019-03-15 16:18 ` kbuild test robot
2019-03-15 21:43 ` David Sterba
2019-03-15 17:13 ` kbuild test robot
1 sibling, 1 reply; 17+ messages in thread
From: kbuild test robot @ 2019-03-15 16:18 UTC (permalink / raw)
To: David Sterba; +Cc: kbuild-all, linux-btrfs, David Sterba
[-- 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 --]
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 4/9] btrfs: use assertion helpers for spinning readers
2019-03-15 16:18 ` kbuild test robot
@ 2019-03-15 21:43 ` David Sterba
0 siblings, 0 replies; 17+ messages in thread
From: David Sterba @ 2019-03-15 21:43 UTC (permalink / raw)
To: kbuild test robot; +Cc: kbuild-all, linux-btrfs
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 4/9] btrfs: use assertion helpers for spinning readers
2019-03-13 15:47 ` [PATCH 4/9] btrfs: use " David Sterba
2019-03-15 16:18 ` kbuild test robot
@ 2019-03-15 17:13 ` kbuild test robot
1 sibling, 0 replies; 17+ messages in thread
From: kbuild test robot @ 2019-03-15 17:13 UTC (permalink / raw)
To: David Sterba; +Cc: kbuild-all, linux-btrfs, David Sterba
[-- 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 --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 5/9] btrfs: add assertion helpers for extent buffer read lock counters
2019-03-13 15:46 [PATCH 0/9] Extent buffer locking cleanups David Sterba
` (3 preceding siblings ...)
2019-03-13 15:47 ` [PATCH 4/9] btrfs: use " David Sterba
@ 2019-03-13 15:47 ` David Sterba
2019-03-13 15:47 ` [PATCH 6/9] btrfs: use " David Sterba
` (5 subsequent siblings)
10 siblings, 0 replies; 17+ messages in thread
From: David Sterba @ 2019-03-13 15:47 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 6/9] btrfs: use assertion helpers for extent buffer read lock counters
2019-03-13 15:46 [PATCH 0/9] Extent buffer locking cleanups David Sterba
` (4 preceding siblings ...)
2019-03-13 15:47 ` [PATCH 5/9] btrfs: add assertion helpers for extent buffer read lock counters David Sterba
@ 2019-03-13 15:47 ` David Sterba
2019-03-13 15:47 ` [PATCH 7/9] btrfs: add assertion helpers for extent buffer write " David Sterba
` (4 subsequent siblings)
10 siblings, 0 replies; 17+ messages in thread
From: David Sterba @ 2019-03-13 15:47 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 7/9] btrfs: add assertion helpers for extent buffer write lock counters
2019-03-13 15:46 [PATCH 0/9] Extent buffer locking cleanups David Sterba
` (5 preceding siblings ...)
2019-03-13 15:47 ` [PATCH 6/9] btrfs: use " David Sterba
@ 2019-03-13 15:47 ` David Sterba
2019-03-13 15:47 ` [PATCH 8/9] btrfs: use " David Sterba
` (3 subsequent siblings)
10 siblings, 0 replies; 17+ messages in thread
From: David Sterba @ 2019-03-13 15:47 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 8/9] btrfs: use assertion helpers for extent buffer write lock counters
2019-03-13 15:46 [PATCH 0/9] Extent buffer locking cleanups David Sterba
` (6 preceding siblings ...)
2019-03-13 15:47 ` [PATCH 7/9] btrfs: add assertion helpers for extent buffer write " David Sterba
@ 2019-03-13 15:47 ` David Sterba
2019-03-13 15:47 ` [PATCH 9/9] btrfs: switch extent_buffer::lock_nested to bool David Sterba
` (2 subsequent siblings)
10 siblings, 0 replies; 17+ messages in thread
From: David Sterba @ 2019-03-13 15:47 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 9/9] btrfs: switch extent_buffer::lock_nested to bool
2019-03-13 15:46 [PATCH 0/9] Extent buffer locking cleanups David Sterba
` (7 preceding siblings ...)
2019-03-13 15:47 ` [PATCH 8/9] btrfs: use " David Sterba
@ 2019-03-13 15:47 ` David Sterba
2019-03-14 7:26 ` [PATCH 0/9] Extent buffer locking cleanups Nikolay Borisov
2019-03-14 13:15 ` Johannes Thumshirn
10 siblings, 0 replies; 17+ messages in thread
From: David Sterba @ 2019-03-13 15:47 UTC (permalink / raw)
To: linux-btrfs; +Cc: 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
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 0/9] Extent buffer locking cleanups
2019-03-13 15:46 [PATCH 0/9] Extent buffer locking cleanups David Sterba
` (8 preceding siblings ...)
2019-03-13 15:47 ` [PATCH 9/9] btrfs: switch extent_buffer::lock_nested to bool David Sterba
@ 2019-03-14 7:26 ` Nikolay Borisov
2019-03-18 19:29 ` David Sterba
2019-03-14 13:15 ` Johannes Thumshirn
10 siblings, 1 reply; 17+ messages in thread
From: Nikolay Borisov @ 2019-03-14 7:26 UTC (permalink / raw)
To: David Sterba, linux-btrfs
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>
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 0/9] Extent buffer locking cleanups
2019-03-14 7:26 ` [PATCH 0/9] Extent buffer locking cleanups Nikolay Borisov
@ 2019-03-18 19:29 ` David Sterba
2019-03-18 19:43 ` Nikolay Borisov
0 siblings, 1 reply; 17+ messages in thread
From: David Sterba @ 2019-03-18 19:29 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: David Sterba, linux-btrfs
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/9] Extent buffer locking cleanups
2019-03-18 19:29 ` David Sterba
@ 2019-03-18 19:43 ` Nikolay Borisov
0 siblings, 0 replies; 17+ messages in thread
From: Nikolay Borisov @ 2019-03-18 19:43 UTC (permalink / raw)
To: dsterba, David Sterba, linux-btrfs
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.
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/9] Extent buffer locking cleanups
2019-03-13 15:46 [PATCH 0/9] Extent buffer locking cleanups David Sterba
` (9 preceding siblings ...)
2019-03-14 7:26 ` [PATCH 0/9] Extent buffer locking cleanups Nikolay Borisov
@ 2019-03-14 13:15 ` Johannes Thumshirn
10 siblings, 0 replies; 17+ messages in thread
From: Johannes Thumshirn @ 2019-03-14 13:15 UTC (permalink / raw)
To: David Sterba, linux-btrfs
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
^ permalink raw reply [flat|nested] 17+ messages in thread