Subject: Eliminating BQ_AGE -- with broken patch
To: None <tech-kern@netbsd.org>
From: Thor Lancelot Simon <tls@rek.tjls.com>
List: tech-kern
Date: 01/30/2004 13:22:42
Paul suggested eliminating the AGE queue some time ago.  I disagreed with him
then, but looking at things more closely I think that if we keep the current
use of B_AGE, but replace enqueueing on BQ_AGE with releasing the buffer's
memory back to the system, it's the right thing to do.

I have a first cut at this, but clearly I've hosed the locking somehow; the
system gets to the point where it is copying in init and hangs.  I'd really
appreciate it if others could look at the patch below and see if they can
find my mistake.

Index: kern/vfs_bio.c
===================================================================
RCS file: /cvsroot/src/sys/kern/vfs_bio.c,v
retrieving revision 1.114
diff -u -r1.114 vfs_bio.c
--- kern/vfs_bio.c	30 Jan 2004 11:32:16 -0000	1.114
+++ kern/vfs_bio.c	30 Jan 2004 18:15:17 -0000
@@ -141,11 +141,10 @@
 /*
  * Definitions for the buffer free lists.
  */
-#define	BQUEUES		3		/* number of free buffer queues */
+#define	BQUEUES		2		/* number of free buffer queues */
 
 #define	BQ_LOCKED	0		/* super-blocks &c */
 #define	BQ_LRU		1		/* lru, useful buffers */
-#define	BQ_AGE		2		/* rubbish */
 
 TAILQ_HEAD(bqueues, buf) bufqueues[BQUEUES];
 int needbuffer;
@@ -263,7 +262,6 @@
 	LOCK_ASSERT(simple_lock_held(&bqueue_slock));
 
 	KDASSERT(!debug_verify_freelist ||
-		checkfreelist(bp, &bufqueues[BQ_AGE]) ||
 		checkfreelist(bp, &bufqueues[BQ_LRU]) ||
 		checkfreelist(bp, &bufqueues[BQ_LOCKED]) );
 
@@ -781,6 +779,8 @@
 	}
 }
 
+void buf_age(struct buf *);
+
 /*
  * Release a buffer on to the free lists.
  * Described in Bach (p. 46).
@@ -839,7 +839,6 @@
 		}
 	}
 
-  KDASSERT(!debug_verify_freelist || !checkfreelist(bp, &bufqueues[BQ_AGE]));
   KDASSERT(!debug_verify_freelist || !checkfreelist(bp, &bufqueues[BQ_LRU]));
   KDASSERT(!debug_verify_freelist || !checkfreelist(bp, &bufqueues[BQ_LOCKED]));
 
@@ -855,13 +854,8 @@
 			reassignbuf(bp, bp->b_vp);
 			brelvp(bp);
 		}
-		if (bp->b_bufsize <= 0)
-			/* no data */
-			goto already_queued;
-		else
-			/* invalid data */
-			bufq = &bufqueues[BQ_AGE];
-		binsheadfree(bp, bufq);
+		buf_age(bp);
+		goto already_queued;
 	} else {
 		/*
 		 * It has valid data.  Put it on the end of the appropriate
@@ -886,8 +880,13 @@
 				has_deps = (*bioops.io_countdeps)(bp, 0);
 			else
 				has_deps = 0;
-			bufq = has_deps ? &bufqueues[BQ_LRU] :
-			    &bufqueues[BQ_AGE];
+
+			if(has_deps) {
+				bufq = &bufqueues[BQ_LRU];
+			} else {
+				buf_age(bp);
+				goto already_queued;
+			}
 		}
 		binstailfree(bp, bufq);
 	}
@@ -1116,8 +1115,7 @@
 		return (bp);
 	}
 
-	if ((bp = TAILQ_FIRST(&bufqueues[BQ_AGE])) != NULL ||
-	    (bp = TAILQ_FIRST(&bufqueues[BQ_LRU])) != NULL) {
+	if ((bp = TAILQ_FIRST(&bufqueues[BQ_LRU])) != NULL) {
 		simple_lock(&bp->b_interlock);
 		bremfree(bp);
 	} else {
@@ -1186,6 +1184,22 @@
 }
 
 /*
+ * Deal with an invalid buffer (called instead of moving it to BQ_AGE).
+ * Sets its size and count to 0; releases its memory to the pool.
+ * The caller must release the actual buffer header -- currently, this
+ * is called from buf_trim, which brelse()s the buffer, and brelse()
+ * itself, which releases the header at the end of the function.
+ */
+void buf_age(struct buf *bp)
+{
+	bufmem -= bp->b_bufsize;
+        if (bp->b_bufsize > 0) {
+                buf_mrelease(bp->b_data, bp->b_bufsize);
+                bp->b_bcount = bp->b_bufsize = 0;
+        }
+}
+
+/*
  * Attempt to free an aged buffer off the queues.
  * Called at splbio and with queue lock held.
  * Returns the amount of buffer memory freed.
@@ -1210,14 +1224,8 @@
 		simple_unlock(&bqueue_slock);
 		goto out;
 	}
-	size = bp->b_bufsize;
-	bufmem -= size;
 	simple_unlock(&bqueue_slock);
-	if (size > 0) {
-		buf_mrelease(bp->b_data, size);
-		bp->b_bcount = bp->b_bufsize = 0;
-	}
-
+	buf_age(bp);
 out:
 	/* brelse() will return the buffer to the global buffer pool */
 	brelse(bp);
@@ -1562,7 +1570,7 @@
 	struct buf *bp;
 	struct bqueues *dp;
 	int counts[(MAXBSIZE / PAGE_SIZE) + 1];
-	static char *bname[BQUEUES] = { "LOCKED", "LRU", "AGE" };
+	static char *bname[BQUEUES] = { "LOCKED", "LRU" };
 
 	for (dp = bufqueues, i = 0; dp < &bufqueues[BQUEUES]; dp++, i++) {
 		count = 0;
Index: ufs/lfs/lfs_bio.c
===================================================================
RCS file: /cvsroot/src/sys/ufs/lfs/lfs_bio.c,v
retrieving revision 1.76
diff -u -r1.76 lfs_bio.c
--- ufs/lfs/lfs_bio.c	4 Dec 2003 14:57:47 -0000	1.76
+++ ufs/lfs/lfs_bio.c	30 Jan 2004 18:15:18 -0000
@@ -716,12 +716,10 @@
 /*
  * Definitions for the buffer free lists.
  */
-#define BQUEUES		4		/* number of free buffer queues */
+#define BQUEUES		2		/* number of free buffer queues */
  
 #define BQ_LOCKED	0		/* super-blocks &c */
 #define BQ_LRU		1		/* lru, useful buffers */
-#define BQ_AGE		2		/* rubbish */ 
-#define BQ_EMPTY	3		/* buffer headers with no memory */
  
 extern TAILQ_HEAD(bqueues, buf) bufqueues[BQUEUES];
 extern struct simplelock bqueue_slock;