NetBSD Problem Report #51264

From www@NetBSD.org  Wed Jun 22 18:14:09 2016
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(Client CN "mail.netbsd.org", Issuer "Postmaster NetBSD.org" (verified OK))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 8C8117A110
	for <gnats-bugs@gnats.NetBSD.org>; Wed, 22 Jun 2016 18:14:09 +0000 (UTC)
Message-Id: <20160622181407.5BA3D7AAA4@mollari.NetBSD.org>
Date: Wed, 22 Jun 2016 18:14:07 +0000 (UTC)
From: joseyluis@gmail.com
Reply-To: joseyluis@gmail.com
To: gnats-bugs@NetBSD.org
Subject: Fix src/sbin/fsck_lfs/bufcache.c
X-Send-Pr-Version: www-1.0

>Number:         51264
>Category:       bin
>Synopsis:       Fix src/sbin/fsck_lfs/bufcache.c
>Confidential:   no
>Severity:       critical
>Priority:       medium
>Responsible:    bin-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Jun 22 18:15:00 +0000 2016
>Closed-Date:    Fri Mar 31 07:22:58 +0000 2023
>Last-Modified:  Fri Mar 31 07:22:58 +0000 2023
>Originator:     Jose Luis Rodriguez Garcia
>Release:        NetBSD current
>Organization:
>Environment:
>Description:
Two bugs:

1- In function bufrehash,the entries are inserted in the hash table using the old hash function, because the hasmask hasn't been updated.

2- In function bufrehash, it must be :

if (max < 0 || max <= hashmax)

instead of:

if (max < 0 || max < hashmax), 

because in the case of max=hashmax both original an new hash table have the same size. It is a waste to reserve more memory and do the copy.

I think that this fix must be pulled o NetBSD 7 and NetBSD 6 (mainly case 1), because it can affect to the cleaner daemon and fsck_lfs.
>How-To-Repeat:

>Fix:
pc2$ diff -u bufcache.c.orig bufcache.c
--- bufcache.c.orig	2016-06-21 21:54:30.000000000 +0200
+++ bufcache.c	2016-06-22 19:57:06.000000000 +0200
@@ -101,17 +101,17 @@
 /* Widen the hash table. */
 void bufrehash(int max)
 {
-	int i, newhashmax, newhashmask;
+	int i, newhashmax;
 	struct ubuf *bp, *nbp;
 	struct bufhash_struct *np;

-	if (max < 0 || max < hashmax)
+	if (max < 0 || max <= hashmax)
 		return;

 	/* Round up to a power of two */
 	for (newhashmax = 1; newhashmax < max; newhashmax <<= 1)
 		;
-	newhashmask = newhashmax - 1;
+	hashmask = newhashmax - 1;

 	/* Allocate new empty hash table, if we can */
 	np = emalloc(newhashmax * sizeof(*bufhash));
@@ -134,7 +134,6 @@
 	free(bufhash);
 	bufhash = np;
 	hashmax = newhashmax;
-	hashmask = newhashmask;
 }

 /* Print statistics of buffer cache usage */

>Release-Note:

>Audit-Trail:
From: "David A. Holland" <dholland@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/51264 CVS commit: src/sbin/fsck_lfs
Date: Sun, 31 Jul 2016 18:27:27 +0000

 Module Name:	src
 Committed By:	dholland
 Date:		Sun Jul 31 18:27:27 UTC 2016

 Modified Files:
 	src/sbin/fsck_lfs: bufcache.c

 Log Message:
 PR 51264 Jose Luis Rodriguez Garcia: lfs userland bufcache rehash is broken


 To generate a diff of this commit:
 cvs rdiff -u -r1.15 -r1.16 src/sbin/fsck_lfs/bufcache.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: bin/51264: Fix src/sbin/fsck_lfs/bufcache.c
Date: Sun, 31 Jul 2016 18:43:59 +0000

 On Wed, Jun 22, 2016 at 06:15:00PM +0000, joseyluis@gmail.com wrote:
  > I think that this fix must be pulled to NetBSD 7 and NetBSD 6
  > (mainly case 1), because it can affect to the cleaner daemon and
  > fsck_lfs.

 The impact is limited (I think zero actually) because none of the
 tools resize more than once and they do it during initialization
 before there are buffers to rehash. Do you have a case in mind where
 it breaks?

 I'm not opposed to pulling it up, I'm just not sure it's actually
 necessary.

 -- 
 David A. Holland
 dholland@netbsd.org

From: Jose Luis Rodriguez Garcia <joseyluis@gmail.com>
To: gnats-bugs@netbsd.org
Cc: gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Subject: Re: bin/51264: Fix src/sbin/fsck_lfs/bufcache.c
Date: Mon, 1 Aug 2016 00:22:48 +0200

 I have seen that it is called by the cleaner daemon, but I don't know
 in which cases the cleaner daemon do a rehash (I haven't read any of
 the cleaner code yet).

 Do as you think it is better

 On Sun, Jul 31, 2016 at 8:45 PM, David Holland <dholland-bugs@netbsd.org> wrote:
 > The following reply was made to PR bin/51264; it has been noted by GNATS.
 >
 > From: David Holland <dholland-bugs@netbsd.org>
 > To: gnats-bugs@NetBSD.org
 > Cc:
 > Subject: Re: bin/51264: Fix src/sbin/fsck_lfs/bufcache.c
 > Date: Sun, 31 Jul 2016 18:43:59 +0000
 >
 >  On Wed, Jun 22, 2016 at 06:15:00PM +0000, joseyluis@gmail.com wrote:
 >   > I think that this fix must be pulled to NetBSD 7 and NetBSD 6
 >   > (mainly case 1), because it can affect to the cleaner daemon and
 >   > fsck_lfs.
 >
 >  The impact is limited (I think zero actually) because none of the
 >  tools resize more than once and they do it during initialization
 >  before there are buffers to rehash. Do you have a case in mind where
 >  it breaks?
 >
 >  I'm not opposed to pulling it up, I'm just not sure it's actually
 >  necessary.
 >
 >  --
 >  David A. Holland
 >  dholland@netbsd.org
 >

From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: bin/51264: Fix src/sbin/fsck_lfs/bufcache.c
Date: Mon, 1 Aug 2016 00:04:54 +0000

 On Sun, Jul 31, 2016 at 10:25:00PM +0000, Jose Luis Rodriguez Garcia wrote:
  >  I have seen that it is called by the cleaner daemon, but I don't know
  >  in which cases the cleaner daemon do a rehash (I haven't read any of
  >  the cleaner code yet).

 It is called when it reloads the ifile, which it does before cleaning
 every segment (apparently) - this will actually resize it since it
 calls bufinit() with 4, but it'll do it before it does any real work.
 And since the ifile doesn't grow on the fly unless you try to do an
 online resize, it probably won't have much effect.

 however, I misread that earlier.

 I now think pulling it up is worthwhile.

 -- 
 David A. Holland
 dholland@netbsd.org

State-Changed-From-To: open->needs-pullups
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Mon, 01 Aug 2016 00:05:16 +0000
State-Changed-Why:
.


From: Jose Luis Rodriguez Garcia <joseyluis@gmail.com>
To: gnats-bugs@netbsd.org
Cc: gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Subject: Re: bin/51264: Fix src/sbin/fsck_lfs/bufcache.c
Date: Mon, 15 Aug 2016 17:28:21 +0200

 I think that it is a very usual situation.
 Reading the code (I copy it from lfs_cleanerd.c file), it is called
 when the ifile grows.
 Ifile grows when the number of inodes increases. (Margo paper, page
 10, first paragraph)
 (file=inode map + segment usage table).

         /* If Ifile is larger than buffer cache, rehash */
         fstat(fs->clfs_ifilefd, &st);
         if (st.st_size / lfs_sb_getbsize(fs) > hashmax) {
                 ohashmax = hashmax;
                 bufrehash(st.st_size / lfs_sb_getbsize(fs));
                 dlog("%s: resized buffer hash from %d to %d",
                      lfs_sb_getfsmnt(fs), ohashmax, hashmax);
         }



 On Mon, Aug 1, 2016 at 2:05 AM, David Holland <dholland-bugs@netbsd.org> wrote:
 >  It is called when it reloads the ifile, which it does before cleaning
 >  every segment (apparently) - this will actually resize it since it
 >  calls bufinit() with 4, but it'll do it before it does any real work.
 >  And since the ifile doesn't grow on the fly unless you try to do an
 >  online resize, it probably won't have much effect.
 >
 >  however, I misread that earlier.
 >
 >  I now think pulling it up is worthwhile.
 >
 >  --
 >  David A. Holland
 >  dholland@netbsd.org
 >

State-Changed-From-To: needs-pullups->closed
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Fri, 31 Mar 2023 07:22:58 +0000
State-Changed-Why:
netbsd-7 is now EOL, pullups no longer needed


>Unformatted:

NetBSD Home
NetBSD PR Database Search

(Contact us) $NetBSD: query-full-pr,v 1.47 2022/09/11 19:34:41 kim Exp $
$NetBSD: gnats_config.sh,v 1.9 2014/08/02 14:16:04 spz Exp $
Copyright © 1994-2023 The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.