Date: Tue, 20 Jan 2009 11:43:22 +0100 From: Christoph Mallon <christoph.mallon@gmx.de> To: FreeBSD Current <freebsd-current@freebsd.org> Cc: uwe@grohnwaldt.eu, FreeBSD Hackers <freebsd-hackers@freebsd.org>, miwi@freebsd.org Subject: Re: Question about panic in brelse() Message-ID: <4975AACA.3050906@gmx.de> In-Reply-To: <496D94FD.9030300@gmx.de> References: <496D94FD.9030300@gmx.de>
next in thread | previous in thread | raw e-mail | index | archive | help
Sadly I got no response about the panic problem so far. I investigated
further and I came to the conclusion, that there are at least two
problems/bugs in brelse(). Here are my findings. All lines refer to
sys/kern/vfs_bio.c at r183754, which is the most recent version of this
file.
Below is the KASSERT(), which triggers in bundirty() (which is called by
brelse()):
KASSERT(bp->b_flags & B_REMFREE || bp->b_qindex == QUEUE_NONE,
("bundirty: buffer %p still on queue %d", bp, bp->b_qindex));
So this assertion triggers, if neither B_REMFREE is set nor the buffer
is in QUEUE_NONE.
Now lets have a look at brelse() before the call to bundirty() happens.
In line 1325 we find:
if (bp->b_flags & B_REMFREE)
bremfreel(bp);
In bremfreel() B_REMFREE is cleared (line 684). So after this B_REMFREE
is guaranteed to be clear and so the first part of the triggered
KASSERT() will be false.
Now for the second part, i.e. QUEUE_NONE: At line 1331 starts an
if-cascade. No matter which case of this cascade is entered, b_qindex is
set and it especially in no case it is set to QUEUE_NONE. so after this
if-cascade it is guaranteed that b_qindex is *not* QUEUE_NONE.
So when we arrive at line 1373ff at the problematic call to bundirty(),
which contains the KASSERT(), it is guaranteed that neither B_REMFREE is
set nor the buffer is in QUEUE_NONE and therefore the KASSERT() will be
triggered. So we have a serious problem here: When the call is done, it
*will* fail.
This is one part of the problem. I have no solution, I don't know what
would be correct here. Further, I looked at r93707 when this call to
bundirty() was added. The situation was basically the same back then,
i.e. the buffer could not be in QUEUE_NONE (the check for B_REMFREE did
not exist back then, so the KASSERT() was more strict). So this change
seems to wrong all along or at least dubious. The commit log of r93707
on the other hand claims that it solved a serious problem. If this was
tested back then, it must have been done without INVARIANTS. It seems to
be, that it was a hot fix - mind the MFC time of 1 day.
Now for the second problem. As stated in the original mail, there is a
buffer for which a write attempt failed:
> b_iocmd = BIO_WRITE
> b_ioflags = BIO_ERROR
> b_error = EIO
> b_flags = B_NOCACHE
Because the error ist "just" an EIO, the buffer is re-dirtied and
BIO_ERROR is cleared (line 1144ff). So writing the buffer should be
tried again.
But later in line 1342ff it is determined that the buffer contains junk,
because B_NOCACHE is set so B_INVAL is also set. This leads to entering
the path in line 1373, which then triggers the KASSERT().
On the other hand, the buffer does not contain junk, because it should
be retried to be written! It only should be not kept around after the
write was successful (or is considered to be failed permamently). So I
think testing B_NOCACHE here alone is to weak and the condition has to
be modified. I propose this change:
@@ -1340,7 +1340,8 @@
}
TAILQ_INSERT_HEAD(&bufqueues[bp->b_qindex], bp, b_freelist);
/* buffers with junk contents */
- } else if (bp->b_flags & (B_INVAL | B_NOCACHE | B_RELBUF) ||
+ } else if (bp->b_flags & (B_INVAL | B_RELBUF) ||
+ ((bp->b_flags & (B_NOCACHE | B_DELWRI)) == B_NOCACHE)
(bp->b_ioflags & BIO_ERROR)) {
bp->b_flags |= B_INVAL;
bp->b_xflags &= ~(BX_BKGRDWRITE | BX_ALTDATA);
This only enters the path to invalidate the buffer in case of B_NOCACHE
being set, if B_DELWRI is *not* set.
Can somebody reconstruct my analysis and confirm/reject it?
Does my proposed solution for the second problem look sensible?
Are the two problems related, i.e. can the first problem only occur
because the second problem exists?
What about the change in r93707 in general? Is it wrong? Does my
proposal remove the cause for the change done in r93707 or are there
other circumstances by which the situation can be triggered?
Hopefully somebody can shed some more light on this.
Christoph
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4975AACA.3050906>