From owner-freebsd-bugs@FreeBSD.ORG Tue Sep 9 10:15:27 2003 Return-Path: Delivered-To: freebsd-bugs@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 63DF716A4C0; Tue, 9 Sep 2003 10:15:27 -0700 (PDT) Received: from mailman.zeta.org.au (mailman.zeta.org.au [203.26.10.16]) by mx1.FreeBSD.org (Postfix) with ESMTP id 029E443FFB; Tue, 9 Sep 2003 10:15:23 -0700 (PDT) (envelope-from bde@zeta.org.au) Received: from gamplex.bde.org (katana.zip.com.au [61.8.7.246]) by mailman.zeta.org.au (8.9.3p2/8.8.7) with ESMTP id DAA18627; Wed, 10 Sep 2003 03:15:08 +1000 Date: Wed, 10 Sep 2003 03:15:07 +1000 (EST) From: Bruce Evans X-X-Sender: bde@gamplex.bde.org To: Hendrik Scholz In-Reply-To: <200309081917.h88JHb0D057921@goanna.lan.raisdorf.net> Message-ID: <20030910030313.V880@gamplex.bde.org> References: <200309081917.h88JHb0D057921@goanna.lan.raisdorf.net> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII cc: freebsd-bugs@freebsd.org cc: FreeBSD-gnats-submit@freebsd.org cc: peter@freebsd.org Subject: Re: bin/56606: df cannot handle 2TB NFS volumes X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 09 Sep 2003 17:15:27 -0000 On Mon, 8 Sep 2003, Hendrik Scholz wrote: > >Description: > Mounting a 1.8TB NFS volume shows negative values: > $ \df /mnt/foo > Filesystem 1K-blocks Used Avail Capacity Mounted on > x.x.x.x:/spool1 -222502944 -463076980 240574036 208% /mnt/foo > $ mount | grep foo > x.x.x.x:/spool1 on /mnt/foo (nfs) > > It should read (copied from Linux box): > x.x.x.x:/spool1 1924980704 1684406656 240574048 88% /news/spool1 I think this was supposed to have been fixed in nfs_vfsops.c nfs_vfsops.c, but the fix was buggy and was turned into verbose nonsense that happens to have no effect in rev.1.135. I sent the following to the author of the fix 3 weeks ago but received no reply. It contains a hopefully correct fix. This has not been tested with multi-TB filesystems but hasn't caused any problems with ones in the 4-13 GB range. [Old mail] % Index: nfs_vfsops.c % =================================================================== % RCS file: /home/ncvs/src/sys/nfsclient/nfs_vfsops.c,v % retrieving revision 1.134 % retrieving revision 1.135 % diff -u -2 -r1.134 -r1.135 % --- nfs_vfsops.c 29 Apr 2003 13:36:04 -0000 1.134 % +++ nfs_vfsops.c 19 May 2003 22:35:00 -0000 1.135 % @@ -38,5 +38,5 @@ % % #include % -__FBSDID("$FreeBSD: src/sys/nfsclient/nfs_vfsops.c,v 1.134 2003/04/29 13:36:04 kan Exp $"); % +__FBSDID("$FreeBSD: src/sys/nfsclient/nfs_vfsops.c,v 1.135 2003/05/19 22:35:00 peter Exp $"); % % #include "opt_bootp.h" % @@ -278,13 +278,16 @@ % sbp->f_bsize = bsize; % tquad = fxdr_hyper(&sfp->sf_tbytes); % - if ((tquad / bsize) > LONG_MAX) % + if (((long)(tquad / bsize) > LONG_MAX) || % + ((long)(tquad / bsize) < LONG_MIN)) % continue; % sbp->f_blocks = tquad / bsize; % tquad = fxdr_hyper(&sfp->sf_fbytes); % - if ((tquad / bsize) > LONG_MAX) % + if (((long)(tquad / bsize) > LONG_MAX) || % + ((long)(tquad / bsize) < LONG_MIN)) % continue; % sbp->f_bfree = tquad / bsize; % tquad = fxdr_hyper(&sfp->sf_abytes); % - if ((tquad / bsize) > LONG_MAX) % + if (((long)(tquad / bsize) > LONG_MAX) || % + ((long)(tquad / bsize) < LONG_MIN)) % continue; % sbp->f_bavail = tquad / bsize; Values of type long have the remarkable property of always being between LONG_MIN and LONG_MAX, so this change is just a verbose way of backing out rev.1.33. Attempted fix: %%% Index: nfs_vfsops.c =================================================================== RCS file: /home/ncvs/src/sys/nfsclient/nfs_vfsops.c,v retrieving revision 1.136 diff -u -2 -r1.136 nfs_vfsops.c --- nfs_vfsops.c 12 Jun 2003 20:48:38 -0000 1.136 +++ nfs_vfsops.c 16 Aug 2003 04:01:37 -0000 @@ -239,5 +239,5 @@ struct mbuf *mreq, *mrep, *md, *mb; struct nfsnode *np; - u_quad_t tquad; + quad_t tquad; int bsize; @@ -270,19 +270,19 @@ for (bsize = NFS_FABLKSIZE; ; bsize *= 2) { sbp->f_bsize = bsize; - tquad = fxdr_hyper(&sfp->sf_tbytes); - if (((long)(tquad / bsize) > LONG_MAX) || - ((long)(tquad / bsize) < LONG_MIN)) + tquad = (quad_t)fxdr_hyper(&sfp->sf_tbytes) / bsize; + if (bsize <= INT_MAX / 2 && + (tquad > LONG_MAX || tquad < LONG_MIN)) continue; - sbp->f_blocks = tquad / bsize; - tquad = fxdr_hyper(&sfp->sf_fbytes); - if (((long)(tquad / bsize) > LONG_MAX) || - ((long)(tquad / bsize) < LONG_MIN)) + sbp->f_blocks = tquad; + tquad = (quad_t)fxdr_hyper(&sfp->sf_fbytes) / bsize; + if (bsize <= INT_MAX / 2 && + (tquad > LONG_MAX || tquad < LONG_MIN)) continue; - sbp->f_bfree = tquad / bsize; - tquad = fxdr_hyper(&sfp->sf_abytes); - if (((long)(tquad / bsize) > LONG_MAX) || - ((long)(tquad / bsize) < LONG_MIN)) + sbp->f_bfree = tquad; + tquad = (quad_t)fxdr_hyper(&sfp->sf_abytes) / bsize; + if (bsize <= INT_MAX / 2 && + (tquad > LONG_MAX || tquad < LONG_MIN)) continue; - sbp->f_bavail = tquad / bsize; + sbp->f_bavail = tquad; sbp->f_files = (fxdr_unsigned(int32_t, sfp->sf_tfiles.nfsuquad[1]) & 0x7fffffff); %%% This has not been tested at runtime. Rev.1.33 apparently didn't work essentially because of overflow (-1 became UQUAD_MAX, and dividing that by bsize made a mess). The patch assumes that values larger than QUAD_MAX really mean negative values. The patch also ensures that doubling bsize never overflows. bsize could be limited to a less preposterous size than INT_MAX. The patch also fixes some style bugs (excessive parentheses). It's interesting that gcc doesn't warn about this. It now seems to warn only about null comparisons of chars with their limits. I think it recently expanded this warning from one involving only one-sided limits of chars. ISTR looking at the code for this more than 10 years ago. It could have warned about all sorts of null comparisons but intentially only gave warnings for a couple of cases to limit the warnings. Hmm, there are more sloppy conversions here: % sbp->f_files = (fxdr_unsigned(int32_t, % sfp->sf_tfiles.nfsuquad[1]) & 0x7fffffff); % sbp->f_ffree = (fxdr_unsigned(int32_t, % sfp->sf_ffiles.nfsuquad[1]) & 0x7fffffff); Looks like it blindly truncates to a 31-bit (positive) int. f_ffree could easily be >= 2^31. Truncating to 2^31-1 or a documented weird (negative) value meaning "overflow" would be better. [End of old mail] Bruce