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:
(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.