Date: Fri, 18 Jan 2013 12:12:41 -0500 From: John Baldwin <jhb@freebsd.org> To: Bruce Evans <brde@optusnet.com.au> Cc: freebsd-fs@freebsd.org, Rick Macklem <rmacklem@freebsd.org> Subject: Re: [PATCH] Use vfs_timestamp() instead of getnanotime() in NFS Message-ID: <201301181212.41321.jhb@freebsd.org> In-Reply-To: <20130118165934.K1042@besplex.bde.org> References: <460209850.2108683.1358475815866.JavaMail.root@erie.cs.uoguelph.ca> <20130118165934.K1042@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Friday, January 18, 2013 1:19:29 am Bruce Evans wrote:
> On Thu, 17 Jan 2013, Rick Macklem wrote:
>
> > John Baldwin wrote:
> >> On Tuesday, January 15, 2013 2:58:42 pm John Baldwin wrote:
> >>> Fixing NFS to properly use vfs_timestamp() seems to be a larger
> >>> project.
> >>
> >> Actually, I have a patch that I think does this below. It builds, have
> >> not
> >> yet booted it (but will do so in a bit).
> >>
> >> Index: fs/nfsclient/nfs_clstate.c
> >> ===================================================================
> >> --- fs/nfsclient/nfs_clstate.c (revision 245225)
> >> +++ fs/nfsclient/nfs_clstate.c (working copy)
> >> @@ -4611,7 +4611,7 @@
> >> }
> >> dp = nfscl_finddeleg(clp, np->n_fhp->nfh_fh, np->n_fhp->nfh_len);
> >> if (dp != NULL && (dp->nfsdl_flags & NFSCLDL_WRITE)) {
> >> - NFSGETNANOTIME(&dp->nfsdl_modtime);
> >> + vfs_timestamp(&dp->nfsdl_modtime);
> >> dp->nfsdl_flags |= NFSCLDL_MODTIMESET;
> >> }
> >> NFSUNLOCKCLSTATE();
> > Not sure about this case. Although nfsdl_modtime is being set for local
> > use, it replaces the mtime returned by the NFS server while the delegation
> > is in use. Ideally it would be the same resolution as the NFS server, but
> > that resolution isn't known to the client. (It is often better than 1sec,
> > which is the default for vfs_timestamp().)
>
> The patch seems about right except for this.
>
> > I'd be tempted to leave it (although the function used by the macro might
> > need to be changed, since Bruce mentions getnanotime() isn't supposed to
> > be used?).
>
> For maximal precision and accuracy, it nanotime() should be used. I'm
> not sure if you need to be at least as precise and accurate as the server.
> Having them synced to nanoseconds accuracy is impossible, but
> getnanotime() gives <= 1/HZ of accuracy and it is easy for them to be
> synced with more accuracy than that. Then the extra accuracy can be
> seen in server timestamps if the server is FreeBSD and uses vfs_timestamp()
> with a either microtime() or nanotime().
I've certainly seen NFS servers use much more finely-grained VFS timestamps
(e.g. Isilon nodes run with vfs.timestamp_precision of 2 or 3 so they give
more precise timestamps than just getnanotime()). OTOH, clock drift between
the client and server could easily screw this up. I will leave this as-is
for now and just commit the vfs_timestamp() changes first.
> Further style fixes:
> - remove the NFSGETNANOTIME() macro. It is only used in the above, and
> in 3 other instances where its use is bogus because only the seconds
> part is used. The `time_second' global gives seconds part with the
> same (in)accuracy as getnanotime(). If you want maximal accuracy
> for just the seconds part, then bintime() should be used (this is
> slightly faster than microtime() and nanotime().
> (get*time()'s seconds part is the same as time_second. This
> inaccurate since it lags bintime()'s seconds part by up to 1/HZ
> seconds (so it differs by a full second for an everage of 1 one
> in every HZ readings). The difference is visible if one reader,
> say make(1) reads the time using bintime() while another reader,
> say vfs_timestamp() reads the time using getbintime().)
Yes, I wondered if I could replace those with time_second. The same
is true of the remaining uses of NFSGETTIME() as they also only use the
seconds portion.
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201301181212.41321.jhb>