Date: Wed, 4 May 2011 11:18:19 +0300 From: Kostik Belousov <kostikbel@gmail.com> To: Rick Macklem <rmacklem@uoguelph.ca> Cc: fs@freebsd.org Subject: Re: newnfs client and statfs Message-ID: <20110504081819.GM48734@deviant.kiev.zoral.com.ua> In-Reply-To: <2143699515.968680.1304468141505.JavaMail.root@erie.cs.uoguelph.ca> References: <20110503174200.V1050@besplex.bde.org> <2143699515.968680.1304468141505.JavaMail.root@erie.cs.uoguelph.ca>
next in thread | previous in thread | raw e-mail | index | archive | help
--ttU9AGiyjxxpUEJf
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
On Tue, May 03, 2011 at 08:15:41PM -0400, Rick Macklem wrote:
> > On Mon, 2 May 2011, Rick Macklem wrote:
> >=20
> > > 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.)
> >=20
> > Sigh.
> >=20
> > % --- 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 =3D (struct statfs *)statfs;
> > % - nfsquad_t tquad;
> > %
> > % if (nmp->nm_flag & (NFSMNT_NFSV3 | NFSMNT_NFSV4)) {
> > % sbp->f_bsize =3D NFS_FABLKSIZE;
> > % - tquad.qval =3D sfp->sf_tbytes;
> > % - sbp->f_blocks =3D (long)(tquad.qval / ((u_quad_t)NFS_FABLKSIZE));
> > % - tquad.qval =3D sfp->sf_fbytes;
> > % - sbp->f_bfree =3D (long)(tquad.qval / ((u_quad_t)NFS_FABLKSIZE));
> > % - tquad.qval =3D sfp->sf_abytes;
> > % - sbp->f_bavail =3D (long)(tquad.qval / ((u_quad_t)NFS_FABLKSIZE));
> > % - tquad.qval =3D sfp->sf_tfiles;
> > % - sbp->f_files =3D (tquad.lval[0] & 0x7fffffff);
> > % - tquad.qval =3D sfp->sf_ffiles;
> > % - sbp->f_ffree =3D (tquad.lval[0] & 0x7fffffff);
> > % + sbp->f_blocks =3D sfp->sf_tbytes / NFS_FABLKSIZE;
> > % + sbp->f_bfree =3D 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 =3D sfp->sf_abytes / NFS_FABLKSIZE;
> > % + sbp->f_files =3D sfp->sf_tfiles;
> > % + /* Since f_ffree is int64_t, clip it to 63bits. */
> > % + if (sfp->sf_ffiles > (uint64_t)INT64_MAX)
> >=20
> > 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.
> >=20
> 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.)
>=20
> > % + sbp->f_ffree =3D INT64_MAX;
> > % + else
> > % + sbp->f_ffree =3D sfp->sf_ffiles;
> > % } else if ((nmp->nm_flag & NFSMNT_NFSV4) =3D=3D 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 =3D (int32_t)sfp->sf_bsize;
> > % sbp->f_blocks =3D (int32_t)sfp->sf_blocks;
> > % sbp->f_bfree =3D (int32_t)sfp->sf_bfree;
> >=20
> > It won't sign extend, but will propagate bit31 as an unsigned bit. For
> > example, sfp->sf_blocks =3D 0x80000000 becomes sbp->f_blocks =3D
> > 0xFFFFFFFF80000000, which is massively different. Again, omitting the
> > cast gives the correct result if the wire insists on its values being
> > unsigned.
> >=20
> Ok, I'll change the comment.
>=20
> > 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 t=
he
> 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.)
>=20
> 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 ?
--ttU9AGiyjxxpUEJf
Content-Type: application/pgp-signature
Content-Disposition: inline
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (FreeBSD)
iEYEARECAAYFAk3BC8oACgkQC3+MBN1Mb4jluQCfdYUpvlQVu1lY+zV/KsWyr97Q
QCcAnisrVqXE3UdiXE8KiGKoigmYk4zR
=ld0I
-----END PGP SIGNATURE-----
--ttU9AGiyjxxpUEJf--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20110504081819.GM48734>