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

[Nikolay Borisov <nborisov@suse.com>]

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.

>