newnfs client and statfs

[Kostik Belousov]

On Tue, May 03, 2011 at 08:15:41PM -0400, Rick Macklem wrote:
> > 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.

Rick, so any final version of the final patch to (re-)test ?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 196 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-fs/attachments/20110504/2f37f76a/attachment.pgp