[PATCH] More time cleanups in the NFS code

[Rick Macklem]

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 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"