newnfs client and statfs

[Rick Macklem]

> 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