Date: Thu, 24 Jan 2013 21:26:15 -0500 (EST) From: Rick Macklem <rmacklem@uoguelph.ca> To: John Baldwin <jhb@freebsd.org> Cc: Rick Macklem <rmacklem@freebsd.org>, bde@freebsd.org, fs@freebsd.org Subject: Re: [PATCH] More time cleanups in the NFS code Message-ID: <1303380973.2342056.1359080775074.JavaMail.root@erie.cs.uoguelph.ca> In-Reply-To: <201301241127.33274.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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@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?1303380973.2342056.1359080775074.JavaMail.root>