The comment suggests that when b_count equals zero it is calling
__wait_no_buffer to trigger some debug, but as there is no debug in
__wait_on_buffer the whole thing is redundant.
AFAICT from the git log this has been the case for at least 5 years, so
it seems safe just to remove this.
Signed-off-by: Richard Kennedy <[email protected]>
---
This patch against 2.6.34-rc4
compiled & tested on x86_64
regards
Richard
diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
index 16ed028..4c62dd4 100644
--- a/include/linux/buffer_head.h
+++ b/include/linux/buffer_head.h
@@ -305,15 +305,10 @@ map_bh(struct buffer_head *bh, struct super_block *sb, sector_t block)
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) || atomic_read(&bh->b_count) == 0)
+ if (buffer_locked(bh))
__wait_on_buffer(bh);
}
On Fri, 16 Apr 2010 11:58:19 +0100
Richard Kennedy <[email protected]> wrote:
> The comment suggests that when b_count equals zero it is calling
> __wait_no_buffer to trigger some debug, but as there is no debug in
> __wait_on_buffer the whole thing is redundant.
>
> AFAICT from the git log this has been the case for at least 5 years, so
> it seems safe just to remove this.
>
> Signed-off-by: Richard Kennedy <[email protected]>
> ---
>
> This patch against 2.6.34-rc4
> compiled & tested on x86_64
>
> regards
> Richard
>
>
> diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
> index 16ed028..4c62dd4 100644
> --- a/include/linux/buffer_head.h
> +++ b/include/linux/buffer_head.h
> @@ -305,15 +305,10 @@ map_bh(struct buffer_head *bh, struct super_block *sb, sector_t block)
> 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) || atomic_read(&bh->b_count) == 0)
> + if (buffer_locked(bh))
> __wait_on_buffer(bh);
> }
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 <[email protected]>
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 <[email protected]>
Cc: Nick Piggin <[email protected]>
Cc: Richard Kennedy <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
---
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
_
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 04/16/2010 05:51 PM, Andrew Morton wrote:
> And while we're there...
>
> This might make reiserfs explode.
> 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.
I don't think that's the case. I think reiserfs just calls
__wait_on_buffer just to skip the duplicate buffer_locked() test since
every call is in an "if buffer_locked()" block. I don't think it's
passing in zero-ref buffers anywhere, and I'd prefer it to explode if it is.
- -Jeff
- --
Jeff Mahoney
SUSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.15 (GNU/Linux)
Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org/
iEYEARECAAYFAkvI4lIACgkQLPWxlyuTD7Ju9wCgphZEI8r9jB+75PIxE4l/S/H+
jlEAnR+vo57PB2ZH+PhTSoxWnQ9V74M3
=bQAA
-----END PGP SIGNATURE-----
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 <[email protected]>
>
> 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 <[email protected]>
> Cc: Nick Piggin <[email protected]>
> Cc: Richard Kennedy <[email protected]>
> Signed-off-by: Andrew Morton <[email protected]>
> ---
>
> 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
On Sat, 22 May 2010 23:05:03 -0700
Greg Thelen <[email protected]> wrote:
> > --- 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);
>
> My system is failing this VM_BUG_ON() occasionally. I think this is due to
> wait_on_buffer() calls with b_count=0 from locations within fs/buffer.c.
Thanks.
Yup, the buffers are protected by lock_page().
> These
> occasional b_count=0 callers are caused by buf reads that complete quickly -
> after the I/O is issued but before it is waited upon. Such fs/buffer.c callers
> need to either bypass this assertion or increment b_count. I don't think they
> need to grab an b_count reference. I suggest a bypass routine in the patch
> below. Does this look good?
I think I'll just drop
buffer_head-remove-redundant-test-from-wait_on_buffer-fix.patch and
wait_on_buffer-remove-the-buffer_locked-test.patch.