Date: Thu, 24 Jan 2013 10:12:10 -0500 (EST) From: Rick Macklem <rmacklem@uoguelph.ca> To: Bruce Evans <brde@optusnet.com.au> Cc: Rick Macklem <rmacklem@FreeBSD.org>, bde@FreeBSD.org, fs@FreeBSD.org Subject: Re: [PATCH] More time cleanups in the NFS code Message-ID: <1868504817.2310318.1359040330206.JavaMail.root@erie.cs.uoguelph.ca> In-Reply-To: <20130124184756.O1180@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Bruce Evans wrote:
> On Wed, 23 Jan 2013, John Baldwin wrote:
>
> > This patch removes all calls to get*time(). Most of them it replaces
> > with
> > time_uptime (especially ones that are attempting to handle time
> > intervals for
> > which time_uptime is far better suited than time_second). One
> > specific case
> > it replaces with nanotime() as suggested by Bruce previously. A few
> > of the
> > timestamps were not used (nd_starttime and the curtime in the lease
> > expiry
> > function).
>
> Looks good.
>
> I didn't check for completeness.
>
> oldnfs might benefit from use of NFSD_MONOSEC.
>
I put NFSD_MONOSEC in for portability between BSDens, when that mattered
to me. Do whatever you like with it, such as get rid of it or ...
rick
> Both nfs's might benefit from use of NFS_REALSEC (doesn't exist but
> would be #defined as time_second if acceses to this global are atomic
> (which I think is implied by its existence)).
>
> > Index: fs/nfs/nfs_commonkrpc.c
> > ===================================================================
> > --- fs/nfs/nfs_commonkrpc.c (revision 245742)
> > +++ fs/nfs/nfs_commonkrpc.c (working copy)
> > @@ -459,18 +459,17 @@
> > {
> > struct nfs_feedback_arg *nf = (struct nfs_feedback_arg *) arg;
> > struct nfsmount *nmp = nf->nf_mount;
> > - struct timeval now;
> > + time_t now;
> >
> > - getmicrouptime(&now);
> > -
> > switch (type) {
> > case FEEDBACK_REXMIT2:
> > case FEEDBACK_RECONNECT:
> > - if (nf->nf_lastmsg + nmp->nm_tprintf_delay < now.tv_sec) {
> > + now = NFSD_MONOSEC;
>
> It's confusing for 'now' to be in mono-time.
>
> > + if (nf->nf_lastmsg + nmp->nm_tprintf_delay < now) {
> > nfs_down(nmp, nf->nf_td,
> > "not responding", 0, NFSSTA_TIMEO);
> > nf->nf_tprintfmsg = TRUE;
> > - nf->nf_lastmsg = now.tv_sec;
> > + nf->nf_lastmsg = now;
> > }
> > break;
>
> It's safest but probably unnecessary (uncritical) to copy the (not
> quite
> volatile) variable NFSD_MONOSEC to a local variable, since it is used
> twice.
>
> Now I don't like the NFSD_MONOSEC macro. It looks like a constant, but
> is actually a not quite volatile variable.
>
> > Index: fs/nfsclient/nfs_clstate.c
> > ===================================================================
> > --- fs/nfsclient/nfs_clstate.c (revision 245742)
> > +++ fs/nfsclient/nfs_clstate.c (working copy)
> > @@ -2447,7 +2447,7 @@
> > u_int32_t clidrev;
> > int error, cbpathdown, islept, igotlock, ret, clearok;
> > uint32_t recover_done_time = 0;
> > - struct timespec mytime;
> > + time_t mytime;
>
> Another name for the cached copy of mono-now.
>
> > @@ -2720,9 +2720,9 @@
> > * Call nfscl_cleanupkext() once per second to check for
> > * open/lock owners where the process has exited.
> > */
> > - NFSGETNANOTIME(&mytime);
> > - if (prevsec != mytime.tv_sec) {
> > - prevsec = mytime.tv_sec;
> > + mytime = NFSD_MONOSEC;
> > + if (prevsec != mytime) {
> > + prevsec = mytime;
> > nfscl_cleanupkext(clp, &lfh);
> > }
> >
>
> Now copying it is clearly needed.
>
> > @@ -4684,11 +4682,9 @@
> > } else
> > error = EPERM;
> > if (error == NFSERR_DELAY) {
> > - NFSGETNANOTIME(&mytime);
> > - if (((u_int32_t)mytime.tv_sec - starttime) >
> > - NFS_REMOVETIMEO &&
> > - ((u_int32_t)mytime.tv_sec - starttime) <
> > - 100000)
> > + mytime = NFSD_MONOSEC;
> > + if (((u_int32_t)mytime - starttime) > NFS_REMOVETIMEO &&
> > + ((u_int32_t)mytime - starttime) < 100000)
> > break;
> > /* Sleep for a short period of time */
> > (void) nfs_catnap(PZERO, 0, "nfsremove");
>
> Should use time_t for all times in seconds and no casts to u_int32_t
> (unless the times are put in data structures -- then 64-bit times are
> wasteful).
>
> Here, when not doing this cleanup, mytime might as well have type
> u_int32_t to begin with, to match starttime. Then the bogus cast would
> be implicit in the assignment to mytime. The old code had to cast to
> break the type of mytime.tv_sec to match that of starttime. This of
> course only mattered when the times were non-monotonic, time_t was 64
> bits, and the non-mono time was later than the middle of 2038.
>
> Bruce
> _______________________________________________
> freebsd-fs@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-fs
> To unsubscribe, send any mail to "freebsd-fs-unsubscribe@freebsd.org"
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1868504817.2310318.1359040330206.JavaMail.root>