From owner-freebsd-fs@FreeBSD.ORG Thu Jan 24 19:52:40 2013 Return-Path: Delivered-To: fs@freebsd.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id AF2B6C49; Thu, 24 Jan 2013 19:52:40 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from bigwig.baldwin.cx (bigknife-pt.tunnel.tserv9.chi1.ipv6.he.net [IPv6:2001:470:1f10:75::2]) by mx1.freebsd.org (Postfix) with ESMTP id 8758513E; Thu, 24 Jan 2013 19:52:40 +0000 (UTC) Received: from pakbsde14.localnet (unknown [38.105.238.108]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id D268FB91A; Thu, 24 Jan 2013 14:52:39 -0500 (EST) From: John Baldwin To: Bruce Evans Subject: Re: [PATCH] More time cleanups in the NFS code Date: Thu, 24 Jan 2013 11:27:33 -0500 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p22; KDE/4.5.5; amd64; ; ) References: <201301231338.39056.jhb@freebsd.org> <20130124184756.O1180@besplex.bde.org> In-Reply-To: <20130124184756.O1180@besplex.bde.org> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <201301241127.33274.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.7 (bigwig.baldwin.cx); Thu, 24 Jan 2013 14:52:39 -0500 (EST) Cc: Rick Macklem , bde@freebsd.org, fs@freebsd.org X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 24 Jan 2013 19:52:40 -0000 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