Re: kern/39052: assertion "!ISSET(bp->b_cflags, BC_BUSY)" failed

[Manuel Bouyer <bouyer%antioche.eu.org@localhost>]

On Fri, Jun 27, 2008 at 09:17:53PM +0200, Manuel Bouyer wrote:
> > 
> > Now, is it OK to have a dirty buffer in the LRU queue ? It looks like
> > we could recycle dirty buffers from the LRU queue without flushing them
> > to disk first, isn't it ?
> 
> One case where this can happen is though bdwrite(), which sets
> BO_DELWRI before calling brelse(). I added a check for this in brelsel()
> (putting a BO_DELWRI buffer in one of the free lists), and got several
> different traces; but it always has bdwrite() in it.
> 
> Where do we go from here ?

I think I got to the roots of this bug: the KASSERT() is wrong.
It's valid to have a BC_BUSY buffer in the LRU queue, if it's also B_VFLUSH.
This case will be handler a few lines later, and the buffer is removed
from the queue but not recycled.

The attached patches fix it, adds a few more KASSERT, and fixes
checkfreelist() to work when we want to check that something is not on the
free list.

Is it OK to commit ?

-- 
Manuel Bouyer <bouyer%antioche.eu.org@localhost>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Index: kern/vfs_bio.c
===================================================================
RCS file: /cvsroot/src/sys/kern/vfs_bio.c,v
retrieving revision 1.205
diff -u -r1.205 vfs_bio.c
--- kern/vfs_bio.c      17 Jun 2008 19:14:14 -0000      1.205
+++ kern/vfs_bio.c      27 Jun 2008 20:19:27 -0000
@@ -167,7 +167,7 @@
 static void binstailfree(buf_t *, struct bqueue *);
 int count_lock_queue(void); /* XXX */
 #ifdef DEBUG
-static int checkfreelist(buf_t *, struct bqueue *);
+static int checkfreelist(buf_t *, struct bqueue *, int);
 #endif
 static void biointr(void *);
 static void biodone2(buf_t *);
@@ -283,9 +283,9 @@
 }
 
 #ifdef DEBUG
 int debug_verify_freelist = 0;
 static int
-checkfreelist(buf_t *bp, struct bqueue *dp)
+checkfreelist(buf_t *bp, struct bqueue *dp, int ison)
 {
        buf_t *b;
 
@@ -294,10 +294,10 @@
 
        TAILQ_FOREACH(b, &dp->bq_queue, b_freelist) {
                if (b == bp)
-                       return 1;
+                       return ison ? 1 : 0;
        }
 
-       return 0;
+       return ison ? 0 : 1;
 }
 #endif
 
@@ -309,6 +309,7 @@
 binsheadfree(buf_t *bp, struct bqueue *dp)
 {
 
+       KASSERT(mutex_owned(&bufcache_lock));
        KASSERT(bp->b_freelistindex == -1);
        TAILQ_INSERT_HEAD(&dp->bq_queue, bp, b_freelist);
        dp->bq_bytes += bp->b_bufsize;
@@ -319,6 +320,7 @@
 binstailfree(buf_t *bp, struct bqueue *dp)
 {
 
+       KASSERT(mutex_owned(&bufcache_lock));
        KASSERT(bp->b_freelistindex == -1);
        TAILQ_INSERT_TAIL(&dp->bq_queue, bp, b_freelist);
        dp->bq_bytes += bp->b_bufsize;
@@ -335,7 +337,7 @@
 
        KASSERT(bqidx != -1);
        dp = &bufqueues[bqidx];
-       KDASSERT(checkfreelist(bp, dp));
+       KDASSERT(checkfreelist(bp, dp, 1));
        KASSERT(dp->bq_bytes >= bp->b_bufsize);
        TAILQ_REMOVE(&dp->bq_queue, bp, b_freelist);
        dp->bq_bytes -= bp->b_bufsize;
@@ -1007,16 +1009,16 @@
                CLR(bp->b_cflags, BC_VFLUSH);
                if (!ISSET(bp->b_cflags, BC_INVAL|BC_AGE) &&
                    !ISSET(bp->b_flags, B_LOCKED) && bp->b_error == 0) {
-                       KDASSERT(checkfreelist(bp, &bufqueues[BQ_LRU]));
+                       KDASSERT(checkfreelist(bp, &bufqueues[BQ_LRU], 1));
                        goto already_queued;
                } else {
                        bremfree(bp);
                }
        }
 
-       KDASSERT(checkfreelist(bp, &bufqueues[BQ_AGE]));
-       KDASSERT(checkfreelist(bp, &bufqueues[BQ_LRU]));
-       KDASSERT(checkfreelist(bp, &bufqueues[BQ_LOCKED]));
+       KDASSERT(checkfreelist(bp, &bufqueues[BQ_AGE], 0));
+       KDASSERT(checkfreelist(bp, &bufqueues[BQ_LRU], 0));
+       KDASSERT(checkfreelist(bp, &bufqueues[BQ_LOCKED], 0));
 
        if ((bp->b_bufsize <= 0) || ISSET(bp->b_cflags, BC_INVAL)) {
                /*
@@ -1313,7 +1315,7 @@
 
        if ((bp = TAILQ_FIRST(&bufqueues[BQ_AGE].bq_queue)) != NULL ||
            (bp = TAILQ_FIRST(&bufqueues[BQ_LRU].bq_queue)) != NULL) {
-               KASSERT(!ISSET(bp->b_cflags, BC_BUSY));
+               KASSERT(!ISSET(bp->b_cflags, BC_BUSY) || ISSET(bp->b_cflags, 
BC_VFLUSH));
                bremfree(bp);
 
                /* Buffer is no longer on free lists. */
@@ -1369,6 +1371,7 @@
                mutex_enter(&bufcache_lock);
                return (NULL);
        }
+       KASSERT(!ISSET(bp->b_oflags, BO_DELWRI));
 
        vp = bp->b_vp;
        if (bioopsp != NULL)