[PATCH] conditionalize some boring buffer_head checks

[PATCH] conditionalize some boring buffer_head checks

[Jeff Garzik]

[-- Attachment #1: Type: text/plain, Size: 463 bytes --]


These checks are executed billions of times per day, with no stack dump 
bug reports sent to lkml.  Arguably, they will only trigger on buggy 
filesystems (programmer error), and thus IMO shouldn't even be executed 
in a non-debug kernel.

Even though BUG_ON() includes unlikely(), I think this patch -- or 
something like it -- is preferable.  The buffer_error() checks aren't 
even marked unlikely().

This is a micro-optimization on a key kernel fast path.



[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 849 bytes --]

===== fs/buffer.c 1.237 vs edited =====
--- 1.237/fs/buffer.c	Wed Apr 14 03:18:09 2004
+++ edited/fs/buffer.c	Wed Apr 14 03:39:15 2004
@@ -2688,6 +2688,7 @@
 {
 	struct bio *bio;
 
+#ifdef BH_DEBUG
 	BUG_ON(!buffer_locked(bh));
 	BUG_ON(!buffer_mapped(bh));
 	BUG_ON(!bh->b_end_io);
@@ -2698,6 +2699,7 @@
 		buffer_error();
 	if (rw == READ && buffer_dirty(bh))
 		buffer_error();
+#endif
 
 	/* Only clear out a write error when rewriting */
 	if (test_set_buffer_req(bh) && rw == WRITE)
===== include/linux/buffer_head.h 1.47 vs edited =====
--- 1.47/include/linux/buffer_head.h	Wed Apr 14 03:18:09 2004
+++ edited/include/linux/buffer_head.h	Wed Apr 14 03:39:31 2004
@@ -13,6 +13,8 @@
 #include <linux/wait.h>
 #include <asm/atomic.h>
 
+#undef BH_DEBUG
+
 enum bh_state_bits {
 	BH_Uptodate,	/* Contains valid data */
 	BH_Dirty,	/* Is dirty */

Re: [PATCH] conditionalize some boring buffer_head checks

[Andrew Morton]

Jeff Garzik <jgarzik@pobox.com> wrote:
>
> 
> These checks are executed billions of times per day, with no stack dump 
> bug reports sent to lkml.  Arguably, they will only trigger on buggy 
> filesystems (programmer error), and thus IMO shouldn't even be executed 
> in a non-debug kernel.
> 
> Even though BUG_ON() includes unlikely(), I think this patch -- or 
> something like it -- is preferable.  The buffer_error() checks aren't 
> even marked unlikely().
> 
> This is a micro-optimization on a key kernel fast path.
> 

buffer_error() was always supposed to be temporary.  Once per month someone
reports the one in __find_get_block_slow(), but that's all.  The only
reason for keeping it around is as a debug aid to filesystem developers.

We could make it a no-op if !CONFIG_BUFFER_DEBUG.

Re: [PATCH] conditionalize some boring buffer_head checks

[Andrew Morton]

Andrew Morton <akpm@osdl.org> wrote:
>
>  buffer_error() was always supposed to be temporary.  Once per month someone
>  reports the one in __find_get_block_slow(), but that's all.  The only
>  reason for keeping it around is as a debug aid to filesystem developers.
> 
>  We could make it a no-op if !CONFIG_BUFFER_DEBUG.

But even if we do that, the compiler cannot optimise away things like:

	if (atomic_read(&bh->b_count) == 0 &&
			!PageLocked(bh->b_page) &&
			!PageWriteback(bh->b_page))
		do {} while (0);

so if it offends you, go kill the thing outright.

Re: [PATCH] conditionalize some boring buffer_head checks

[Jeff Garzik]

[-- Attachment #1: Type: text/plain, Size: 1010 bytes --]

Andrew Morton wrote:
> Andrew Morton <akpm@osdl.org> wrote:
> 
>> buffer_error() was always supposed to be temporary.  Once per month someone
>> reports the one in __find_get_block_slow(), but that's all.  The only
>> reason for keeping it around is as a debug aid to filesystem developers.
>>
>> We could make it a no-op if !CONFIG_BUFFER_DEBUG.
> 
> 
> But even if we do that, the compiler cannot optimise away things like:
> 
> 	if (atomic_read(&bh->b_count) == 0 &&
> 			!PageLocked(bh->b_page) &&
> 			!PageWriteback(bh->b_page))
> 		do {} while (0);

Nod.


> so if it offends you, go kill the thing outright.

If you like config variables, here's a second patch (untested but should 
be obvious).  As a side note, we need a Kconfig for generic debugging 
options...

I would rather not kill the code in submit_bh() outright, just disable 
it for non-filesystem developers.

For me, buffer_error() definition is secondary to disabling these 
checks-that-noone-hits in the submit_bh() fast path.

	Jeff



[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 9635 bytes --]

===== arch/alpha/Kconfig 1.36 vs edited =====
--- 1.36/arch/alpha/Kconfig	Sat Mar 20 13:29:54 2004
+++ edited/arch/alpha/Kconfig	Wed Apr 14 04:03:41 2004
@@ -690,6 +690,13 @@
 	  Say Y here only if you plan to use gdb to debug the kernel.
 	  If you don't debug the kernel, you can say N.
 
+config DEBUG_BUFFERS
+	bool "Enable additional filesystem buffer_head checks"
+	depends on DEBUG_KERNEL
+	help
+	  If you say Y here, additional checks are performed that aid
+	  filesystem development.
+
 endmenu
 
 source "security/Kconfig"
===== arch/arm/Kconfig 1.53 vs edited =====
--- 1.53/arch/arm/Kconfig	Thu Apr  8 15:41:09 2004
+++ edited/arch/arm/Kconfig	Wed Apr 14 04:03:58 2004
@@ -724,6 +724,13 @@
 	  you are concerned with the code size or don't want to see these
 	  messages.
 
+config DEBUG_BUFFERS
+	bool "Enable additional filesystem buffer_head checks"
+	depends on DEBUG_KERNEL
+	help
+	  If you say Y here, additional checks are performed that aid
+	  filesystem development.
+
 # These options are only for real kernel hackers who want to get their hands dirty. 
 config DEBUG_LL
 	bool "Kernel low-level debugging functions"
===== arch/arm26/Kconfig 1.11 vs edited =====
--- 1.11/arch/arm26/Kconfig	Mon Mar  1 10:52:18 2004
+++ edited/arch/arm26/Kconfig	Wed Apr 14 04:03:47 2004
@@ -316,6 +316,13 @@
 	  Say Y here only if you plan to use gdb to debug the kernel.
 	  If you don't debug the kernel, you can say N.
 
+config DEBUG_BUFFERS
+	bool "Enable additional filesystem buffer_head checks"
+	depends on DEBUG_KERNEL
+	help
+	  If you say Y here, additional checks are performed that aid
+	  filesystem development.
+
 # These options are only for real kernel hackers who want to get their hands dirty. 
 config DEBUG_LL
 	bool "Kernel low-level debugging functions"
===== arch/i386/Kconfig 1.116 vs edited =====
--- 1.116/arch/i386/Kconfig	Mon Apr 12 13:54:45 2004
+++ edited/arch/i386/Kconfig	Wed Apr 14 04:04:34 2004
@@ -1286,6 +1286,13 @@
 	  If you say Y here, various routines which may sleep will become very
 	  noisy if they are called with a spinlock held.	
 
+config DEBUG_BUFFERS
+	bool "Enable additional filesystem buffer_head checks"
+	depends on DEBUG_KERNEL
+	help
+	  If you say Y here, additional checks are performed that aid
+	  filesystem development.
+
 config FRAME_POINTER
 	bool "Compile the kernel with frame pointers"
 	help
===== arch/ia64/Kconfig 1.69 vs edited =====
--- 1.69/arch/ia64/Kconfig	Mon Apr 12 21:50:46 2004
+++ edited/arch/ia64/Kconfig	Wed Apr 14 04:04:46 2004
@@ -503,6 +503,13 @@
 	  Say Y here only if you plan to use gdb to debug the kernel.
 	  If you don't debug the kernel, you can say N.
 
+config DEBUG_BUFFERS
+	bool "Enable additional filesystem buffer_head checks"
+	depends on DEBUG_KERNEL
+	help
+	  If you say Y here, additional checks are performed that aid
+	  filesystem development.
+
 config SYSVIPC_COMPAT
 	bool
 	depends on COMPAT && SYSVIPC
===== arch/m68k/Kconfig 1.31 vs edited =====
--- 1.31/arch/m68k/Kconfig	Thu Feb 26 06:25:58 2004
+++ edited/arch/m68k/Kconfig	Wed Apr 14 04:04:50 2004
@@ -688,6 +688,13 @@
 	  Say Y here only if you plan to use gdb to debug the kernel.
 	  If you don't debug the kernel, you can say N.
 	  
+config DEBUG_BUFFERS
+	bool "Enable additional filesystem buffer_head checks"
+	depends on DEBUG_KERNEL
+	help
+	  If you say Y here, additional checks are performed that aid
+	  filesystem development.
+
 endmenu
 
 source "security/Kconfig"
===== arch/mips/Kconfig 1.24 vs edited =====
--- 1.24/arch/mips/Kconfig	Fri Mar 19 01:04:54 2004
+++ edited/arch/mips/Kconfig	Wed Apr 14 04:05:05 2004
@@ -1253,6 +1253,13 @@
 	  If you say Y here, various routines which may sleep will become very
 	  noisy if they are called with a spinlock held.
 
+config DEBUG_BUFFERS
+	bool "Enable additional filesystem buffer_head checks"
+	depends on DEBUG_KERNEL
+	help
+	  If you say Y here, additional checks are performed that aid
+	  filesystem development.
+
 config RTC_DS1742
 	bool "DS1742 BRAM/RTC support"
 	depends on TOSHIBA_JMR3927 || TOSHIBA_RBTX4927
===== arch/parisc/Kconfig 1.28 vs edited =====
--- 1.28/arch/parisc/Kconfig	Fri Mar 19 01:04:54 2004
+++ edited/arch/parisc/Kconfig	Wed Apr 14 04:05:10 2004
@@ -222,6 +222,13 @@
 	  Say Y here only if you plan to use gdb to debug the kernel.
 	  If you don't debug the kernel, you can say N.
 	  
+config DEBUG_BUFFERS
+	bool "Enable additional filesystem buffer_head checks"
+	depends on DEBUG_KERNEL
+	help
+	  If you say Y here, additional checks are performed that aid
+	  filesystem development.
+
 endmenu
 
 source "security/Kconfig"
===== arch/ppc/Kconfig 1.57 vs edited =====
--- 1.57/arch/ppc/Kconfig	Tue Mar 30 10:39:41 2004
+++ edited/arch/ppc/Kconfig	Wed Apr 14 04:05:26 2004
@@ -1209,6 +1209,13 @@
 	  debug the kernel.
 	  If you don't debug the kernel, you can say N.
 
+config DEBUG_BUFFERS
+	bool "Enable additional filesystem buffer_head checks"
+	depends on DEBUG_KERNEL
+	help
+	  If you say Y here, additional checks are performed that aid
+	  filesystem development.
+
 config BOOTX_TEXT
 	bool "Support for early boot text console (BootX or OpenFirmware only)"
 	depends PPC_OF
===== arch/ppc64/Kconfig 1.52 vs edited =====
--- 1.52/arch/ppc64/Kconfig	Mon Apr 12 13:54:05 2004
+++ edited/arch/ppc64/Kconfig	Wed Apr 14 04:05:16 2004
@@ -395,6 +395,13 @@
 	  Say Y here only if you plan to use gdb to debug the kernel.
 	  If you don't debug the kernel, you can say N.
 	  
+config DEBUG_BUFFERS
+	bool "Enable additional filesystem buffer_head checks"
+	depends on DEBUG_KERNEL
+	help
+	  If you say Y here, additional checks are performed that aid
+	  filesystem development.
+
 endmenu
 
 source "security/Kconfig"
===== arch/s390/Kconfig 1.25 vs edited =====
--- 1.25/arch/s390/Kconfig	Fri Mar 19 01:04:54 2004
+++ edited/arch/s390/Kconfig	Wed Apr 14 04:05:32 2004
@@ -397,6 +397,13 @@
 	  Say Y here only if you plan to use gdb to debug the kernel.
 	  If you don't debug the kernel, you can say N.
 	  
+config DEBUG_BUFFERS
+	bool "Enable additional filesystem buffer_head checks"
+	depends on DEBUG_KERNEL
+	help
+	  If you say Y here, additional checks are performed that aid
+	  filesystem development.
+
 config DEBUG_SPINLOCK_SLEEP
 	bool "Sleep-inside-spinlock checking"
 	help
===== arch/sparc/Kconfig 1.28 vs edited =====
--- 1.28/arch/sparc/Kconfig	Fri Mar 19 01:04:54 2004
+++ edited/arch/sparc/Kconfig	Wed Apr 14 04:06:04 2004
@@ -448,6 +448,13 @@
 	  of the BUG call as well as the EIP and oops trace.  This aids
 	  debugging but costs about 70-100K of memory.
 
+config DEBUG_BUFFERS
+	bool "Enable additional filesystem buffer_head checks"
+	depends on DEBUG_KERNEL
+	help
+	  If you say Y here, additional checks are performed that aid
+	  filesystem development.
+
 endmenu
 
 source "security/Kconfig"
===== arch/sparc64/Kconfig 1.50 vs edited =====
--- 1.50/arch/sparc64/Kconfig	Fri Mar 19 01:04:54 2004
+++ edited/arch/sparc64/Kconfig	Wed Apr 14 04:05:51 2004
@@ -679,6 +679,13 @@
 	  Say Y here only if you plan to use gdb to debug the kernel.
 	  If you don't debug the kernel, you can say N.
 	  
+config DEBUG_BUFFERS
+	bool "Enable additional filesystem buffer_head checks"
+	depends on DEBUG_KERNEL
+	help
+	  If you say Y here, additional checks are performed that aid
+	  filesystem development.
+
 config STACK_DEBUG
 	depends on DEBUG_KERNEL
 	bool "Stack Overflow Detection Support"
===== arch/um/Kconfig 1.14 vs edited =====
--- 1.14/arch/um/Kconfig	Mon Apr 12 13:53:57 2004
+++ edited/arch/um/Kconfig	Wed Apr 14 04:06:09 2004
@@ -233,6 +233,13 @@
         If you're truly short on disk space or don't expect to report any
         bugs back to the UML developers, say N, otherwise say Y.
 
+config DEBUG_BUFFERS
+	bool "Enable additional filesystem buffer_head checks"
+	depends on DEBUG_KERNEL
+	help
+	  If you say Y here, additional checks are performed that aid
+	  filesystem development.
+
 config FRAME_POINTER
 	bool
 	default y if DEBUG_INFO
===== arch/v850/Kconfig 1.21 vs edited =====
--- 1.21/arch/v850/Kconfig	Mon Feb 16 19:42:32 2004
+++ edited/arch/v850/Kconfig	Wed Apr 14 04:06:12 2004
@@ -320,6 +320,13 @@
 	  Say Y here only if you plan to use gdb to debug the kernel.
 	  If you don't debug the kernel, you can say N.
 
+config DEBUG_BUFFERS
+	bool "Enable additional filesystem buffer_head checks"
+	depends on DEBUG_KERNEL
+	help
+	  If you say Y here, additional checks are performed that aid
+	  filesystem development.
+
 config MAGIC_SYSRQ
 	bool "Magic SysRq key"
 	depends on DEBUG_KERNEL
===== arch/x86_64/Kconfig 1.47 vs edited =====
--- 1.47/arch/x86_64/Kconfig	Mon Apr 12 13:53:56 2004
+++ edited/arch/x86_64/Kconfig	Wed Apr 14 04:06:16 2004
@@ -471,6 +471,13 @@
 	  Please note that this option requires new binutils.
 	  If you don't debug the kernel, you can say N.
 	  
+config DEBUG_BUFFERS
+	bool "Enable additional filesystem buffer_head checks"
+	depends on DEBUG_KERNEL
+	help
+	  If you say Y here, additional checks are performed that aid
+	  filesystem development.
+
 config FRAME_POINTER
        bool "Compile the kernel with frame pointers"
        help
===== fs/buffer.c 1.237 vs edited =====
--- 1.237/fs/buffer.c	Wed Apr 14 03:18:09 2004
+++ edited/fs/buffer.c	Wed Apr 14 04:06:40 2004
@@ -2688,6 +2688,7 @@
 {
 	struct bio *bio;
 
+#ifdef CONFIG_DEBUG_BUFFERS
 	BUG_ON(!buffer_locked(bh));
 	BUG_ON(!buffer_mapped(bh));
 	BUG_ON(!bh->b_end_io);
@@ -2698,6 +2699,7 @@
 		buffer_error();
 	if (rw == READ && buffer_dirty(bh))
 		buffer_error();
+#endif
 
 	/* Only clear out a write error when rewriting */
 	if (test_set_buffer_req(bh) && rw == WRITE)

Re: [PATCH] conditionalize some boring buffer_head checks

[Andrew Morton]

Jeff Garzik <jgarzik@pobox.com> wrote:
>
> I would rather not kill the code in submit_bh() outright, just disable 
>  it for non-filesystem developers.

submit_bh() is a slowpath ;) The one in mark_buffer_dirty() will be called
more often, possibly others.  Kill!

Re: [PATCH] conditionalize some boring buffer_head checks

[Tim Hockin]

Somewhat off the original topic, but am I the only one who finds it weird
(and error-prone) that you have to add the same KConfig to a dozen or more
Kconfig files?

Shouldn't there be a KConfig libe, and all you need to do for the arch is
reference the CONFIG_FOO from the lib?  Would at least save all the
duplicate strings and definitions...

Kconfig.lib:
	config DEBUG_BUFFERS
		bool "Enable additional filesystem buffer_head checks"
		depends on DEBUG_KERNEL
		help
		  If you say Y here, additional checks are performed that
		  aid filesystem development.

arch/*/Kconfig
	libpath /Kconfig.lib
	...
	insert DEBUG_BUFFERS
	...

If the inserted symbol is not found in the Kconfig libpath, error out.
You can then break debug Kconfigs into a separate lib file, etc.  Maybe
that's too far, but you get the idea?

Sorry, just a nit that bothers me.

Re: [PATCH] conditionalize some boring buffer_head checks

[Jens Axboe]

On Wed, Apr 14 2004, Jeff Garzik wrote:
> 
> These checks are executed billions of times per day, with no stack dump 
> bug reports sent to lkml.  Arguably, they will only trigger on buggy 
> filesystems (programmer error), and thus IMO shouldn't even be executed 
> in a non-debug kernel.
> 
> Even though BUG_ON() includes unlikely(), I think this patch -- or 
> something like it -- is preferable.  The buffer_error() checks aren't 
> even marked unlikely().
> 
> This is a micro-optimization on a key kernel fast path.

As Andrew mentioned, this isn't a fast path at all. You are potentially
even going to block on this call, and even if you don't you'll end up
spending a butt load of cycles in the io scheduler.

> ===== fs/buffer.c 1.237 vs edited =====
> --- 1.237/fs/buffer.c	Wed Apr 14 03:18:09 2004
> +++ edited/fs/buffer.c	Wed Apr 14 03:39:15 2004
> @@ -2688,6 +2688,7 @@
>  {
>  	struct bio *bio;
>  
> +#ifdef BH_DEBUG
>  	BUG_ON(!buffer_locked(bh));
>  	BUG_ON(!buffer_mapped(bh));
>  	BUG_ON(!bh->b_end_io);

The last one will be 'caught' at the other end of io completion, so I
guess that could be killed (even though you already lost the context of
the error, then). The first two are buffer state errors, I think those
should be kept unconditionally.

> @@ -2698,6 +2699,7 @@
>  		buffer_error();
>  	if (rw == READ && buffer_dirty(bh))
>  		buffer_error();
> +#endif

I'm fine with killing the buffer_error(), maybe

	if (rw == WRITE && !buffer_uptodate(bh))
		buffer_error();

should be kept though.

-- 
Jens Axboe

Re: [PATCH] conditionalize some boring buffer_head checks

[Jeff Garzik]

Jens Axboe wrote:
> On Wed, Apr 14 2004, Jeff Garzik wrote:
>>===== fs/buffer.c 1.237 vs edited =====
>>--- 1.237/fs/buffer.c	Wed Apr 14 03:18:09 2004
>>+++ edited/fs/buffer.c	Wed Apr 14 03:39:15 2004
>>@@ -2688,6 +2688,7 @@
>> {
>> 	struct bio *bio;
>> 
>>+#ifdef BH_DEBUG
>> 	BUG_ON(!buffer_locked(bh));
>> 	BUG_ON(!buffer_mapped(bh));
>> 	BUG_ON(!bh->b_end_io);
> 
> 
> The last one will be 'caught' at the other end of io completion, so I
> guess that could be killed (even though you already lost the context of
> the error, then). The first two are buffer state errors, I think those
> should be kept unconditionally.
> 
> 
>>@@ -2698,6 +2699,7 @@
>> 		buffer_error();
>> 	if (rw == READ && buffer_dirty(bh))
>> 		buffer_error();
>>+#endif
> 
> 
> I'm fine with killing the buffer_error(), maybe
> 
> 	if (rw == WRITE && !buffer_uptodate(bh))
> 		buffer_error();
> 
> should be kept though.


Well, all of these are buffer state (and programmer) errors...

	Jeff

Re: [PATCH] conditionalize some boring buffer_head checks

[Jeff Garzik]

[-- Attachment #1: Type: text/plain, Size: 429 bytes --]

Andrew Morton wrote:
> Jeff Garzik <jgarzik@pobox.com> wrote:
> 
>>I would rather not kill the code in submit_bh() outright, just disable 
>> it for non-filesystem developers.
> 
> 
> submit_bh() is a slowpath ;) The one in mark_buffer_dirty() will be called

Ah, indeed.


> more often, possibly others.  Kill!

Overkill?  (attached)  I didn't mess with the BUG_ON's in submit_bh this 
time, just buffer_error() stuff.

	Jeff



[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 7997 bytes --]

===== fs/buffer.c 1.237 vs edited =====
--- 1.237/fs/buffer.c	Wed Apr 14 03:18:09 2004
+++ edited/fs/buffer.c	Wed Apr 14 04:32:02 2004
@@ -51,25 +51,6 @@
 	wait_queue_head_t wqh;
 } ____cacheline_aligned_in_smp bh_wait_queue_heads[1<<BH_WAIT_TABLE_ORDER];
 
-/*
- * Debug/devel support stuff
- */
-
-void __buffer_error(char *file, int line)
-{
-	static int enough;
-
-	if (enough > 10)
-		return;
-	enough++;
-	printk("buffer layer error at %s:%d\n", file, line);
-#ifndef CONFIG_KALLSYMS
-	printk("Pass this trace through ksymoops for reporting\n");
-#endif
-	dump_stack();
-}
-EXPORT_SYMBOL(__buffer_error);
-
 inline void
 init_buffer(struct buffer_head *bh, bh_end_io_t *handler, void *private)
 {
@@ -99,17 +80,6 @@
 
 void fastcall unlock_buffer(struct buffer_head *bh)
 {
-	/*
-	 * unlock_buffer against a zero-count bh is a bug, if the page
-	 * is not locked.  Because then nothing protects the buffer's
-	 * waitqueue, which is used here. (Well.  Other locked buffers
-	 * against the page will pin it.  But complain anyway).
-	 */
-	if (atomic_read(&bh->b_count) == 0 &&
-			!PageLocked(bh->b_page) &&
-			!PageWriteback(bh->b_page))
-		buffer_error();
-
 	clear_buffer_locked(bh);
 	smp_mb__after_clear_bit();
 	wake_up_buffer(bh);
@@ -125,10 +95,6 @@
 	wait_queue_head_t *wqh = bh_waitq_head(bh);
 	DEFINE_WAIT(wait);
 
-	if (atomic_read(&bh->b_count) == 0 &&
-			(!bh->b_page || !PageLocked(bh->b_page)))
-		buffer_error();
-
 	do {
 		prepare_to_wait(wqh, &wait, TASK_UNINTERRUPTIBLE);
 		if (buffer_locked(bh)) {
@@ -146,8 +112,6 @@
 static void
 __set_page_buffers(struct page *page, struct buffer_head *head)
 {
-	if (page_has_buffers(page))
-		buffer_error();
 	page_cache_get(page);
 	SetPagePrivate(page);
 	page->private = (unsigned long)head;
@@ -433,7 +397,8 @@
 		}
 		bh = bh->b_this_page;
 	} while (bh != head);
-	buffer_error();
+
+	BUG();
 	printk("block=%llu, b_blocknr=%llu\n",
 		(unsigned long long)block, (unsigned long long)bh->b_blocknr);
 	printk("b_state=0x%08lx, b_size=%u\n", bh->b_state, bh->b_size);
@@ -847,10 +812,7 @@
 		struct buffer_head *bh = head;
 
 		do {
-			if (buffer_uptodate(bh))
-				set_buffer_dirty(bh);
-			else
-				buffer_error();
+			set_buffer_dirty(bh);
 			bh = bh->b_this_page;
 		} while (bh != head);
 	}
@@ -1151,7 +1113,7 @@
 	return page;
 
 failed:
-	buffer_error();
+	BUG();
 	unlock_page(page);
 	page_cache_release(page);
 	return NULL;
@@ -1247,8 +1209,6 @@
  */
 void fastcall mark_buffer_dirty(struct buffer_head *bh)
 {
-	if (!buffer_uptodate(bh))
-		buffer_error();
 	if (!buffer_dirty(bh) && !test_set_buffer_dirty(bh))
 		__set_page_dirty_nobuffers(bh->b_page);
 }
@@ -1267,7 +1227,7 @@
 		return;
 	}
 	printk(KERN_ERR "VFS: brelse: Trying to free free buffer\n");
-	buffer_error();		/* For the stack backtrace */
+	BUG();
 }
 
 /*
@@ -1294,8 +1254,6 @@
 		unlock_buffer(bh);
 		return bh;
 	} else {
-		if (buffer_dirty(bh))
-			buffer_error();
 		get_bh(bh);
 		bh->b_end_io = end_buffer_read_sync;
 		submit_bh(READ, bh);
@@ -1737,8 +1695,6 @@
 	last_block = (i_size_read(inode) - 1) >> inode->i_blkbits;
 
 	if (!page_has_buffers(page)) {
-		if (!PageUptodate(page))
-			buffer_error();
 		create_empty_buffers(page, 1 << inode->i_blkbits,
 					(1 << BH_Dirty)|(1 << BH_Uptodate));
 	}
@@ -1777,8 +1733,6 @@
 			clear_buffer_dirty(bh);
 			set_buffer_uptodate(bh);
 		} else if (!buffer_mapped(bh) && buffer_dirty(bh)) {
-			if (buffer_new(bh))
-				buffer_error();
 			err = get_block(inode, block, bh, 1);
 			if (err)
 				goto recover;
@@ -1811,8 +1765,6 @@
 			continue;
 		}
 		if (test_clear_buffer_dirty(bh)) {
-			if (!buffer_uptodate(bh))
-				buffer_error();
 			mark_buffer_async_write(bh);
 		} else {
 			unlock_buffer(bh);
@@ -1942,8 +1894,6 @@
 				unmap_underlying_metadata(bh->b_bdev,
 							bh->b_blocknr);
 				if (PageUptodate(page)) {
-					if (!buffer_mapped(bh))
-						buffer_error();
 					set_buffer_uptodate(bh);
 					continue;
 				}
@@ -2001,8 +1951,6 @@
 			void *kaddr;
 
 			clear_buffer_new(bh);
-			if (buffer_uptodate(bh))
-				buffer_error();
 			kaddr = kmap_atomic(page, KM_USER0);
 			memset(kaddr+block_start, 0, bh->b_size);
 			kunmap_atomic(kaddr, KM_USER0);
@@ -2068,8 +2016,6 @@
 
 	if (!PageLocked(page))
 		PAGE_BUG(page);
-	if (PageUptodate(page))
-		buffer_error();
 	blocksize = 1 << inode->i_blkbits;
 	if (!page_has_buffers(page))
 		create_empty_buffers(page, blocksize, 0);
@@ -2692,13 +2638,6 @@
 	BUG_ON(!buffer_mapped(bh));
 	BUG_ON(!bh->b_end_io);
 
-	if ((rw == READ || rw == READA) && buffer_uptodate(bh))
-		buffer_error();
-	if (rw == WRITE && !buffer_uptodate(bh))
-		buffer_error();
-	if (rw == READ && buffer_dirty(bh))
-		buffer_error();
-
 	/* Only clear out a write error when rewriting */
 	if (test_set_buffer_req(bh) && rw == WRITE)
 		clear_buffer_write_io_error(bh);
@@ -2798,21 +2737,6 @@
 }
 
 /*
- * Sanity checks for try_to_free_buffers.
- */
-static void check_ttfb_buffer(struct page *page, struct buffer_head *bh)
-{
-	if (!buffer_uptodate(bh) && !buffer_req(bh)) {
-		if (PageUptodate(page) && page->mapping
-			&& buffer_mapped(bh)	/* discard_buffer */
-			&& S_ISBLK(page->mapping->host->i_mode))
-		{
-			buffer_error();
-		}
-	}
-}
-
-/*
  * try_to_free_buffers() checks if all the buffers on this particular page
  * are unused, and releases them if so.
  *
@@ -2847,7 +2771,6 @@
 
 	bh = head;
 	do {
-		check_ttfb_buffer(page, bh);
 		if (buffer_write_io_error(bh))
 			set_bit(AS_EIO, &page->mapping->flags);
 		if (buffer_busy(bh))
@@ -2856,9 +2779,6 @@
 			was_uptodate = 0;
 		bh = bh->b_this_page;
 	} while (bh != head);
-
-	if (!was_uptodate && PageUptodate(page) && !PageError(page))
-		buffer_error();
 
 	do {
 		struct buffer_head *next = bh->b_this_page;
===== fs/mpage.c 1.54 vs edited =====
--- 1.54/fs/mpage.c	Mon Apr 12 13:54:41 2004
+++ edited/fs/mpage.c	Wed Apr 14 04:33:06 2004
@@ -485,8 +485,7 @@
 			break;
 		block_in_file++;
 	}
-	if (page_block == 0)
-		buffer_error();
+	BUG_ON(page_block == 0);
 
 	first_unmapped = page_block;
 
===== fs/ext3/inode.c 1.90 vs edited =====
--- 1.90/fs/ext3/inode.c	Tue Jan 20 20:58:53 2004
+++ edited/fs/ext3/inode.c	Wed Apr 14 04:33:34 2004
@@ -1358,8 +1358,6 @@
 	}
 
 	if (!page_has_buffers(page)) {
-		if (!PageUptodate(page))
-			buffer_error();
 		create_empty_buffers(page, inode->i_sb->s_blocksize,
 				(1 << BH_Dirty)|(1 << BH_Uptodate));
 	}
===== fs/ntfs/aops.c 1.96 vs edited =====
--- 1.96/fs/ntfs/aops.c	Mon Apr 12 13:54:35 2004
+++ edited/fs/ntfs/aops.c	Wed Apr 14 04:33:30 2004
@@ -1340,8 +1340,6 @@
 			void *kaddr;
 
 			clear_buffer_new(bh);
-			if (buffer_uptodate(bh))
-				buffer_error();
 			kaddr = kmap_atomic(page, KM_USER0);
 			memset(kaddr + block_start, 0, bh->b_size);
 			kunmap_atomic(kaddr, KM_USER0);
===== fs/reiserfs/inode.c 1.97 vs edited =====
--- 1.97/fs/reiserfs/inode.c	Mon Apr 12 13:54:58 2004
+++ edited/fs/reiserfs/inode.c	Wed Apr 14 04:33:23 2004
@@ -1925,7 +1925,6 @@
     th.t_trans_id = 0;
 
     if (!buffer_uptodate(bh_result)) {
-        buffer_error();
 	return -EIO;
     }
 
@@ -2057,8 +2056,6 @@
      * in the BH_Uptodate is just a sanity check.
      */
     if (!page_has_buffers(page)) {
-	if (!PageUptodate(page))
-	    buffer_error();
 	create_empty_buffers(page, inode->i_sb->s_blocksize, 
 	                    (1 << BH_Dirty) | (1 << BH_Uptodate));
     }
@@ -2120,8 +2117,6 @@
 	    }
 	}
 	if (test_clear_buffer_dirty(bh)) {
-	    if (!buffer_uptodate(bh))
-		buffer_error();
 	    mark_buffer_async_write(bh);
 	} else {
 	    unlock_buffer(bh);
===== include/linux/buffer_head.h 1.47 vs edited =====
--- 1.47/include/linux/buffer_head.h	Wed Apr 14 03:18:09 2004
+++ edited/include/linux/buffer_head.h	Wed Apr 14 04:32:14 2004
@@ -62,13 +62,6 @@
 };
 
 /*
- * Debug
- */
-
-void __buffer_error(char *file, int line);
-#define buffer_error() __buffer_error(__FILE__, __LINE__)
-
-/*
  * macro tricks to expand the set_buffer_foo(), clear_buffer_foo()
  * and buffer_foo() functions.
  */

Re: [PATCH] conditionalize some boring buffer_head checks

[Jens Axboe]

On Wed, Apr 14 2004, Jeff Garzik wrote:
> Jens Axboe wrote:
> >On Wed, Apr 14 2004, Jeff Garzik wrote:
> >>===== fs/buffer.c 1.237 vs edited =====
> >>--- 1.237/fs/buffer.c	Wed Apr 14 03:18:09 2004
> >>+++ edited/fs/buffer.c	Wed Apr 14 03:39:15 2004
> >>@@ -2688,6 +2688,7 @@
> >>{
> >>	struct bio *bio;
> >>
> >>+#ifdef BH_DEBUG
> >>	BUG_ON(!buffer_locked(bh));
> >>	BUG_ON(!buffer_mapped(bh));
> >>	BUG_ON(!bh->b_end_io);
> >
> >
> >The last one will be 'caught' at the other end of io completion, so I
> >guess that could be killed (even though you already lost the context of
> >the error, then). The first two are buffer state errors, I think those
> >should be kept unconditionally.
> >
> >
> >>@@ -2698,6 +2699,7 @@
> >>		buffer_error();
> >>	if (rw == READ && buffer_dirty(bh))
> >>		buffer_error();
> >>+#endif
> >
> >
> >I'm fine with killing the buffer_error(), maybe
> >
> >	if (rw == WRITE && !buffer_uptodate(bh))
> >		buffer_error();
> >
> >should be kept though.
> 
> 
> Well, all of these are buffer state (and programmer) errors...

Certainly, that is what they are meant to catch :-)

That's why I agree that some of them can be skipped, but I do think that
the ones I listed should be kept. It's an order of magnitude easier to
find and debug these errors if you are warned up front. I don't think
that saving those few cycles in an io submission path justifies that.

-- 
Jens Axboe

Re: [PATCH] conditionalize some boring buffer_head checks

[Jeff Garzik]

Tim Hockin wrote:
> Somewhat off the original topic, but am I the only one who finds it weird
> (and error-prone) that you have to add the same KConfig to a dozen or more
> Kconfig files?
> 
> Shouldn't there be a KConfig libe, and all you need to do for the arch is
> reference the CONFIG_FOO from the lib?  Would at least save all the
> duplicate strings and definitions...
> 
> Kconfig.lib:
> 	config DEBUG_BUFFERS
> 		bool "Enable additional filesystem buffer_head checks"
> 		depends on DEBUG_KERNEL
> 		help
> 		  If you say Y here, additional checks are performed that
> 		  aid filesystem development.
> 
> arch/*/Kconfig
> 	libpath /Kconfig.lib
> 	...
> 	insert DEBUG_BUFFERS
> 	...


Seems a lot easier just to gather the common definitions into a Kconfig 
file, and include it via the standard 'source' directive.

	Jeff

Re: [PATCH] conditionalize some boring buffer_head checks

[Jeff Garzik]

[-- Attachment #1: Type: text/plain, Size: 376 bytes --]

Andrew Morton wrote:
> Jeff Garzik <jgarzik@pobox.com> wrote:
> 
>>I would rather not kill the code in submit_bh() outright, just disable 
>> it for non-filesystem developers.
> 
> 
> submit_bh() is a slowpath ;) The one in mark_buffer_dirty() will be called
> more often, possibly others.  Kill!

Jens seems to like the debugging checks, so here's an alterna-patch.

	Jeff



[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 18145 bytes --]

===== arch/alpha/Kconfig 1.36 vs edited =====
--- 1.36/arch/alpha/Kconfig	Sat Mar 20 13:29:54 2004
+++ edited/arch/alpha/Kconfig	Wed Apr 14 04:58:08 2004
@@ -690,6 +690,13 @@
 	  Say Y here only if you plan to use gdb to debug the kernel.
 	  If you don't debug the kernel, you can say N.
 
+config DEBUG_BUFFERS
+	bool "Enable additional filesystem buffer_head checks"
+	depends on DEBUG_KERNEL
+	help
+	  If you say Y here, additional checks are performed that aid
+	  filesystem development.
+
 endmenu
 
 source "security/Kconfig"
===== arch/arm/Kconfig 1.53 vs edited =====
--- 1.53/arch/arm/Kconfig	Thu Apr  8 15:41:09 2004
+++ edited/arch/arm/Kconfig	Wed Apr 14 04:58:08 2004
@@ -724,6 +724,13 @@
 	  you are concerned with the code size or don't want to see these
 	  messages.
 
+config DEBUG_BUFFERS
+	bool "Enable additional filesystem buffer_head checks"
+	depends on DEBUG_KERNEL
+	help
+	  If you say Y here, additional checks are performed that aid
+	  filesystem development.
+
 # These options are only for real kernel hackers who want to get their hands dirty. 
 config DEBUG_LL
 	bool "Kernel low-level debugging functions"
===== arch/arm26/Kconfig 1.11 vs edited =====
--- 1.11/arch/arm26/Kconfig	Mon Mar  1 10:52:18 2004
+++ edited/arch/arm26/Kconfig	Wed Apr 14 04:58:08 2004
@@ -316,6 +316,13 @@
 	  Say Y here only if you plan to use gdb to debug the kernel.
 	  If you don't debug the kernel, you can say N.
 
+config DEBUG_BUFFERS
+	bool "Enable additional filesystem buffer_head checks"
+	depends on DEBUG_KERNEL
+	help
+	  If you say Y here, additional checks are performed that aid
+	  filesystem development.
+
 # These options are only for real kernel hackers who want to get their hands dirty. 
 config DEBUG_LL
 	bool "Kernel low-level debugging functions"
===== arch/i386/Kconfig 1.116 vs edited =====
--- 1.116/arch/i386/Kconfig	Mon Apr 12 13:54:45 2004
+++ edited/arch/i386/Kconfig	Wed Apr 14 04:58:08 2004
@@ -1286,6 +1286,13 @@
 	  If you say Y here, various routines which may sleep will become very
 	  noisy if they are called with a spinlock held.	
 
+config DEBUG_BUFFERS
+	bool "Enable additional filesystem buffer_head checks"
+	depends on DEBUG_KERNEL
+	help
+	  If you say Y here, additional checks are performed that aid
+	  filesystem development.
+
 config FRAME_POINTER
 	bool "Compile the kernel with frame pointers"
 	help
===== arch/ia64/Kconfig 1.69 vs edited =====
--- 1.69/arch/ia64/Kconfig	Mon Apr 12 21:50:46 2004
+++ edited/arch/ia64/Kconfig	Wed Apr 14 04:58:08 2004
@@ -503,6 +503,13 @@
 	  Say Y here only if you plan to use gdb to debug the kernel.
 	  If you don't debug the kernel, you can say N.
 
+config DEBUG_BUFFERS
+	bool "Enable additional filesystem buffer_head checks"
+	depends on DEBUG_KERNEL
+	help
+	  If you say Y here, additional checks are performed that aid
+	  filesystem development.
+
 config SYSVIPC_COMPAT
 	bool
 	depends on COMPAT && SYSVIPC
===== arch/m68k/Kconfig 1.31 vs edited =====
--- 1.31/arch/m68k/Kconfig	Thu Feb 26 06:25:58 2004
+++ edited/arch/m68k/Kconfig	Wed Apr 14 04:58:08 2004
@@ -688,6 +688,13 @@
 	  Say Y here only if you plan to use gdb to debug the kernel.
 	  If you don't debug the kernel, you can say N.
 	  
+config DEBUG_BUFFERS
+	bool "Enable additional filesystem buffer_head checks"
+	depends on DEBUG_KERNEL
+	help
+	  If you say Y here, additional checks are performed that aid
+	  filesystem development.
+
 endmenu
 
 source "security/Kconfig"
===== arch/mips/Kconfig 1.24 vs edited =====
--- 1.24/arch/mips/Kconfig	Fri Mar 19 01:04:54 2004
+++ edited/arch/mips/Kconfig	Wed Apr 14 04:58:08 2004
@@ -1253,6 +1253,13 @@
 	  If you say Y here, various routines which may sleep will become very
 	  noisy if they are called with a spinlock held.
 
+config DEBUG_BUFFERS
+	bool "Enable additional filesystem buffer_head checks"
+	depends on DEBUG_KERNEL
+	help
+	  If you say Y here, additional checks are performed that aid
+	  filesystem development.
+
 config RTC_DS1742
 	bool "DS1742 BRAM/RTC support"
 	depends on TOSHIBA_JMR3927 || TOSHIBA_RBTX4927
===== arch/parisc/Kconfig 1.28 vs edited =====
--- 1.28/arch/parisc/Kconfig	Fri Mar 19 01:04:54 2004
+++ edited/arch/parisc/Kconfig	Wed Apr 14 04:58:08 2004
@@ -222,6 +222,13 @@
 	  Say Y here only if you plan to use gdb to debug the kernel.
 	  If you don't debug the kernel, you can say N.
 	  
+config DEBUG_BUFFERS
+	bool "Enable additional filesystem buffer_head checks"
+	depends on DEBUG_KERNEL
+	help
+	  If you say Y here, additional checks are performed that aid
+	  filesystem development.
+
 endmenu
 
 source "security/Kconfig"
===== arch/ppc/Kconfig 1.57 vs edited =====
--- 1.57/arch/ppc/Kconfig	Tue Mar 30 10:39:41 2004
+++ edited/arch/ppc/Kconfig	Wed Apr 14 04:58:08 2004
@@ -1209,6 +1209,13 @@
 	  debug the kernel.
 	  If you don't debug the kernel, you can say N.
 
+config DEBUG_BUFFERS
+	bool "Enable additional filesystem buffer_head checks"
+	depends on DEBUG_KERNEL
+	help
+	  If you say Y here, additional checks are performed that aid
+	  filesystem development.
+
 config BOOTX_TEXT
 	bool "Support for early boot text console (BootX or OpenFirmware only)"
 	depends PPC_OF
===== arch/ppc64/Kconfig 1.52 vs edited =====
--- 1.52/arch/ppc64/Kconfig	Mon Apr 12 13:54:05 2004
+++ edited/arch/ppc64/Kconfig	Wed Apr 14 04:58:08 2004
@@ -395,6 +395,13 @@
 	  Say Y here only if you plan to use gdb to debug the kernel.
 	  If you don't debug the kernel, you can say N.
 	  
+config DEBUG_BUFFERS
+	bool "Enable additional filesystem buffer_head checks"
+	depends on DEBUG_KERNEL
+	help
+	  If you say Y here, additional checks are performed that aid
+	  filesystem development.
+
 endmenu
 
 source "security/Kconfig"
===== arch/s390/Kconfig 1.25 vs edited =====
--- 1.25/arch/s390/Kconfig	Fri Mar 19 01:04:54 2004
+++ edited/arch/s390/Kconfig	Wed Apr 14 04:58:08 2004
@@ -397,6 +397,13 @@
 	  Say Y here only if you plan to use gdb to debug the kernel.
 	  If you don't debug the kernel, you can say N.
 	  
+config DEBUG_BUFFERS
+	bool "Enable additional filesystem buffer_head checks"
+	depends on DEBUG_KERNEL
+	help
+	  If you say Y here, additional checks are performed that aid
+	  filesystem development.
+
 config DEBUG_SPINLOCK_SLEEP
 	bool "Sleep-inside-spinlock checking"
 	help
===== arch/sparc/Kconfig 1.28 vs edited =====
--- 1.28/arch/sparc/Kconfig	Fri Mar 19 01:04:54 2004
+++ edited/arch/sparc/Kconfig	Wed Apr 14 04:58:08 2004
@@ -448,6 +448,13 @@
 	  of the BUG call as well as the EIP and oops trace.  This aids
 	  debugging but costs about 70-100K of memory.
 
+config DEBUG_BUFFERS
+	bool "Enable additional filesystem buffer_head checks"
+	depends on DEBUG_KERNEL
+	help
+	  If you say Y here, additional checks are performed that aid
+	  filesystem development.
+
 endmenu
 
 source "security/Kconfig"
===== arch/sparc64/Kconfig 1.50 vs edited =====
--- 1.50/arch/sparc64/Kconfig	Fri Mar 19 01:04:54 2004
+++ edited/arch/sparc64/Kconfig	Wed Apr 14 04:58:08 2004
@@ -679,6 +679,13 @@
 	  Say Y here only if you plan to use gdb to debug the kernel.
 	  If you don't debug the kernel, you can say N.
 	  
+config DEBUG_BUFFERS
+	bool "Enable additional filesystem buffer_head checks"
+	depends on DEBUG_KERNEL
+	help
+	  If you say Y here, additional checks are performed that aid
+	  filesystem development.
+
 config STACK_DEBUG
 	depends on DEBUG_KERNEL
 	bool "Stack Overflow Detection Support"
===== arch/um/Kconfig 1.14 vs edited =====
--- 1.14/arch/um/Kconfig	Mon Apr 12 13:53:57 2004
+++ edited/arch/um/Kconfig	Wed Apr 14 04:58:08 2004
@@ -233,6 +233,13 @@
         If you're truly short on disk space or don't expect to report any
         bugs back to the UML developers, say N, otherwise say Y.
 
+config DEBUG_BUFFERS
+	bool "Enable additional filesystem buffer_head checks"
+	depends on DEBUG_KERNEL
+	help
+	  If you say Y here, additional checks are performed that aid
+	  filesystem development.
+
 config FRAME_POINTER
 	bool
 	default y if DEBUG_INFO
===== arch/v850/Kconfig 1.21 vs edited =====
--- 1.21/arch/v850/Kconfig	Mon Feb 16 19:42:32 2004
+++ edited/arch/v850/Kconfig	Wed Apr 14 04:58:08 2004
@@ -320,6 +320,13 @@
 	  Say Y here only if you plan to use gdb to debug the kernel.
 	  If you don't debug the kernel, you can say N.
 
+config DEBUG_BUFFERS
+	bool "Enable additional filesystem buffer_head checks"
+	depends on DEBUG_KERNEL
+	help
+	  If you say Y here, additional checks are performed that aid
+	  filesystem development.
+
 config MAGIC_SYSRQ
 	bool "Magic SysRq key"
 	depends on DEBUG_KERNEL
===== arch/x86_64/Kconfig 1.47 vs edited =====
--- 1.47/arch/x86_64/Kconfig	Mon Apr 12 13:53:56 2004
+++ edited/arch/x86_64/Kconfig	Wed Apr 14 04:58:08 2004
@@ -471,6 +471,13 @@
 	  Please note that this option requires new binutils.
 	  If you don't debug the kernel, you can say N.
 	  
+config DEBUG_BUFFERS
+	bool "Enable additional filesystem buffer_head checks"
+	depends on DEBUG_KERNEL
+	help
+	  If you say Y here, additional checks are performed that aid
+	  filesystem development.
+
 config FRAME_POINTER
        bool "Compile the kernel with frame pointers"
        help
===== fs/buffer.c 1.237 vs edited =====
--- 1.237/fs/buffer.c	Wed Apr 14 03:18:09 2004
+++ edited/fs/buffer.c	Wed Apr 14 05:05:28 2004
@@ -55,6 +55,7 @@
  * Debug/devel support stuff
  */
 
+#ifdef CONFIG_DEBUG_BUFFERS
 void __buffer_error(char *file, int line)
 {
 	static int enough;
@@ -69,6 +70,7 @@
 	dump_stack();
 }
 EXPORT_SYMBOL(__buffer_error);
+#endif /* CONFIG_DEBUG_BUFFERS */
 
 inline void
 init_buffer(struct buffer_head *bh, bh_end_io_t *handler, void *private)
@@ -105,10 +107,9 @@
 	 * waitqueue, which is used here. (Well.  Other locked buffers
 	 * against the page will pin it.  But complain anyway).
 	 */
-	if (atomic_read(&bh->b_count) == 0 &&
+	buffer_errchk  (atomic_read(&bh->b_count) == 0 &&
 			!PageLocked(bh->b_page) &&
-			!PageWriteback(bh->b_page))
-		buffer_error();
+			!PageWriteback(bh->b_page));
 
 	clear_buffer_locked(bh);
 	smp_mb__after_clear_bit();
@@ -125,9 +126,8 @@
 	wait_queue_head_t *wqh = bh_waitq_head(bh);
 	DEFINE_WAIT(wait);
 
-	if (atomic_read(&bh->b_count) == 0 &&
-			(!bh->b_page || !PageLocked(bh->b_page)))
-		buffer_error();
+	buffer_errchk(atomic_read(&bh->b_count) == 0 &&
+			(!bh->b_page || !PageLocked(bh->b_page)));
 
 	do {
 		prepare_to_wait(wqh, &wait, TASK_UNINTERRUPTIBLE);
@@ -146,8 +146,7 @@
 static void
 __set_page_buffers(struct page *page, struct buffer_head *head)
 {
-	if (page_has_buffers(page))
-		buffer_error();
+	buffer_errchk(page_has_buffers(page));
 	page_cache_get(page);
 	SetPagePrivate(page);
 	page->private = (unsigned long)head;
@@ -433,7 +432,7 @@
 		}
 		bh = bh->b_this_page;
 	} while (bh != head);
-	buffer_error();
+	BUG();
 	printk("block=%llu, b_blocknr=%llu\n",
 		(unsigned long long)block, (unsigned long long)bh->b_blocknr);
 	printk("b_state=0x%08lx, b_size=%u\n", bh->b_state, bh->b_size);
@@ -847,10 +846,10 @@
 		struct buffer_head *bh = head;
 
 		do {
-			if (buffer_uptodate(bh))
+			if (likely(buffer_uptodate(bh)))
 				set_buffer_dirty(bh);
 			else
-				buffer_error();
+				BUG();
 			bh = bh->b_this_page;
 		} while (bh != head);
 	}
@@ -1151,7 +1150,7 @@
 	return page;
 
 failed:
-	buffer_error();
+	BUG();
 	unlock_page(page);
 	page_cache_release(page);
 	return NULL;
@@ -1247,8 +1246,7 @@
  */
 void fastcall mark_buffer_dirty(struct buffer_head *bh)
 {
-	if (!buffer_uptodate(bh))
-		buffer_error();
+	buffer_errchk(!buffer_uptodate(bh));
 	if (!buffer_dirty(bh) && !test_set_buffer_dirty(bh))
 		__set_page_dirty_nobuffers(bh->b_page);
 }
@@ -1267,7 +1265,7 @@
 		return;
 	}
 	printk(KERN_ERR "VFS: brelse: Trying to free free buffer\n");
-	buffer_error();		/* For the stack backtrace */
+	BUG();
 }
 
 /*
@@ -1294,8 +1292,7 @@
 		unlock_buffer(bh);
 		return bh;
 	} else {
-		if (buffer_dirty(bh))
-			buffer_error();
+		buffer_errchk(buffer_dirty(bh));
 		get_bh(bh);
 		bh->b_end_io = end_buffer_read_sync;
 		submit_bh(READ, bh);
@@ -1687,8 +1684,7 @@
 	old_bh = __find_get_block_slow(bdev, block, 0);
 	if (old_bh) {
 #if 0	/* This happens.  Later. */
-		if (buffer_dirty(old_bh))
-			buffer_error();
+		buffer_errchk(buffer_dirty(old_bh));
 #endif
 		clear_buffer_dirty(old_bh);
 		wait_on_buffer(old_bh);
@@ -1737,8 +1733,7 @@
 	last_block = (i_size_read(inode) - 1) >> inode->i_blkbits;
 
 	if (!page_has_buffers(page)) {
-		if (!PageUptodate(page))
-			buffer_error();
+		buffer_errchk(!PageUptodate(page));
 		create_empty_buffers(page, 1 << inode->i_blkbits,
 					(1 << BH_Dirty)|(1 << BH_Uptodate));
 	}
@@ -1768,8 +1763,7 @@
 			 * this page can be outside i_size when there is a
 			 * truncate in progress.
 			 *
-			 * if (buffer_mapped(bh))
-			 *	buffer_error();
+			 * buffer_errchk(buffer_mapped(bh));
 			 */
 			/*
 			 * The buffer was zeroed by block_write_full_page()
@@ -1777,8 +1771,7 @@
 			clear_buffer_dirty(bh);
 			set_buffer_uptodate(bh);
 		} else if (!buffer_mapped(bh) && buffer_dirty(bh)) {
-			if (buffer_new(bh))
-				buffer_error();
+			buffer_errchk(buffer_new(bh));
 			err = get_block(inode, block, bh, 1);
 			if (err)
 				goto recover;
@@ -1811,8 +1804,7 @@
 			continue;
 		}
 		if (test_clear_buffer_dirty(bh)) {
-			if (!buffer_uptodate(bh))
-				buffer_error();
+			buffer_errchk(!buffer_uptodate(bh));
 			mark_buffer_async_write(bh);
 		} else {
 			unlock_buffer(bh);
@@ -1942,8 +1934,7 @@
 				unmap_underlying_metadata(bh->b_bdev,
 							bh->b_blocknr);
 				if (PageUptodate(page)) {
-					if (!buffer_mapped(bh))
-						buffer_error();
+					buffer_errchk(!buffer_mapped(bh));
 					set_buffer_uptodate(bh);
 					continue;
 				}
@@ -2001,8 +1992,7 @@
 			void *kaddr;
 
 			clear_buffer_new(bh);
-			if (buffer_uptodate(bh))
-				buffer_error();
+			buffer_errchk(buffer_uptodate(bh));
 			kaddr = kmap_atomic(page, KM_USER0);
 			memset(kaddr+block_start, 0, bh->b_size);
 			kunmap_atomic(kaddr, KM_USER0);
@@ -2068,8 +2058,7 @@
 
 	if (!PageLocked(page))
 		PAGE_BUG(page);
-	if (PageUptodate(page))
-		buffer_error();
+	buffer_errchk(PageUptodate(page));
 	blocksize = 1 << inode->i_blkbits;
 	if (!page_has_buffers(page))
 		create_empty_buffers(page, blocksize, 0);
@@ -2692,12 +2681,9 @@
 	BUG_ON(!buffer_mapped(bh));
 	BUG_ON(!bh->b_end_io);
 
-	if ((rw == READ || rw == READA) && buffer_uptodate(bh))
-		buffer_error();
-	if (rw == WRITE && !buffer_uptodate(bh))
-		buffer_error();
-	if (rw == READ && buffer_dirty(bh))
-		buffer_error();
+	buffer_errchk((rw == READ || rw == READA) && buffer_uptodate(bh));
+	buffer_errchk(rw == WRITE && !buffer_uptodate(bh));
+	buffer_errchk(rw == READ && buffer_dirty(bh));
 
 	/* Only clear out a write error when rewriting */
 	if (test_set_buffer_req(bh) && rw == WRITE)
@@ -2807,7 +2793,7 @@
 			&& buffer_mapped(bh)	/* discard_buffer */
 			&& S_ISBLK(page->mapping->host->i_mode))
 		{
-			buffer_error();
+			BUG();
 		}
 	}
 }
@@ -2857,8 +2843,7 @@
 		bh = bh->b_this_page;
 	} while (bh != head);
 
-	if (!was_uptodate && PageUptodate(page) && !PageError(page))
-		buffer_error();
+	buffer_errchk(!was_uptodate && PageUptodate(page) && !PageError(page));
 
 	do {
 		struct buffer_head *next = bh->b_this_page;
===== fs/mpage.c 1.54 vs edited =====
--- 1.54/fs/mpage.c	Mon Apr 12 13:54:41 2004
+++ edited/fs/mpage.c	Wed Apr 14 05:06:40 2004
@@ -485,8 +485,7 @@
 			break;
 		block_in_file++;
 	}
-	if (page_block == 0)
-		buffer_error();
+	buffer_errchk(page_block == 0);
 
 	first_unmapped = page_block;
 
===== fs/ext3/inode.c 1.90 vs edited =====
--- 1.90/fs/ext3/inode.c	Tue Jan 20 20:58:53 2004
+++ edited/fs/ext3/inode.c	Wed Apr 14 05:06:48 2004
@@ -1358,8 +1358,7 @@
 	}
 
 	if (!page_has_buffers(page)) {
-		if (!PageUptodate(page))
-			buffer_error();
+		buffer_errchk(!PageUptodate(page));
 		create_empty_buffers(page, inode->i_sb->s_blocksize,
 				(1 << BH_Dirty)|(1 << BH_Uptodate));
 	}
===== fs/ntfs/aops.c 1.96 vs edited =====
--- 1.96/fs/ntfs/aops.c	Mon Apr 12 13:54:35 2004
+++ edited/fs/ntfs/aops.c	Wed Apr 14 05:06:55 2004
@@ -1340,8 +1340,7 @@
 			void *kaddr;
 
 			clear_buffer_new(bh);
-			if (buffer_uptodate(bh))
-				buffer_error();
+			buffer_errchk(buffer_uptodate(bh));
 			kaddr = kmap_atomic(page, KM_USER0);
 			memset(kaddr + block_start, 0, bh->b_size);
 			kunmap_atomic(kaddr, KM_USER0);
===== fs/reiserfs/inode.c 1.97 vs edited =====
--- 1.97/fs/reiserfs/inode.c	Mon Apr 12 13:54:58 2004
+++ edited/fs/reiserfs/inode.c	Wed Apr 14 05:07:16 2004
@@ -1924,10 +1924,8 @@
     /* catch places below that try to log something without starting a trans */
     th.t_trans_id = 0;
 
-    if (!buffer_uptodate(bh_result)) {
-        buffer_error();
+    if (!buffer_uptodate(bh_result))
 	return -EIO;
-    }
 
     kmap(bh_result->b_page) ;
 start_over:
@@ -2057,8 +2055,7 @@
      * in the BH_Uptodate is just a sanity check.
      */
     if (!page_has_buffers(page)) {
-	if (!PageUptodate(page))
-	    buffer_error();
+	buffer_errchk(!PageUptodate(page));
 	create_empty_buffers(page, inode->i_sb->s_blocksize, 
 	                    (1 << BH_Dirty) | (1 << BH_Uptodate));
     }
@@ -2120,8 +2117,7 @@
 	    }
 	}
 	if (test_clear_buffer_dirty(bh)) {
-	    if (!buffer_uptodate(bh))
-		buffer_error();
+	    buffer_errchk(!buffer_uptodate(bh));
 	    mark_buffer_async_write(bh);
 	} else {
 	    unlock_buffer(bh);
===== include/linux/buffer_head.h 1.47 vs edited =====
--- 1.47/include/linux/buffer_head.h	Wed Apr 14 03:18:09 2004
+++ edited/include/linux/buffer_head.h	Wed Apr 14 05:08:07 2004
@@ -7,6 +7,7 @@
 #ifndef _LINUX_BUFFER_HEAD_H
 #define _LINUX_BUFFER_HEAD_H
 
+#include <linux/config.h>
 #include <linux/types.h>
 #include <linux/fs.h>
 #include <linux/linkage.h>
@@ -65,8 +66,16 @@
  * Debug
  */
 
+#ifdef CONFIG_DEBUG_BUFFERS
 void __buffer_error(char *file, int line);
-#define buffer_error() __buffer_error(__FILE__, __LINE__)
+#define buffer_error(condition)					\
+	do {							\
+		if (unlikely(condition))			\
+			__buffer_error(__FILE__, __LINE__);	\
+	} while (0)
+#else
+#define buffer_error(condition)  do {} while (0)
+#endif
 
 /*
  * macro tricks to expand the set_buffer_foo(), clear_buffer_foo()

Re: [PATCH] conditionalize some boring buffer_head checks

[Andi Kleen]

Tim Hockin <thockin@hockin.org> writes:
>
> arch/*/Kconfig
> 	libpath /Kconfig.lib
> 	...
> 	insert DEBUG_BUFFERS
> 	...
>
> If the inserted symbol is not found in the Kconfig libpath, error out.
> You can then break debug Kconfigs into a separate lib file, etc.  Maybe
> that's too far, but you get the idea?

Sounds like a good idea to me. It would clean up the Kconfigs a lot.
Includes are often not finegrained enough.

-Andi

Re: [PATCH] conditionalize some boring buffer_head checks

[Chris Friesen]

Jeff Garzik wrote:
> Tim Hockin wrote:
> 
>> Somewhat off the original topic, but am I the only one who finds it weird
>> (and error-prone) that you have to add the same KConfig to a dozen or 
>> more
>> Kconfig files?
>>
>> Shouldn't there be a KConfig libe, and all you need to do for the arch is
>> reference the CONFIG_FOO from the lib?  Would at least save all the
>> duplicate strings and definitions...

> Seems a lot easier just to gather the common definitions into a Kconfig 
> file, and include it via the standard 'source' directive.

Either way, I personally would be extremely grateful for some kind of 
standard way to add a new configurable generic feature to every 
architecture.  I'm tired of having to add it manually.  I hadn't 
realized about the "source" feature--seems like we should be able to 
pull a lot of stuff into something like that even now.

On a side note, why is there no Kconfig for the "kernel" directory?

Chris

Re: [PATCH] conditionalize some boring buffer_head checks

[Randy.Dunlap]

On Wed, 14 Apr 2004 09:31:50 -0400 Chris Friesen wrote:

| Jeff Garzik wrote:
| > Tim Hockin wrote:
| > 
| >> Somewhat off the original topic, but am I the only one who finds it weird
| >> (and error-prone) that you have to add the same KConfig to a dozen or 
| >> more
| >> Kconfig files?
| >>
| >> Shouldn't there be a KConfig libe, and all you need to do for the arch is
| >> reference the CONFIG_FOO from the lib?  Would at least save all the
| >> duplicate strings and definitions...
| 
| > Seems a lot easier just to gather the common definitions into a Kconfig 
| > file, and include it via the standard 'source' directive.
| 
| Either way, I personally would be extremely grateful for some kind of 
| standard way to add a new configurable generic feature to every 
| architecture.  I'm tired of having to add it manually.  I hadn't 
| realized about the "source" feature--seems like we should be able to 
| pull a lot of stuff into something like that even now.
| 
| On a side note, why is there no Kconfig for the "kernel" directory?

Use init/Kconfig for that.

--
~Randy
"We have met the enemy and he is us."  -- Pogo (by Walt Kelly)
(Again.  Sometimes I think ln -s /usr/src/linux/.config .signature)

Re: [PATCH] conditionalize some boring buffer_head checks

[Matt Mackall]

On Wed, Apr 14, 2004 at 05:10:17AM -0400, Jeff Garzik wrote:
> Andrew Morton wrote:
> >Jeff Garzik <jgarzik@pobox.com> wrote:
> >
> >>I would rather not kill the code in submit_bh() outright, just disable 
> >>it for non-filesystem developers.
> >
> >
> >submit_bh() is a slowpath ;) The one in mark_buffer_dirty() will be called
> >more often, possibly others.  Kill!
> 
> Jens seems to like the debugging checks, so here's an alterna-patch.
> 
> 	Jeff
> 
> 

> ===== arch/alpha/Kconfig 1.36 vs edited =====
> +++ edited/arch/alpha/Kconfig	Wed Apr 14 04:58:08 2004
> @@ -690,6 +690,13 @@
>  	  Say Y here only if you plan to use gdb to debug the kernel.
>  	  If you don't debug the kernel, you can say N.
>  
> +config DEBUG_BUFFERS
> +	bool "Enable additional filesystem buffer_head checks"
> +	depends on DEBUG_KERNEL
> +	help
> +	  If you say Y here, additional checks are performed that aid
> +	  filesystem development.
> +
>  endmenu

Sticking this in arch/*/Kconfig seems silly (as does much of the
duplication in said files). Can we stick this and other debug bits
under the kallsyms option in init/Kconfig instead? Or alternately move
debugging bits into their own file that gets included as appropriate.

-- 
Matt Mackall : http://www.selenic.com : Linux development and consulting

Re: [PATCH] conditionalize some boring buffer_head checks

[Randy.Dunlap]

On Wed, 14 Apr 2004 16:25:39 -0500 Matt Mackall wrote:

| On Wed, Apr 14, 2004 at 05:10:17AM -0400, Jeff Garzik wrote:
| > Andrew Morton wrote:
| > >Jeff Garzik <jgarzik@pobox.com> wrote:
| > >
| > >>I would rather not kill the code in submit_bh() outright, just disable 
| > >>it for non-filesystem developers.
| > >
| > >
| > >submit_bh() is a slowpath ;) The one in mark_buffer_dirty() will be called
| > >more often, possibly others.  Kill!
| > 
| > Jens seems to like the debugging checks, so here's an alterna-patch.
| > 
| > 	Jeff
| > 
| > 
| 
| > ===== arch/alpha/Kconfig 1.36 vs edited =====
| > +++ edited/arch/alpha/Kconfig	Wed Apr 14 04:58:08 2004
| > @@ -690,6 +690,13 @@
| >  	  Say Y here only if you plan to use gdb to debug the kernel.
| >  	  If you don't debug the kernel, you can say N.
| >  
| > +config DEBUG_BUFFERS
| > +	bool "Enable additional filesystem buffer_head checks"
| > +	depends on DEBUG_KERNEL
| > +	help
| > +	  If you say Y here, additional checks are performed that aid
| > +	  filesystem development.
| > +
| >  endmenu
| 
| Sticking this in arch/*/Kconfig seems silly (as does much of the
| duplication in said files). Can we stick this and other debug bits
| under the kallsyms option in init/Kconfig instead? Or alternately move
| debugging bits into their own file that gets included as appropriate.

Yes, I'll jump onto that if noone else does it.
Matt, are you making a patch for this?

--
~Randy
"We have met the enemy and he is us."  -- Pogo (by Walt Kelly)
(Again.  Sometimes I think ln -s /usr/src/linux/.config .signature)

Re: [PATCH] conditionalize some boring buffer_head checks

[Jeff Garzik]

On Wed, Apr 14, 2004 at 04:25:39PM -0500, Matt Mackall wrote:
> Sticking this in arch/*/Kconfig seems silly (as does much of the
> duplication in said files). Can we stick this and other debug bits
> under the kallsyms option in init/Kconfig instead? Or alternately move
> debugging bits into their own file that gets included as appropriate.

I would rather have an arch/generic/Kconfig.debug file that gets
included.  init/Kconfig may be generic, but its name hardly implies its
purpose as used.

There are clearly two classes of debug options, one arch-specific, and
one not.

	Jeff

Re: [PATCH] conditionalize some boring buffer_head checks

[Matt Mackall]

On Wed, Apr 14, 2004 at 02:27:54PM -0700, Randy.Dunlap wrote:
> On Wed, 14 Apr 2004 16:25:39 -0500 Matt Mackall wrote:
> 
> | On Wed, Apr 14, 2004 at 05:10:17AM -0400, Jeff Garzik wrote:
> | > Andrew Morton wrote:
> | > >Jeff Garzik <jgarzik@pobox.com> wrote:
> | > >
> | > >>I would rather not kill the code in submit_bh() outright, just disable 
> | > >>it for non-filesystem developers.
> | > >
> | > >
> | > >submit_bh() is a slowpath ;) The one in mark_buffer_dirty() will be called
> | > >more often, possibly others.  Kill!
> | > 
> | > Jens seems to like the debugging checks, so here's an alterna-patch.
> | > 
> | > 	Jeff
> | > 
> | > 
> | 
> | > ===== arch/alpha/Kconfig 1.36 vs edited =====
> | > +++ edited/arch/alpha/Kconfig	Wed Apr 14 04:58:08 2004
> | > @@ -690,6 +690,13 @@
> | >  	  Say Y here only if you plan to use gdb to debug the kernel.
> | >  	  If you don't debug the kernel, you can say N.
> | >  
> | > +config DEBUG_BUFFERS
> | > +	bool "Enable additional filesystem buffer_head checks"
> | > +	depends on DEBUG_KERNEL
> | > +	help
> | > +	  If you say Y here, additional checks are performed that aid
> | > +	  filesystem development.
> | > +
> | >  endmenu
> | 
> | Sticking this in arch/*/Kconfig seems silly (as does much of the
> | duplication in said files). Can we stick this and other debug bits
> | under the kallsyms option in init/Kconfig instead? Or alternately move
> | debugging bits into their own file that gets included as appropriate.
> 
> Yes, I'll jump onto that if noone else does it.
> Matt, are you making a patch for this?

It's all yours.

-- 
Matt Mackall : http://www.selenic.com : Linux development and consulting

Re: [PATCH] conditionalize some boring buffer_head checks

[Matt Mackall]

On Wed, Apr 14, 2004 at 05:33:36PM -0400, Jeff Garzik wrote:
> On Wed, Apr 14, 2004 at 04:25:39PM -0500, Matt Mackall wrote:
> > Sticking this in arch/*/Kconfig seems silly (as does much of the
> > duplication in said files). Can we stick this and other debug bits
> > under the kallsyms option in init/Kconfig instead? Or alternately move
> > debugging bits into their own file that gets included as appropriate.
> 
> I would rather have an arch/generic/Kconfig.debug file that gets
> included.  init/Kconfig may be generic, but its name hardly implies its
> purpose as used.

Yes, and it's rather a dumping ground too.

> There are clearly two classes of debug options, one arch-specific, and
> one not.

There is a third class of options that aren't intrinsically
arch-specific, but are currently implemented on only a subset of
architectures (eg kgdb), that I would argue properly belong in the
generic debug section.

-- 
Matt Mackall : http://www.selenic.com : Linux development and consulting

Re: [PATCH] conditionalize some boring buffer_head checks

[Zwane Mwaikambo]

On Wed, 14 Apr 2004, Jeff Garzik wrote:

> On Wed, Apr 14, 2004 at 04:25:39PM -0500, Matt Mackall wrote:
> > Sticking this in arch/*/Kconfig seems silly (as does much of the
> > duplication in said files). Can we stick this and other debug bits
> > under the kallsyms option in init/Kconfig instead? Or alternately move
> > debugging bits into their own file that gets included as appropriate.
>
> I would rather have an arch/generic/Kconfig.debug file that gets
> included.  init/Kconfig may be generic, but its name hardly implies its
> purpose as used.
>
> There are clearly two classes of debug options, one arch-specific, and
> one not.

This sounds like lib/Kconfig