Re: [PATCH RFC] buffer_head: remove redundant test from wait_on_buffer

[Richard Kennedy <richard@rsk.demon.co.uk>]

On Fri, 2010-04-16 at 14:51 -0700, Andrew Morton wrote:

> That debug check got inadvertently crippled during some wait_on_bit()
> conversion.
> 
> It's still a nasty bug to call wait_on_buffer() against a zero-ref
> buffer so perhaps we should fix it up rather than removing its remains.
> 
> diff -puN include/linux/buffer_head.h~buffer_head-remove-redundant-test-from-wait_on_buffer-fix include/linux/buffer_head.h
> --- a/include/linux/buffer_head.h~buffer_head-remove-redundant-test-from-wait_on_buffer-fix
> +++ a/include/linux/buffer_head.h
> @@ -305,10 +305,15 @@ map_bh(struct buffer_head *bh, struct su
>  	bh->b_size = sb->s_blocksize;
>  }
>  
> +/*
> + * Calling wait_on_buffer() for a zero-ref buffer is illegal, so we call into
> + * __wait_on_buffer() just to trip a debug check.  Because debug code in inline
> + * functions is bloaty.
> + */
>  static inline void wait_on_buffer(struct buffer_head *bh)
>  {
>  	might_sleep();
> -	if (buffer_locked(bh))
> +	if (buffer_locked(bh) || atomic_read(&bh->b_count) == 0)
>  		__wait_on_buffer(bh);
>  }
>  
> diff -puN fs/buffer.c~buffer_head-remove-redundant-test-from-wait_on_buffer-fix fs/buffer.c
> --- a/fs/buffer.c~buffer_head-remove-redundant-test-from-wait_on_buffer-fix
> +++ a/fs/buffer.c
> @@ -90,6 +90,12 @@ EXPORT_SYMBOL(unlock_buffer);
>   */
>  void __wait_on_buffer(struct buffer_head * bh)
>  {
> +	/*
> +	 * Calling wait_on_buffer() against a zero-ref buffer is a nasty bug
> +	 * because it will almost always "work".  However this buffer can be
> +	 * reclaimed at any time.  So check for it.
> +	 */
> +	VM_BUG_ON(atomic_read(&bh->b_count) == 0);
>  	wait_on_bit(&bh->b_state, BH_Lock, sync_buffer, TASK_UNINTERRUPTIBLE);
>  }
>  EXPORT_SYMBOL(__wait_on_buffer);
> _
> 
> 
> And while we're there...
> 
> This might make reiserfs explode.
> 
> 
> 
> From: Andrew Morton <akpm@linux-foundation.org>
> 
> The first thing __wait_on_buffer()->wait_on_bit() does is to test that the
> bit was set, so the buffer_locked() test is now redundant.  And once we
> remove that, we can remove the check for zero ->b_count also.
> 
> And now that wait_on_buffer() unconditionally calls __wait_on_buffer(), we
> can move the might_sleep() check into __wait_on_buffer() to save some text.
> 
> The downside of all of this is that wait_on_buffer() against an unlocked
> buffer will now always perform a function call.  Is it a common case?
> 
> We can remove __wait_on_buffer() altogether now.  For some strange reason
> reiserfs calls __wait_on_buffer() directly.  Maybe it's passing in
> zero-ref buffers.  If so, we'll get warnings now and shall need to look at
> that.
> 
> Cc: Jens Axboe <jens.axboe@oracle.com>
> Cc: Nick Piggin <nickpiggin@yahoo.com.au>
> Cc: Richard Kennedy <richard@rsk.demon.co.uk>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  fs/buffer.c                 |    2 ++
>  include/linux/buffer_head.h |    4 +---
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff -puN include/linux/buffer_head.h~wait_on_buffer-remove-the-buffer_locked-test include/linux/buffer_head.h
> --- a/include/linux/buffer_head.h~wait_on_buffer-remove-the-buffer_locked-test
> +++ a/include/linux/buffer_head.h
> @@ -312,9 +312,7 @@ map_bh(struct buffer_head *bh, struct su
>   */
>  static inline void wait_on_buffer(struct buffer_head *bh)
>  {
> -	might_sleep();
> -	if (buffer_locked(bh) || atomic_read(&bh->b_count) == 0)
> -		__wait_on_buffer(bh);
> +	__wait_on_buffer(bh);
>  }
>  
>  static inline int trylock_buffer(struct buffer_head *bh)
> diff -puN fs/buffer.c~wait_on_buffer-remove-the-buffer_locked-test fs/buffer.c
> --- a/fs/buffer.c~wait_on_buffer-remove-the-buffer_locked-test
> +++ a/fs/buffer.c
> @@ -90,6 +90,8 @@ EXPORT_SYMBOL(unlock_buffer);
>   */
>  void __wait_on_buffer(struct buffer_head * bh)
>  {
> +	might_sleep();
> +
>  	/*
>  	 * Calling wait_on_buffer() against a zero-ref buffer is a nasty bug
>  	 * because it will almost always "work".  However this buffer can be
> _
> 
Hi Andrew,
I've tested your patches against 2.6.34-rc4 on lvm/ext4. I'm not seeing
any vm bugs, so it all looks good to me.
thanks
Richard