conditionalize some boring buffer_head checks

PATCH In review Jeff Garzik 10 participants 24 messages Apr 14, 2004 – Apr 15, 2004

Subsystems (12)

Arm (32-bit) VFS Library code MIPS +8 more subsystems

lore

Share & tools

lore

Jeff Garzik

author

2004-04-14 07:43 UTC #0 lore raw

PATCH 1/1 conditionalize some boring buffer_head checks

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.

===== fs/buffer.c 1.237 vs edited =====

fs/buffer.c +2 -0

@@ -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 =====

include/linux/buffer_head.h +2 -0

@@ -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 */

Andrew Morton

2004-04-14 07:59 UTC #1 lore raw

Quoted 7 lines · Jeff Garzik

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.

Quoted 6 lines

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.

Andrew Morton

2004-04-14 08:03 UTC #2 lore raw

Quoted 6 lines · Andrew Morton

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.

Jeff Garzik

author

2004-04-14 08:11 UTC #3 lore raw

Quoted 9 lines · Andrew Morton

Quoted 5 lines · Andrew Morton
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.

Quoted 6 lines

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.

Quoted 1 line

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

===== arch/alpha/Kconfig 1.36 vs edited =====

arch/alpha/Kconfig +7 -0

@@ -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 =====

arch/arm/Kconfig +7 -0

@@ -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 =====

arch/arm26/Kconfig +7 -0

@@ -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 =====

arch/i386/Kconfig +7 -0

@@ -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 =====

arch/ia64/Kconfig +7 -0

@@ -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 =====

arch/m68k/Kconfig +7 -0

@@ -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 =====

arch/mips/Kconfig +7 -0

@@ -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 =====

arch/parisc/Kconfig +7 -0

@@ -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 =====

arch/ppc/Kconfig +7 -0

@@ -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 =====

arch/ppc64/Kconfig +7 -0

@@ -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 =====

arch/s390/Kconfig +7 -0

@@ -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 =====

arch/sparc/Kconfig +7 -0

@@ -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 =====

arch/sparc64/Kconfig +7 -0

@@ -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 =====

arch/um/Kconfig +7 -0

@@ -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 =====

arch/v850/Kconfig +7 -0

@@ -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 =====

arch/x86_64/Kconfig +7 -0

@@ -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 =====

fs/buffer.c +2 -0

@@ -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)

Andrew Morton

2004-04-14 08:17 UTC #4 lore raw

Quoted 3 lines · Jeff Garzik

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!

Tim Hockin

2004-04-14 08:27 UTC #17 lore raw

In reply to Jeff Garzik

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.

Jens Axboe

2004-04-14 08:30 UTC #21 lore raw

In reply to Jeff Garzik

Quoted 11 lines · Jeff Garzik

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 line 2694

Quoted 11 lines

===== fs/buffer.c 1.237 vs edited =====

fs/buffer.c +1 -0

@@ -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.

fs/buffer.c line 2702

Quoted 5 lines

fs/buffer.c +1 -0

@@ -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.

Signature · 1 line

Jens Axboe

Jeff Garzik

author

2004-04-14 08:42 UTC #22 lore raw

Quoted 29 lines · Jens Axboe

fs/buffer.c line 2694
Quoted 11 lines · Jeff Garzik
===== fs/buffer.c 1.237 vs edited =====
fs/buffer.c +1 -0
@@ -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.
fs/buffer.c line 2702
Quoted 5 lines
fs/buffer.c +1 -0
@@ -2698,6 +2699,7 @@
 		buffer_error();
 	if (rw == READ && buffer_dirty(bh))
 		buffer_error();
+#endif
I'm fine with killing the buffer_error(), maybe

Quoted 4 lines

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

should be kept though.

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

Jeff

Jeff Garzik

author

2004-04-14 08:46 UTC #5 lore raw

In reply to Andrew Morton

Quoted 6 lines · Andrew Morton

Quoted 1 line · Jeff Garzik
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.

Quoted 1 line

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

===== fs/buffer.c 1.237 vs edited =====

fs/buffer.c +5 ~~-85~~

@@ -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 =====

fs/mpage.c +1 -2

@@ -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 =====

fs/ext3/inode.c +0 -2

@@ -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 =====

fs/ntfs/aops.c +0 -2

@@ -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 =====

fs/reiserfs/inode.c +0 -5

@@ -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 =====

include/linux/buffer_head.h +0 -7

@@ -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.
  */

Jens Axboe

2004-04-14 08:49 UTC #23 lore raw

In reply to Jeff Garzik

Quoted 30 lines · Jeff Garzik

Quoted 29 lines · Jens Axboe
fs/buffer.c line 2694
Quoted 11 lines · Jeff Garzik
===== fs/buffer.c 1.237 vs edited =====
fs/buffer.c +1 -0
@@ -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.
fs/buffer.c line 2702
Quoted 5 lines
fs/buffer.c +1 -0
@@ -2698,6 +2699,7 @@
 		buffer_error();
 	if (rw == READ && buffer_dirty(bh))
 		buffer_error();
+#endif
I'm fine with killing the buffer_error(), maybe

Quoted 7 lines

Quoted 4 lines
	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.

Signature · 1 line

Jens Axboe

Jeff Garzik

author

2004-04-14 08:51 UTC #18 lore raw

In reply to Tim Hockin

Quoted 16 lines · 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.

Quoted 5 lines

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

Jeff Garzik

author

2004-04-14 09:11 UTC #6 lore raw

In reply to Andrew Morton

Quoted 7 lines · Andrew Morton

Quoted 1 line · Jeff Garzik
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 =====

arch/alpha/Kconfig +7 -0

@@ -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 =====

arch/arm/Kconfig +7 -0

@@ -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 =====

arch/arm26/Kconfig +7 -0

@@ -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 =====

arch/i386/Kconfig +7 -0

@@ -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 =====

arch/ia64/Kconfig +7 -0

@@ -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 =====

arch/m68k/Kconfig +7 -0

@@ -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 =====

arch/mips/Kconfig +7 -0

@@ -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 =====

arch/parisc/Kconfig +7 -0

@@ -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 =====

arch/ppc/Kconfig +7 -0

@@ -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 =====

arch/ppc64/Kconfig +7 -0

@@ -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 =====

arch/s390/Kconfig +7 -0

@@ -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 =====

arch/sparc/Kconfig +7 -0

@@ -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 =====

arch/sparc64/Kconfig +7 -0

@@ -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 =====

arch/um/Kconfig +7 -0

@@ -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 =====

arch/v850/Kconfig +7 -0

@@ -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 =====

arch/x86_64/Kconfig +7 -0

@@ -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 =====

fs/buffer.c +27 ~~-42~~

204 lines not on this page · raw message

===== fs/mpage.c 1.54 vs edited =====

fs/mpage.c +1 -2

@@ -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 =====

fs/ext3/inode.c +1 -2

@@ -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 =====

fs/ntfs/aops.c +1 -2

@@ -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 =====

fs/reiserfs/inode.c +3 -7

@@ -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 =====

include/linux/buffer_head.h +10 -1

@@ -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()

Chris Friesen

2004-04-14 13:32 UTC #19 lore raw

In reply to Jeff Garzik

Quoted 11 lines · Jeff Garzik

Quoted 7 lines · 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...
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

Randy.Dunlap

2004-04-14 15:11 UTC #20 lore raw

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)

Matt Mackall

2004-04-14 21:26 UTC #7 lore raw

In reply to Jeff Garzik

Quoted 20 lines · Jeff Garzik

Quoted 8 lines · Andrew Morton
Quoted 2 lines · Jeff Garzik
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
arch/alpha/Kconfig +0 -0
@@ -690,6 +690,13 @@
 	  Say Y here only if you plan to use gdb to debug the kernel.

arch/alpha/Kconfig line 700

Quoted 10 lines

arch/alpha/Kconfig +7 -0

 	  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.

Signature · 1 line

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

Jeff Garzik

author

2004-04-14 21:33 UTC #8 lore raw

Quoted 4 lines · Matt Mackall

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

Randy.Dunlap

2004-04-14 21:33 UTC #11 lore raw

In reply to Matt Mackall

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 <[email protected]> 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)

Matt Mackall

2004-04-14 21:40 UTC #12 lore raw

Quoted 39 lines · 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 <[email protected]> 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.

Quoted 2 lines

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

It's all yours.

Signature · 1 line

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

Matt Mackall

2004-04-14 21:49 UTC #9 lore raw

In reply to Jeff Garzik

Quoted 9 lines · Jeff Garzik

Quoted 4 lines · Matt Mackall
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.

Quoted 2 lines

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.

Signature · 1 line

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

Zwane Mwaikambo

2004-04-15 06:12 UTC #10 lore raw

In reply to Jeff Garzik

Quoted 6 lines · Jeff Garzik

Quoted 4 lines · Matt Mackall
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.

Quoted 6 lines

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

Randy.Dunlap

2004-04-15 21:00 UTC #13 lore raw

PATCH] Kconfig.debug family

In reply to Matt Mackall

On Wed, 14 Apr 2004 16:25:39 -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.

This patch:
- creates lib/Kconfig.debug for generic kernel debug options
- creates arch/*/Kconfig.debug for arch-specific debug options
- moves KALLSYMS to the generic kernel debug options list

This is a first cut for review/comments. I will double-check the generic options list to see how it needs to be corrected...

Applies to 2.6.6-rc1.
All arches have been tested by using 'make ARCH=foo {menu|x}config'. However, ARCH=cris fails due to a missing file:
source "arch/cris/drivers/Kconfig".

--
~Randy

 arch/alpha/Kconfig.debug     |   54 ++++++++++++++
 arch/arm/Kconfig             |  159 -------------------------------------------
 arch/arm/Kconfig.debug       |  113 ++++++++++++++++++++++++++++++
 arch/arm26/Kconfig.debug     |   58 +++++++++++++++
 arch/cris/Kconfig.debug      |   15 ++++
 arch/h8300/Kconfig           |   71 -------------------
 arch/h8300/Kconfig.debug     |   67 ++++++++++++++++++
 arch/i386/Kconfig            |  124 ---------------------------------
 arch/i386/Kconfig.debug      |   15 ++++
 arch/ia64/Kconfig            |  113 ------------------------------
 arch/ia64/Kconfig.debug      |   63 +++++++++++++++++
 arch/m68k/Kconfig.debug      |    9 ++
 arch/m68knommu/Kconfig       |   54 --------------
 arch/m68knommu/Kconfig.debug |   42 +++++++++++
 arch/mips/Kconfig.debug      |   67 ++++++++++++++++++
 arch/parisc/Kconfig.debug    |    5 +
 arch/ppc/Kconfig.debug       |   72 +++++++++++++++++++
 arch/ppc64/Kconfig           |   79 ---------------------
 arch/ppc64/Kconfig.debug     |   27 +++++++
 arch/s390/Kconfig.debug      |    5 +
 arch/sh/Kconfig.debug        |  111 ++++++++++++++++++++++++++++++
 arch/sparc/Kconfig.debug     |   13 +++
 arch/sparc64/Kconfig.debug   |   38 ++++++++++
 arch/um/Kconfig              |   59 ---------------
 arch/um/Kconfig.debug        |   35 +++++++++
 arch/v850/Kconfig.debug      |   10 ++
 arch/x86_64/Kconfig          |  100 ---------------------------
 arch/x86_64/Kconfig.debug    |   47 ++++++++++++
 init/Kconfig                 |    8 --
 lib/Kconfig.debug            |  151 ++++++++++++++++++++++++++++++++++++++++
 30 files changed, 1033 insertions(+), 751 deletions(-)

arch/alpha/Kconfig.debug +54 -0

diff -Naurp -X /home/rddunlap/doc/dontdiff-osdl linux-266-rc1-pv/arch/alpha/Kconfig.debug linux-266-rc1-kdebug/arch/alpha/Kconfig.debug
@@ -0,0 +1,54 @@
+
+menu "Alpha kernel hacking"
+
+config ALPHA_LEGACY_START_ADDRESS
+	bool "Legacy kernel start address"
+	depends on ALPHA_GENERIC
+	default n
+	---help---
+	  The 2.4 kernel changed the kernel start address from 0x310000
+	  to 0x810000 to make room for the Wildfire's larger SRM console.
+	  Recent consoles on Titan and Marvel machines also require the
+	  extra room.
+
+	  If you're using aboot 0.7 or later, the bootloader will examine the
+	  ELF headers to determine where to transfer control. Unfortunately,
+	  most older bootloaders -- APB or MILO -- hardcoded the kernel start
+	  address rather than examining the ELF headers, and the result is a
+	  hard lockup.
+
+	  Say Y if you have a broken bootloader.  Say N if you do not, or if
+	  you wish to run on Wildfire, Titan, or Marvel.
+
+config ALPHA_LEGACY_START_ADDRESS
+	bool
+	depends on !ALPHA_GENERIC && !ALPHA_TITAN && !ALPHA_MARVEL && !ALPHA_WILDFIRE
+	default y
+
+config MATHEMU
+	tristate "Kernel FP software completion" if DEBUG_KERNEL
+	default y if !DEBUG_KERNEL
+	help
+	  This option is required for IEEE compliant floating point arithmetic
+	  on the Alpha. The only time you would ever not say Y is to say M in
+	  order to debug the code. Say Y unless you know what you are doing.
+
+config DEBUG_RWLOCK
+	bool "Read-write spinlock debugging"
+	depends on DEBUG_KERNEL
+	help
+	  If you say Y here then read-write lock processing will count how many
+	  times it has tried to get the lock and issue an error message after
+	  too many attempts.  If you suspect a rwlock problem or a kernel
+	  hacker asks for this option then say Y.  Otherwise say N.
+
+config DEBUG_SEMAPHORE
+	bool "Semaphore debugging"
+	depends on DEBUG_KERNEL
+	help
+	  If you say Y here then semaphore processing will issue lots of
+	  verbose debugging messages.  If you suspect a semaphore problem or a
+	  kernel hacker asks for this option then say Y.  Otherwise say N.
+
+endmenu
+

arch/arm/Kconfig +2 ~~-157~~

166 lines not on this page · raw message

arch/arm/Kconfig.debug +113 -0

113 lines not on this page · raw message

arch/arm26/Kconfig.debug +58 -0

58 lines not on this page · raw message

arch/cris/Kconfig.debug +15 -0

diff -Naurp -X /home/rddunlap/doc/dontdiff-osdl linux-266-rc1-pv/arch/cris/Kconfig.debug linux-266-rc1-kdebug/arch/cris/Kconfig.debug
@@ -0,0 +1,15 @@
+
+menu "CRIS kernel hacking"
+
+#bool 'Debug kmalloc/kfree' CONFIG_DEBUG_MALLOC
+
+config PROFILE
+	bool "Kernel profiling support"
+
+config PROFILE_SHIFT
+	int "Profile shift count"
+	depends on PROFILE
+	default "2"
+
+endmenu
+

arch/h8300/Kconfig +2 ~~-69~~

77 lines not on this page · raw message

arch/h8300/Kconfig.debug +67 -0

67 lines not on this page · raw message

arch/i386/Kconfig +2 ~~-122~~

131 lines not on this page · raw message

arch/i386/Kconfig.debug +15 -0

diff -Naurp -X /home/rddunlap/doc/dontdiff-osdl linux-266-rc1-pv/arch/i386/Kconfig.debug linux-266-rc1-kdebug/arch/i386/Kconfig.debug
@@ -0,0 +1,15 @@
+
+menu "X86 kernel hacking"
+
+config X86_FIND_SMP_CONFIG
+	bool
+	depends on X86_LOCAL_APIC || X86_VOYAGER
+	default y
+
+config X86_MPPARSE
+	bool
+	depends on X86_LOCAL_APIC && !X86_VISWS
+	default y
+
+endmenu
+

arch/ia64/Kconfig +2 ~~-111~~

119 lines not on this page · raw message

arch/ia64/Kconfig.debug +63 -0

63 lines not on this page · raw message

arch/m68k/Kconfig.debug +9 -0

diff -Naurp -X /home/rddunlap/doc/dontdiff-osdl linux-266-rc1-pv/arch/m68k/Kconfig.debug linux-266-rc1-kdebug/arch/m68k/Kconfig.debug
@@ -0,0 +1,9 @@
+
+menu "M68K kernel hacking"
+
+config DEBUG_BUGVERBOSE
+	bool "Verbose BUG() reporting"
+	depends on DEBUG_KERNEL
+
+endmenu
+

arch/m68knommu/Kconfig +2 ~~-52~~

60 lines not on this page · raw message

arch/m68knommu/Kconfig.debug +42 -0

42 lines not on this page · raw message

arch/mips/Kconfig.debug +67 -0

67 lines not on this page · raw message

arch/parisc/Kconfig.debug +5 -0

diff -Naurp -X /home/rddunlap/doc/dontdiff-osdl linux-266-rc1-pv/arch/parisc/Kconfig.debug linux-266-rc1-kdebug/arch/parisc/Kconfig.debug
@@ -0,0 +1,5 @@
+
+menu "PA-RISC kernel hacking"
+
+endmenu
+

arch/ppc/Kconfig.debug +72 -0

72 lines not on this page · raw message

arch/ppc64/Kconfig +2 ~~-77~~

91 lines not on this page · raw message

arch/ppc64/Kconfig.debug +27 -0

diff -Naurp -X /home/rddunlap/doc/dontdiff-osdl linux-266-rc1-pv/arch/ppc64/Kconfig.debug linux-266-rc1-kdebug/arch/ppc64/Kconfig.debug
@@ -0,0 +1,27 @@
+
+menu "PPC64 kernel hacking"
+
+config DEBUGGER
+	bool "Enable debugger hooks"
+	depends on DEBUG_KERNEL
+	help
+	  Include in-kernel hooks for kernel debuggers. Unless you are
+	  intending to debug the kernel, say N here.
+
+config XMON
+	bool "Include xmon kernel debugger"
+	depends on DEBUGGER
+	help
+	  Include in-kernel hooks for the xmon kernel monitor/debugger.
+	  Unless you are intending to debug the kernel, say N here.
+
+config XMON_DEFAULT
+	bool "Enable xmon by default"
+	depends on XMON
+
+config PPCDBG
+	bool "Include PPCDBG realtime debugging"
+	depends on DEBUG_KERNEL
+
+endmenu
+

arch/s390/Kconfig.debug +5 -0

diff -Naurp -X /home/rddunlap/doc/dontdiff-osdl linux-266-rc1-pv/arch/s390/Kconfig.debug linux-266-rc1-kdebug/arch/s390/Kconfig.debug
@@ -0,0 +1,5 @@
+
+menu "S/390 kernel hacking"
+
+endmenu
+

arch/sh/Kconfig.debug +111 -0

111 lines not on this page · raw message

arch/sparc/Kconfig.debug +13 -0

diff -Naurp -X /home/rddunlap/doc/dontdiff-osdl linux-266-rc1-pv/arch/sparc/Kconfig.debug linux-266-rc1-kdebug/arch/sparc/Kconfig.debug
@@ -0,0 +1,13 @@
+
+menu "SPARC kernel hacking"
+
+config DEBUG_BUGVERBOSE
+	bool "Verbose BUG() reporting (adds 70K)"
+	depends on DEBUG_KERNEL
+	help
+	  Say Y here to make BUG() panics output the file name and line number
+	  of the BUG call as well as the EIP and oops trace.  This aids
+	  debugging but costs about 70-100K of memory.
+
+endmenu
+

arch/sparc64/Kconfig.debug +38 -0

diff -Naurp -X /home/rddunlap/doc/dontdiff-osdl linux-266-rc1-pv/arch/sparc64/Kconfig.debug linux-266-rc1-kdebug/arch/sparc64/Kconfig.debug
@@ -0,0 +1,38 @@
+
+menu "UltraSPARC kernel hacking"
+
+config DEBUG_BUGVERBOSE
+	bool "Verbose BUG() reporting (adds 70K)"
+	depends on DEBUG_KERNEL
+	help
+	  Say Y here to make BUG() panics output the file name and line number
+	  of the BUG call as well as the EIP and oops trace.  This aids
+	  debugging but costs about 70-100K of memory.
+
+config DEBUG_DCFLUSH
+	bool "D-cache flush debugging"
+	depends on DEBUG_KERNEL
+
+config STACK_DEBUG
+	depends on DEBUG_KERNEL
+	bool "Stack Overflow Detection Support"
+
+config DEBUG_BOOTMEM
+	depends on DEBUG_KERNEL
+	bool "Debug BOOTMEM initialization"
+
+# We have a custom atomic_dec_and_lock() implementation but it's not
+# compatible with spinlock debugging so we need to fall back on
+# the generic version in that case.
+config HAVE_DEC_LOCK
+	bool
+	depends on SMP && !DEBUG_SPINLOCK
+	default y
+
+config MCOUNT
+	bool
+	depends on STACK_DEBUG
+	default y
+
+endmenu
+

arch/um/Kconfig +2 ~~-57~~

71 lines not on this page · raw message

arch/um/Kconfig.debug +35 -0

diff -Naurp -X /home/rddunlap/doc/dontdiff-osdl linux-266-rc1-pv/arch/um/Kconfig.debug linux-266-rc1-kdebug/arch/um/Kconfig.debug
@@ -0,0 +1,35 @@
+
+menu "UML kernel hacking"
+
+config PT_PROXY
+	bool "Enable ptrace proxy"
+	depends on XTERM_CHAN && DEBUG_INFO
+
+config GPROF
+	bool "Enable gprof support"
+	depends on DEBUG_INFO
+	help
+        This allows profiling of a User-Mode Linux kernel with the gprof
+        utility.
+
+        See <http://user-mode-linux.sourceforge.net/gprof.html> for more
+        details.
+
+        If you're involved in UML kernel development and want to use gprof,
+        say Y.  If you're unsure, say N.
+
+config GCOV
+	bool "Enable gcov support"
+	depends on DEBUG_INFO
+	help
+        This option allows developers to retrieve coverage data from a UML
+        session.
+
+        See <http://user-mode-linux.sourceforge.net/gprof.html> for more
+        details.
+
+        If you're involved in UML kernel development and want to use gcov,
+        say Y.  If you're unsure, say N.
+
+endmenu
+

arch/v850/Kconfig.debug +10 -0

diff -Naurp -X /home/rddunlap/doc/dontdiff-osdl linux-266-rc1-pv/arch/v850/Kconfig.debug linux-266-rc1-kdebug/arch/v850/Kconfig.debug
@@ -0,0 +1,10 @@
+
+menu "V850 kernel hacking"
+
+config NO_KERNEL_MSG
+	bool "Suppress Kernel BUG Messages"
+	help
+	  Do not output any debug BUG messages within the kernel.
+
+endmenu
+

arch/x86_64/Kconfig +2 ~~-98~~

106 lines not on this page · raw message

arch/x86_64/Kconfig.debug +47 -0

47 lines not on this page · raw message

init/Kconfig +0 -8

diff -Naurp -X /home/rddunlap/doc/dontdiff-osdl linux-266-rc1-pv/init/Kconfig linux-266-rc1-kdebug/init/Kconfig
@@ -227,14 +227,6 @@ menuconfig EMBEDDED
           environments which can tolerate a "non-standard" kernel.
           Only use this if you really know what you are doing.
 
-config KALLSYMS
-	 bool "Load all symbols for debugging/kksymoops" if EMBEDDED
-	 default y
-	 help
-	   Say Y here to let the kernel print out symbolic crash information and
-	   symbolic stack backtraces. This increases the size of the kernel
-	   somewhat, as all symbols have to be loaded into the kernel image.
-
 config FUTEX
 	bool "Enable futex support" if EMBEDDED
 	default y

lib/Kconfig.debug +151 -0

151 lines not on this page · raw message

Sam Ravnborg

2004-04-15 21:30 UTC #14 lore raw

Re: PATCH] Kconfig.debug family

Quoted 8 lines · Randy.Dunlap

On Wed, 14 Apr 2004 16:25:39 -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.

Quoted 8 lines

This patch:
- creates lib/Kconfig.debug for generic kernel debug options
- creates arch/*/Kconfig.debug for arch-specific debug options
- moves KALLSYMS to the generic kernel debug options list
This is a first cut for review/comments. I will double-check the generic options list to see how it needs to be corrected...

Looks good to me - and I really like concentrating this in a more logic common place.

If you could move even more to the generic part it would be fine.

Sam

Marc-Christian Petersen

2004-04-15 21:38 UTC #15 lore raw

Re: PATCH] Kconfig.debug family

In reply to Randy.Dunlap

On Thursday 15 April 2004 22:52, Randy.Dunlap wrote:

Hi Randy,

Quoted 6 lines

This patch:
- creates lib/Kconfig.debug for generic kernel debug options
- creates arch/*/Kconfig.debug for arch-specific debug options
- moves KALLSYMS to the generic kernel debug options list This is a first cut for review/comments. I will double-check the generic options list to see how it needs to be corrected...

I really appreciate it. But I have one comment/suggestion:

We might use lib/Kconfig.debug for all arches and use proper "depend on" if there is stuff for some specific arch only. So we have every debug stuff just in one file. What do you think?

ciao, Marc

Randy.Dunlap

2004-04-15 21:46 UTC #16 lore raw

Re: PATCH] Kconfig.debug family

On Thu, 15 Apr 2004 23:36:30 +0200 Marc-Christian Petersen wrote:

| On Thursday 15 April 2004 22:52, Randy.Dunlap wrote:
| 
| Hi Randy,
| 
| > This patch:
| > - creates lib/Kconfig.debug for generic kernel debug options
| > - creates arch/*/Kconfig.debug for arch-specific debug options
| > - moves KALLSYMS to the generic kernel debug options list
| > This is a first cut for review/comments.  I will double-check
| > the generic options list to see how it needs to be corrected...
| 
| I really appreciate it. But I have one comment/suggestion:
| 
| We might use lib/Kconfig.debug for all arches and use proper "depend on" if 
| there is stuff for some specific arch only. So we have every debug stuff just 
| in one file. What do you think?

Hi Marc,

That sounds like my first version (unposted), which failed IMO. It looks OK if you/user does not enable viewing all flags/options (in [xg]config -- I used xconfig). However, if those options are enabled, you/user sees all config options for all architectures, and I didn't find that acceptable. Feel free to disagree with me and persuade me otherwise, because having them *all* in lib/Kconfig.debug is nicer IMO.

The full patch to do this (against 2.6.5-bk2) is now available at:
http://developer.osdl.org/rddunlap/patches/kconfig_debug_265bk2.patch

if you or anyone else wants to compare how they look.

Thanks,
--
~Randy