From owner-freebsd-fs@FreeBSD.ORG Wed May 4 00:15:43 2011 Return-Path: Delivered-To: fs@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id D60AD1065670 for ; Wed, 4 May 2011 00:15:42 +0000 (UTC) (envelope-from rmacklem@uoguelph.ca) Received: from esa-jnhn.mail.uoguelph.ca (esa-jnhn.mail.uoguelph.ca [131.104.91.44]) by mx1.freebsd.org (Postfix) with ESMTP id 7D8D08FC14 for ; Wed, 4 May 2011 00:15:42 +0000 (UTC) X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: ApwEAH6ZwE2DaFvO/2dsb2JhbACEUaJGiHKreJEdgSqDV4EBBI8Yjk4 X-IronPort-AV: E=Sophos;i="4.64,312,1301889600"; d="scan'208";a="120327109" Received: from erie.cs.uoguelph.ca (HELO zcs3.mail.uoguelph.ca) ([131.104.91.206]) by esa-jnhn-pri.mail.uoguelph.ca with ESMTP; 03 May 2011 20:15:41 -0400 Received: from zcs3.mail.uoguelph.ca (localhost.localdomain [127.0.0.1]) by zcs3.mail.uoguelph.ca (Postfix) with ESMTP id 8AABCB3F24; Tue, 3 May 2011 20:15:41 -0400 (EDT) Date: Tue, 3 May 2011 20:15:41 -0400 (EDT) From: Rick Macklem To: Bruce Evans Message-ID: <2143699515.968680.1304468141505.JavaMail.root@erie.cs.uoguelph.ca> In-Reply-To: <20110503174200.V1050@besplex.bde.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Originating-IP: [172.17.91.203] X-Mailer: Zimbra 6.0.10_GA_2692 (ZimbraWebClient - IE7 (Win)/6.0.10_GA_2692) Cc: fs@FreeBSD.org Subject: Re: newnfs client and statfs X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 04 May 2011 00:15:43 -0000 > On Mon, 2 May 2011, Rick Macklem wrote: > > > I have attached a version of the patch that I intend to commit > > unless it doesn't work for Kostik's test case. Kostik, could > > you please test this one. > > > > Yes, Bruce, I realize you won't like it, but I > > have put some comments in it > > to try and clarify why it is coded the way it is. > > (The arithmetic seems to work the way I would expect it to for > > i386, which is the only arch I have for testing.) > > Sigh. > > % --- fs/nfsclient/nfs_clport.c.sav 2011-04-30 20:16:39.000000000 > -0400 > % +++ fs/nfsclient/nfs_clport.c 2011-05-02 19:32:31.000000000 -0400 > % @@ -838,21 +838,33 @@ void > % nfscl_loadsbinfo(struct nfsmount *nmp, struct nfsstatfs *sfp, void > *statfs) > % { > % struct statfs *sbp = (struct statfs *)statfs; > % - nfsquad_t tquad; > % > % if (nmp->nm_flag & (NFSMNT_NFSV3 | NFSMNT_NFSV4)) { > % sbp->f_bsize = NFS_FABLKSIZE; > % - tquad.qval = sfp->sf_tbytes; > % - sbp->f_blocks = (long)(tquad.qval / ((u_quad_t)NFS_FABLKSIZE)); > % - tquad.qval = sfp->sf_fbytes; > % - sbp->f_bfree = (long)(tquad.qval / ((u_quad_t)NFS_FABLKSIZE)); > % - tquad.qval = sfp->sf_abytes; > % - sbp->f_bavail = (long)(tquad.qval / ((u_quad_t)NFS_FABLKSIZE)); > % - tquad.qval = sfp->sf_tfiles; > % - sbp->f_files = (tquad.lval[0] & 0x7fffffff); > % - tquad.qval = sfp->sf_ffiles; > % - sbp->f_ffree = (tquad.lval[0] & 0x7fffffff); > % + sbp->f_blocks = sfp->sf_tbytes / NFS_FABLKSIZE; > % + sbp->f_bfree = sfp->sf_fbytes / NFS_FABLKSIZE; > % + /* > % + * Although sf_abytes is uint64_t and f_bavail is int64_t, > % + * the value after dividing by NFS_FABLKSIZE is small > % + * enough that it will fit in 63bits, so it is ok to > % + * assign it to f_bavail without fear that it will become > % + * negative. > % + */ > % + sbp->f_bavail = sfp->sf_abytes / NFS_FABLKSIZE; > % + sbp->f_files = sfp->sf_tfiles; > % + /* Since f_ffree is int64_t, clip it to 63bits. */ > % + if (sfp->sf_ffiles > (uint64_t)INT64_MAX) > > This cast has no effect. INT64_MAX has type int64_t. sf_ffiles has > uint64_t. The default binary promotions cause both types to be > promoted > to the minimally larger common type. This type is uint64_t. Thus > INT64_MAX is converted automatically to the correct type. > Yea, I didn't tthink the cast mattered and it didn't affect the outcome for my little userland test program, so I'll take it out. (I was trying the "play it safe", but if you say it doesn't matter, I believe you.) > % + sbp->f_ffree = INT64_MAX; > % + else > % + sbp->f_ffree = sfp->sf_ffiles; > % } else if ((nmp->nm_flag & NFSMNT_NFSV4) == 0) { > % + /* > % + * The type casts to (int32_t) ensure that this code is > % + * compatible with the old NFS client, in that it will > % + * sign extend a value with bit31 set. This may or may > % + * not be correct for NFSv2, but since it is a legacy > % + * environment, I'd rather retain backwards compatibility. > % + */ > % sbp->f_bsize = (int32_t)sfp->sf_bsize; > % sbp->f_blocks = (int32_t)sfp->sf_blocks; > % sbp->f_bfree = (int32_t)sfp->sf_bfree; > > It won't sign extend, but will propagate bit31 as an unsigned bit. For > example, sfp->sf_blocks = 0x80000000 becomes sbp->f_blocks = > 0xFFFFFFFF80000000, which is massively different. Again, omitting the > cast gives the correct result if the wire insists on its values being > unsigned. > Ok, I'll change the comment. > The result is only backwards compatible with relatively recent FreeBSD > nfs clients. All FreeBSD clients are completely broken if bit31 is > set, and compatibility with this brokenness is not useful (but as I > pointed out in another reply, we would never have seen the broken case > when the old clients weren't old, since it takes a server file system > size of about 32TB for bit 31 to be set). Well, the last legitimate use of the FreeBSD NFSv2 client was a diskless root fs stored on a non-FreeBSD NFS server (because pxeboot didn't know the correct file handle size). Since this is now fixed, there really isn't any use for the NFSv2 client, as far as I know. Given that and the fact that no one is complaining about it being broken, I feel it should just be left alone. (Or remain "bug compatible" with the regular NFS client, if you prefer.) I'm afraid I have other things to work on and just don't see changing NFSv2 (a 1985 protocol superceded by NFSv3 in 1994) a priority, rick. > The details of the > brokenness > vary: > > Net/2, FreeBSD-1, 4.4BSD-Lite, FreeBSD-[2-4]: > f_blocks was plain long: > if long is 32 bits, then sfp->sf_blocks = 0x80000000 becomes > sbp->f_blocks = -0x7fffffff - 1 (LONG_MIN) > if long is 64 bits, then sfp->sf_blocks = 0x80000000 becomes > sbp->f_blocks = -0x80000000L (INT32_MIN (same as 32-bit LONG_MIN) > > FreeBSD-current after 2003/11/12, FreeBSD-[5-9]: > f_blocks is now uint64_t: > changing it (and others from a signed type to an unsigned type mainly > gave lots of sign extension bugs, including here. The bugs remain > mostly unfixed. > sfp->sf_blocks = 0x80000000 becomes > sbp->f_blocks = 0xFFFFFFFF80000000 ((uint64_t)INT32_MIN) on all > arches. > > Neither of the garbage values INT32_MIN, ((uint64_t)INT_MIN) gives > useful > behaviour. The former is negative, though the wire value cannot be > negative > (not sure about this for v2). Applications that are naive enough to > believe > this value should assume that the the file system has a negative size > and > never try to write anything. The latter is enormous and positive. If > the > wire count really is 0x80000000, then that is already very large, so > believing that the value is 0xFFFFFFFF80000000 should make little > difference. > > The bugs are a little different for signed fields like f_bavail. Now > there > are no sign extension bugs or version-dependent misbehaviours. There > are > just overflow bugs in the bogus casts. (int32_t)0x80000000 overflows > to > INT32_MIN (only on 2's complement machines, but no others are > supported), > and assignment to sbp->f_bavail doesn't change this garbage value. Now > the bugs are even further off, since it takes about a 400 TB ffs > server file > system to reach them. (400 TB with 8% minfree gives a 32TB reserve for > root. After using all 32TB of this reserve, there would be -32TB > available > for non-root. -32TB is INT32_MIN in 16K-blocks.) > > Bruce