[patch/2.4] ll_rw_blk stomping on bh state [Re: kernel BUG at journal.c:1732! (2.4.19)]

[patch/2.4] ll_rw_blk stomping on bh state [Re: kernel BUG at journal.c:1732! (2.4.19)]

[Stephen C. Tweedie]

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

Hi,

On Mon, Oct 28, 2002 at 11:13:57AM +0000, Mark Hazell wrote:
 
> I got your addresses from the MAINTAINERS file in the kernel source
> tree, so apologies if i should have sent this somewhere else first.
> 
> Summary: I was copying 700mb of data to my ext3 RAID-1 39gig filesystem
> (2.4gig free according to 'df') yesterday, when the kernel spewed out
> the lines at the bottom of this email.

The start of this is just anonymous disk corruption -- there's no way
I can tell how it happened, but this:

> Oct 28 02:44:14 recondo kernel: attempt to access beyond end of device
> Oct 28 02:44:14 recondo kernel: 09:00: rw=1, want=38708548,
> limit=38708544

looks like you've got a corrupt indirect block on disk somewhere which
is pointing to illegal data blocks off the end of the disk.

That said, ext3 should survive such corruption.  It fails to do so
because of the core block IO code, which in generic_make_request(),
does:

		if (maxsector < count || maxsector - count < sector) {
			/* Yecch */
			bh->b_state &= (1 << BH_Lock) | (1 << BH_Mapped);

and this has the unfortunate side effect of zapping key ext3 metadata
in the buffer state bits, leading up to

> Oct 28 02:44:15 recondo kernel: Assertion failure in
> __journal_remove_journal_he ad() at journal.c:1732: "buffer_jbd(bh)"
> Oct 28 02:44:15 recondo kernel: kernel BUG at journal.c:1732!

when ext3 next comes across the buffer that it knows it owns, but
which has been cleared of ext3 metadata.

The patch below fixes it for me (it's easy to reproduce --- just set
up an ext3 filesystem on an LVM device and then lvreduce it while live
to force half of the filesystem off the end of the device.)

Folks, just which buffer flags do we want to preserve in this case?

--Stephen

[-- Attachment #2: 000-buffer_clearbits.patch --]
[-- Type: text/plain, Size: 554 bytes --]

--- linux-uml-jbddebug/drivers/block/ll_rw_blk.c.=K0001=.orig	Tue Nov 12 14:35:45 2002
+++ linux-uml-jbddebug/drivers/block/ll_rw_blk.c	Tue Nov 12 14:35:45 2002
@@ -1129,7 +1129,9 @@
 
 		if (maxsector < count || maxsector - count < sector) {
 			/* Yecch */
-			bh->b_state &= (1 << BH_Lock) | (1 << BH_Mapped);
+			bh->b_state &= ~((1 << BH_Uptodate) | (1 << BH_Dirty) |
+					 (1 << BH_New) | (1 << BH_Wait_IO) |
+					 (1 << BH_Launder));
 
 			/* This may well happen - the kernel calls bread()
 			   without checking the size of the device, e.g.,

Re: [patch/2.4] ll_rw_blk stomping on bh state [Re: kernel BUG at journal.c:1732! (2.4.19)]

[Andrew Morton]

"Stephen C. Tweedie" wrote:
> 
>                 if (maxsector < count || maxsector - count < sector) {
>                         /* Yecch */
>                         bh->b_state &= (1 << BH_Lock) | (1 << BH_Mapped);
> 
> ...
> 
> Folks, just which buffer flags do we want to preserve in this case?
> 

Why do we want to clear any flags in there at all?  To prevent
a storm of error messages from a buffer which has a silly block
number?

If so, how about setting a new state bit which causes subsequent
IO attempts to silently drop the IO on the floor?

Re: [patch/2.4] ll_rw_blk stomping on bh state [Re: kernel BUG at journal.c:1732! (2.4.19)]

[Stephen C. Tweedie]

Hi,

On Tue, Nov 12, 2002 at 09:57:05AM -0800, Andrew Morton wrote:
> "Stephen C. Tweedie" wrote:
> > 
> >                 if (maxsector < count || maxsector - count < sector) {
> >                         /* Yecch */
> >                         bh->b_state &= (1 << BH_Lock) | (1 << BH_Mapped);
> > ...
> > Folks, just which buffer flags do we want to preserve in this case?

> Why do we want to clear any flags in there at all?  To prevent
> a storm of error messages from a buffer which has a silly block
> number?

That's the only reason I can think of.  Simply scrubbing all the state
bits is totally the wrong way of going about that, of course.
 
> If so, how about setting a new state bit which causes subsequent
> IO attempts to silently drop the IO on the floor?

The only problem I could think of there would be weird interactions
with LVM if somebody lvextends a volume and the buffer suddenly
becomes valid again.  I can't bring myself to care about breaking that
situation. :-)

--Stephen

Re: [patch/2.4] ll_rw_blk stomping on bh state [Re: kernel BUG at journal.c:1732! (2.4.19)]

[Stephen C. Tweedie]

Hi,

On Tue, Nov 12, 2002 at 06:53:45PM +0000, Stephen C. Tweedie wrote:
 
> On Tue, Nov 12, 2002 at 09:57:05AM -0800, Andrew Morton wrote:
> > "Stephen C. Tweedie" wrote:
> > > 
> > >                 if (maxsector < count || maxsector - count < sector) {
> > >                         /* Yecch */
> > >                         bh->b_state &= (1 << BH_Lock) | (1 << BH_Mapped);
> > > ...
> > > Folks, just which buffer flags do we want to preserve in this case?
> 
> > Why do we want to clear any flags in there at all?  To prevent
> > a storm of error messages from a buffer which has a silly block
> > number?
> 
> That's the only reason I can think of.  Simply scrubbing all the state
> bits is totally the wrong way of going about that, of course.

So what's the vote on this?  It's a decision between clearing only the
obvious bit (BH_Dirty) on the one hand, and keeping the code as
unchanged as possible to reduce the possibility of introducing new
bugs.

But frankly I can't see any convincing argument for clearing anything
except the dirty state in this case.

--Stephen

Re: [patch/2.4] ll_rw_blk stomping on bh state [Re: kernel BUG at journal.c:1732! (2.4.19)]

[Andrew Morton]

"Stephen C. Tweedie" wrote:
> 
> Hi,
> 
> On Tue, Nov 12, 2002 at 06:53:45PM +0000, Stephen C. Tweedie wrote:
> 
> > On Tue, Nov 12, 2002 at 09:57:05AM -0800, Andrew Morton wrote:
> > > "Stephen C. Tweedie" wrote:
> > > >
> > > >                 if (maxsector < count || maxsector - count < sector) {
> > > >                         /* Yecch */
> > > >                         bh->b_state &= (1 << BH_Lock) | (1 << BH_Mapped);
> > > > ...
> > > > Folks, just which buffer flags do we want to preserve in this case?
> >
> > > Why do we want to clear any flags in there at all?  To prevent
> > > a storm of error messages from a buffer which has a silly block
> > > number?
> >
> > That's the only reason I can think of.  Simply scrubbing all the state
> > bits is totally the wrong way of going about that, of course.
> 
> So what's the vote on this?  It's a decision between clearing only the
> obvious bit (BH_Dirty) on the one hand, and keeping the code as
> unchanged as possible to reduce the possibility of introducing new
> bugs.
> 
> But frankly I can't see any convincing argument for clearing anything
> except the dirty state in this case.
> 

I'd agree with that.  And the dirty bit will already be cleared, won't it?

Maybe just treat it as an IO error and leave it at that; surely that won't
introduce any problems, given all the testing that has gone into the
error handling paths :)