>Number:         11472
>Category:       kern
>Synopsis:       LFS locked queue accounting not perfect
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sun Nov 12 00:07:00 PST 2000
>Closed-Date:
>Last-Modified:
>Originator:     Jesse Off
>Release:        1.5BETA
>Organization:
>Environment:
NetBSD construct.home 1.5_BETA NetBSD 1.5_BETA (CONSTRUCT) #10: Sun Nov 12 00:11:33 MST 2000     joff@construct.home:/usr/src/sys/arch/i386/compile/CONSTRUCT i386
>Description:
The LFS kernel code tries to keep track of the amount of locked buffers
and the byte total of all locked buffers.  However, it currently fails
to keep an accurate count.  As things exist now, lfs_countlocked() is
called from time to time to recount the locked buffers to adjust these
global variables.  If you happen to have #define DEBUG_LFS, netbsd will
start complaining each time lfs_countlocked sees that the tallys were
wrong (and subsequently fixes them).  

A patch is enclosed below.
>How-To-Repeat:
#define DEBUG_LFS and watch your screen fill up with lfs_countlocked() 
messages.
>Fix:
There were two issues.  One was the fact that we mixed incrementing/
decrementing of locked_queue_bytes with both bp->b_count and 
bp->b_bufsize (b_bufsize is probably what we want).  The other was that 
there were a few places in lfs_segment.c where we locked and unlocked 
buffers without updating the locked_queue_* variables.

There is a comment in lfs_segment.c that I'm not sure applys anymore.
The comment reads that we have to call lfs_countlocked because 
vinvalbuf() removes buffers without our knowing.  My inkling is that
this comment is obsolete because we now have our own buffer invalidation
routine for truncating, but I'm not sure.  It is also my thinking that we 
may be able to get rid of lfs_countlocked() altogether or #define it so 
its only included in DEBUG builds.  However, all I included in this 
patch is to have lfs_countlocked spew its error message with an 
#ifdef DIAGNOSTIC  instead of an #ifdef DEBUG_LFS with the idea being 
that now this error should happen much less (if at all).

Index: lfs_bio.c
===================================================================
RCS file: /cvsroot/syssrc/sys/ufs/lfs/lfs_bio.c,v
retrieving revision 1.23.2.1
diff -u -r1.23.2.1 lfs_bio.c
--- lfs_bio.c   2000/09/14 18:50:17     1.23.2.1
+++ lfs_bio.c   2000/11/12 07:34:32
@@ -575,7 +575,7 @@
                              " buffers locked than exist");
 #endif
        }
-#ifdef DEBUG_LFS
+#ifdef DIAGNOSTIC
        /* Theoretically this function never really does anything */
        if (n != *count)
                printf("lfs_countlocked: adjusted buf count from %d to %d\n",
Index: lfs_segment.c
===================================================================
RCS file: /cvsroot/syssrc/sys/ufs/lfs/lfs_segment.c,v
retrieving revision 1.49.2.3
diff -u -r1.49.2.3 lfs_segment.c
--- lfs_segment.c       2000/09/14 18:50:19     1.49.2.3
+++ lfs_segment.c       2000/11/12 07:34:41
@@ -142,6 +142,9 @@
 int lfs_dostats = 1;
 struct lfs_stats lfs_stats;

+extern int locked_queue_count;
+extern long locked_queue_bytes;
+
 /* op values to lfs_writevnodes */
 #define        VN_REG          0
 #define        VN_DIROP        1
@@ -249,6 +252,10 @@
                                lfs_freebuf(bp);
                        } else {
                                bremfree(bp);
+                               if (bp->b_flags & B_LOCKED) {
+                                       --locked_queue_count;
+                                       locked_queue_bytes -= bp->b_bufsize;
+                               }
                                bp->b_flags &= ~(B_ERROR | B_READ | B_DELWRI |
                                          B_LOCKED | B_GATHERED);
                                bp->b_flags |= B_DONE;
@@ -784,6 +791,8 @@
                sp->idp = ((struct dinode *)bp->b_data) +
                        (sp->ninodes % INOPB(fs));
        if(gotblk) {
+               ++locked_queue_count;
+               locked_queue_bytes += bp->b_bufsize;
                bp->b_flags |= B_LOCKED;
                brelse(bp);
        }
@@ -1244,8 +1253,6 @@
        struct lfs *fs;
        struct segment *sp;
 {
-       extern int locked_queue_count;
-       extern long locked_queue_bytes;
        struct buf **bpp, *bp, *cbp, *newbp;
        SEGUSE *sup;
        SEGSUM *ssp;
@@ -1381,6 +1388,10 @@
                                        splx(s);
                                }
                        } else {
+                               if (bp->b_flags & B_LOCKED) {
+                                       --locked_queue_count;
+                                       locked_queue_bytes -= bp->b_bufsize;
+                               }
                                bp->b_flags &= ~(B_ERROR | B_READ | B_DELWRI |
                                                 B_LOCKED | B_GATHERED);
                                if (bp->b_flags & B_CALL)
Index: lfs_inode.c
===================================================================
RCS file: /cvsroot/syssrc/sys/ufs/lfs/lfs_inode.c,v
retrieving revision 1.37.2.2
diff -u -r1.37.2.2 lfs_inode.c
--- lfs_inode.c 2000/11/01 03:56:53     1.37.2.2
+++ lfs_inode.c 2000/11/12 07:34:45
@@ -695,7 +695,7 @@
                if (bp->b_flags & B_LOCKED) {
                        bp->b_flags &= ~B_LOCKED;
                        --locked_queue_count;
-                       locked_queue_bytes -= bp->b_bcount;
+                       locked_queue_bytes -= bp->b_bufsize;
                }
                brelse(bp);
        }
@@ -722,7 +722,7 @@
                if (bp->b_flags & B_LOCKED) {
                        bp->b_flags &= ~B_LOCKED;
                        --locked_queue_count;
-                       locked_queue_bytes -= bp->b_bcount;
+                       locked_queue_bytes -= bp->b_bufsize;
                }
                brelse(bp);
        }

>Release-Note:
>Audit-Trail:
>Unformatted: