Re: [PATCH 1/2] fs: buffer: use clear_and_wake_up_bit() in unlock_buffer()

On Wed 20-05-26 16:58:16, Agatha Isabelle Moreira wrote:
> Use `clear_and_wake_up_bit()` in `unlock_buffer()`, since the helper was
> introduced in 'commit 8236b0ae31c83 ("bdi: wake up concurrent
> wb_shutdown() callers.")' as a generic way of doing the same sequence of
> operations:
>        	clear_bit_unlock();
> 	smp_mb__after_atomic();
> 	wake_up_bit();
> 
> The helper was implemented to avoid bugs caused by forgetting to call
> `wake_up_bit()` after `clear_bit_unlock()`.
> 
> Since `unlock_buffer()` predates git and was last modified in
> 'commit 4e857c58efeb9 ("arch: Mass conversion of smp_mb__*()")', years
> before `clear_and_wake_up_bit()`, it still uses the open-coded sequence.
> 
> Replace the open-coded sequence with the helper to avoid duplicate code
> and reduce code paths to maintain.
> 
> Suggested-by: shuo chen <[email protected]>
> Link: https://lore.kernel.org/kernelnewbies/agzoqV835-co4kAN@guidai/T/#t
> Signed-off-by: Agatha Isabelle Moreira <[email protected]>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <[email protected]>

								Honza

> ---
>  fs/buffer.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/fs/buffer.c b/fs/buffer.c
> index b0b3792b1496..4348b240bd97 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -74,9 +74,7 @@ EXPORT_SYMBOL(__lock_buffer);
>  
>  void unlock_buffer(struct buffer_head *bh)
>  {
> -	clear_bit_unlock(BH_Lock, &bh->b_state);
> -	smp_mb__after_atomic();
> -	wake_up_bit(&bh->b_state, BH_Lock);
> +	clear_and_wake_up_bit(BH_Lock, &bh->b_state);
>  }
>  EXPORT_SYMBOL(unlock_buffer);
>  
> -- 
> 2.53.0
> 
-- 
Jan Kara <[email protected]>
SUSE Labs, CR