[PATCH] More time cleanups in the NFS code

[Rick Macklem]

John Baldwin wrote:
> On Thursday, January 24, 2013 3:15:26 am 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.
> >
> > 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)).
> 
> Accesses should be atomic.
> 
> > > 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.
> 
> 'now' is all relative anyway. :)
> 
> > > + 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.
> 
> I have a separate patch to make both time_second and time_uptime
> volatile.
> The global 'ticks' should also be made volatile for the same reason.
> 
> > > @@ -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).
> 
> Ah, for some reason I had thought starttime was stuffed into a packet
> or some
> such. It is not, so I redid this as:
> 
> @@ -4650,8 +4648,7 @@ out:
> APPLESTATIC void
> nfsd_recalldelegation(vnode_t vp, NFSPROC_T *p)
> {
> - struct timespec mytime;
> - int32_t starttime;
> + time_t starttime, elapsed;
> int error;
> 
> /*
> @@ -4675,8 +4672,7 @@ nfsd_recalldelegation(vnode_t vp, NFSPROC_T *p)
> * Now, call nfsrv_checkremove() in a loop while it returns
> * NFSERR_DELAY. Return upon any other error or when timed out.
> */
> - NFSGETNANOTIME(&mytime);
> - starttime = (u_int32_t)mytime.tv_sec;
> + starttime = NFSD_MONOSEC;
> do {
> if (NFSVOPLOCK(vp, LK_EXCLUSIVE) == 0) {
> error = nfsrv_checkremove(vp, 0, p);
> @@ -4684,11 +4680,8 @@ nfsd_recalldelegation(vnode_t vp, NFSPROC_T *p)
> } 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)
> + elapsed = NFSD_MONOSEC - starttime;
> + if (elapsed > NFS_REMOVETIMEO && elapsed < 100000)

For this patch, I think you can get rid of "&& elapsed < 100000"
from the above line. I can't really remember, but I think I
coded this as a sanity check for a clock going backwards.
(And, yes, this patch looks fine to me, too.)

Thanks again for working on this, rick

> break;
> /* Sleep for a short period of time */
> (void) nfs_catnap(PZERO, 0, "nfsremove");
> 
> 
> --
> John Baldwin
> _______________________________________________
> freebsd-fs at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-fs
> To unsubscribe, send any mail to "freebsd-fs-unsubscribe at freebsd.org"