Re: [PATCH 2/5] btrfs: set blocking_writers directly, no increment or decrement

[Nikolay Borisov <nborisov@suse.com>]

On 17.10.19 г. 22:38 ч., David Sterba wrote:
> The increment and decrement was inherited from previous version that
> used atomics, switched in commit 06297d8cefca ("btrfs: switch
> extent_buffer blocking_writers from atomic to int"). The only possible
> values are 0 and 1 so we can set them directly.
> 
> The generated assembly (gcc 9.x) did the direct value assignment in
> btrfs_set_lock_blocking_write:
> 
>      5d:   test   %eax,%eax
>      5f:   je     62 <btrfs_set_lock_blocking_write+0x22>
>      61:   retq
> 
>   -  62:   lock incl 0x44(%rdi)
>   -  66:   add    $0x50,%rdi
>   -  6a:   jmpq   6f <btrfs_set_lock_blocking_write+0x2f>
> 
>   +  62:   movl   $0x1,0x44(%rdi)
>   +  69:   add    $0x50,%rdi
>   +  6d:   jmpq   72 <btrfs_set_lock_blocking_write+0x32>
> 
> The part in btrfs_tree_unlock did a decrement because
> BUG_ON(blockers > 1) is probably not a strong hint for the compiler, but
> otherwise the output looks safe:
> 
>   - lock decl 0x44(%rdi)
> 
>   + sub    $0x1,%eax
>   + mov    %eax,0x44(%rdi)

I find it strange that plain inc/decs had the lock prefix in the
resulting assembly. The sub instruction can have the lock prefix but
here the compiler chooses not to. While this doesn't mean it's buggy I'm
just curious what's happening.

> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/locking.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/locking.c b/fs/btrfs/locking.c
> index c84c650e56c7..00edf91c3d1c 100644
> --- a/fs/btrfs/locking.c
> +++ b/fs/btrfs/locking.c
> @@ -109,7 +109,7 @@ void btrfs_set_lock_blocking_write(struct extent_buffer *eb)
>  	if (eb->blocking_writers == 0) {
>  		btrfs_assert_spinning_writers_put(eb);
>  		btrfs_assert_tree_locked(eb);
> -		eb->blocking_writers++;
> +		eb->blocking_writers = 1;
>  		write_unlock(&eb->lock);
>  	}
>  }
> @@ -305,7 +305,7 @@ void btrfs_tree_unlock(struct extent_buffer *eb)
>  
>  	if (blockers) {
>  		btrfs_assert_no_spinning_writers(eb);
> -		eb->blocking_writers--;
> +		eb->blocking_writers = 0;
>  		/*
>  		 * We need to order modifying blocking_writers above with
>  		 * actually waking up the sleepers to ensure they see the
>