[RFC PATCH 0/2] avoid clobbering registers with J_ASSERT macro

[RFC PATCH 0/2] avoid clobbering registers with J_ASSERT macro

[Chris Snook]

The J_ASSERT() macro in jbd and jbd2 calls printk() prior to BUG().  While this 
makes it more convenient to read the assertion failure, it also clobbers 
registers, which can sometimes make debugging harder, which is clearly not the 
intended purpose.  I recently banged my head on this myself.

The following patches to jbd and jbd2 enable the printk only if 
CONFIG_JBD[2]_DEBUG is set.  Otherwise, it will simply BUG if the condition is 
violated.  This way test kernels still get the benefit of the J_ASSERT printk, 
while production kernels, which come from a more stable source base where it's 
easier to trace line numbers back to specific lines of code, simply get the BUG, 
with all registers preserved.

This is, of course, not the only way of fixing this problem, but it seems to be 
the least invasive way, which is why I'm proposing these patches.

	-- Chris

[RFC PATCH 1/2] jbd: avoid clobbering registers with J_ASSERT macro

[Chris Snook]

From: Chris Snook <csnook@redhat.com>

Don't printk before BUG in J_ASSERT unless CONFIG_JBD_DEBUG is set.

Signed-off-by: Chris Snook <csnook@redhat.com>


--- linux-2.6.23-rc3-orig/include/linux/jbd.h	2007-07-08 19:32:17.000000000 -0400
+++ linux-2.6.23-rc3-patch/include/linux/jbd.h	2007-08-17 01:43:34.000000000 -0400
@@ -246,7 +246,10 @@ typedef struct journal_superblock_s
 #include <linux/fs.h>
 #include <linux/sched.h>
 
+#ifdef CONFIG_JBD_DEBUG
 #define JBD_ASSERTIONS
+#endif
+
 #ifdef JBD_ASSERTIONS
 #define J_ASSERT(assert)						\
 do {									\
@@ -273,7 +276,7 @@ void buffer_assertion_failure(struct buf
 #endif
 
 #else
-#define J_ASSERT(assert)	do { } while (0)
+#define J_ASSERT(assert)	BUG_ON(!(assert))
 #endif		/* JBD_ASSERTIONS */
 
 #if defined(JBD_PARANOID_IOFAIL)

[RFC PATCH 2/2] jbd2: avoid clobbering registers with J_ASSERT macro

[Chris Snook]

From: Chris Snook <csnook@redhat.com>

Don't printk before BUG in J_ASSERT unless CONFIG_JBD2_DEBUG is set.

Signed-off-by: Chris Snook <csnook@redhat.com>

--- linux-2.6.23-rc3-orig/include/linux/jbd2.h	2007-08-13 03:14:13.000000000 -0400
+++ linux-2.6.23-rc3-patch/include/linux/jbd2.h	2007-08-17 01:44:34.000000000 -0400
@@ -255,7 +255,10 @@ typedef struct journal_superblock_s
 #include <linux/fs.h>
 #include <linux/sched.h>
 
+#ifdef CONFIG_JBD2_DEBUG
 #define JBD_ASSERTIONS
+#endif
+
 #ifdef JBD_ASSERTIONS
 #define J_ASSERT(assert)						\
 do {									\
@@ -282,7 +285,7 @@ void buffer_assertion_failure(struct buf
 #endif
 
 #else
-#define J_ASSERT(assert)	do { } while (0)
+#define J_ASSERT(assert)	BUG_ON(!(assert))
 #endif		/* JBD_ASSERTIONS */
 
 #if defined(JBD_PARANOID_IOFAIL)

Re: [RFC PATCH 0/2] avoid clobbering registers with J_ASSERT macro

[Andrew Morton]

On Fri, 17 Aug 2007 01:54:18 -0400
Chris Snook <csnook@redhat.com> wrote:

> The J_ASSERT() macro in jbd and jbd2 calls printk() prior to BUG().  While this 
> makes it more convenient to read the assertion failure, it also clobbers 
> registers, which can sometimes make debugging harder, which is clearly not the 
> intended purpose.  I recently banged my head on this myself.
> 
> The following patches to jbd and jbd2 enable the printk only if 
> CONFIG_JBD[2]_DEBUG is set.  Otherwise, it will simply BUG if the condition is 
> violated.  This way test kernels still get the benefit of the J_ASSERT printk, 
> while production kernels, which come from a more stable source base where it's 
> easier to trace line numbers back to specific lines of code, simply get the BUG, 
> with all registers preserved.
> 
> This is, of course, not the only way of fixing this problem, but it seems to be 
> the least invasive way, which is why I'm proposing these patches.

How's about we just remove that printk?  Do

#define J_ASSERT(e) BUG_ON(e)?

The rest of the kernel seems to be able to cope with that...

Re: [RFC PATCH 0/2] avoid clobbering registers with J_ASSERT macro

[Chris Snook]

Andrew Morton wrote:
> On Fri, 17 Aug 2007 01:54:18 -0400
> Chris Snook <csnook@redhat.com> wrote:
> 
>> The J_ASSERT() macro in jbd and jbd2 calls printk() prior to BUG().  While this 
>> makes it more convenient to read the assertion failure, it also clobbers 
>> registers, which can sometimes make debugging harder, which is clearly not the 
>> intended purpose.  I recently banged my head on this myself.
>>
>> The following patches to jbd and jbd2 enable the printk only if 
>> CONFIG_JBD[2]_DEBUG is set.  Otherwise, it will simply BUG if the condition is 
>> violated.  This way test kernels still get the benefit of the J_ASSERT printk, 
>> while production kernels, which come from a more stable source base where it's 
>> easier to trace line numbers back to specific lines of code, simply get the BUG, 
>> with all registers preserved.
>>
>> This is, of course, not the only way of fixing this problem, but it seems to be 
>> the least invasive way, which is why I'm proposing these patches.
> 
> How's about we just remove that printk?  Do
> 
> #define J_ASSERT(e) BUG_ON(e)?
> 
> The rest of the kernel seems to be able to cope with that...

Perfectly fine with me.  I proposed the conditional on the grounds that someone 
probably had a purpose for the original J_ASSERT macro at some point in history, 
but if that purpose is long since obsolete, let's just get rid of it.  I'll repost.

	-- Chris

Re: [RFC PATCH 0/2] avoid clobbering registers with J_ASSERT macro

[Stephen C. Tweedie]

Hi,

On Mon, 2007-08-20 at 09:18 -0400, Chris Snook wrote:

> > How's about we just remove that printk?  Do
> > 
> > #define J_ASSERT(e) BUG_ON(e)?
> > 
> > The rest of the kernel seems to be able to cope with that...
> 
> Perfectly fine with me.  I proposed the conditional on the grounds that someone 
> probably had a purpose for the original J_ASSERT macro at some point in history, 
> but if that purpose is long since obsolete, let's just get rid of it.  I'll repost.

It did.  The original J_ASSERT predates BUG() entirely, and was added so
that we got the file/line-no information.  But with the current BUG()
macro, I can't see any reason for J_ASSERT still to try to gather that
information itself.

--Stephen

Re: [RFC PATCH 0/2] avoid clobbering registers with J_ASSERT macro

[Matthew Wilcox]

On Mon, Aug 20, 2007 at 04:22:04PM +0100, Stephen C. Tweedie wrote:
> Hi,
> 
> On Mon, 2007-08-20 at 09:18 -0400, Chris Snook wrote:
> 
> > > How's about we just remove that printk?  Do
> > > 
> > > #define J_ASSERT(e) BUG_ON(e)?

ITYM #define J_ASSERT(e) BUG_ON(!e)

> It did.  The original J_ASSERT predates BUG() entirely, and was added so
> that we got the file/line-no information.  But with the current BUG()
> macro, I can't see any reason for J_ASSERT still to try to gather that
> information itself.

Do you still want to keep J_ASSERT, or should all uses of it be replaced
with BUG_ON?

(to put it another way; if you were writing JBD now, would you add your
own J_ASSERT, or would you just use BUG_ON directly?)

-- 
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [RFC PATCH 0/2] avoid clobbering registers with J_ASSERT macro

[Stephen C. Tweedie]

Hi,

On Mon, 2007-08-20 at 09:46 -0600, Matthew Wilcox wrote:

> Do you still want to keep J_ASSERT, or should all uses of it be replaced
> with BUG_ON?
> 
> (to put it another way; if you were writing JBD now, would you add your
> own J_ASSERT, or would you just use BUG_ON directly?)

The _BH and _JH variants I'd keep, as they allow developers to hook
state debugging printks into the output easily.  J_ASSERT itself... I
wouldn't think I'd bother reinventing BUG_ON these days.

--Stephen