Date: Thu, 24 Jan 2013 11:27:33 -0500 From: John Baldwin <jhb@freebsd.org> 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: <201301241127.33274.jhb@freebsd.org> In-Reply-To: <20130124184756.O1180@besplex.bde.org> References: <201301231338.39056.jhb@freebsd.org> <20130124184756.O1180@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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)
break;
/* Sleep for a short period of time */
(void) nfs_catnap(PZERO, 0, "nfsremove");
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201301241127.33274.jhb>