From owner-freebsd-fs@FreeBSD.ORG Fri Jan 25 02:26:23 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 12092C3A; Fri, 25 Jan 2013 02:26:23 +0000 (UTC) (envelope-from rmacklem@uoguelph.ca) Received: from esa-annu.net.uoguelph.ca (esa-annu.mail.uoguelph.ca [131.104.91.36]) by mx1.freebsd.org (Postfix) with ESMTP id 2D60E615; Fri, 25 Jan 2013 02:26:21 +0000 (UTC) X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: AqAEAA/sAVGDaFvO/2dsb2JhbABEhkW4GXOCHgEBAQMBAQEBIAQnIAsFFg4KAgINGQIpAQkmBggHBAEcBIdzBgyrJpJfgSOLcIJVgRMDiGGKfYIugRyPLIMWgVE1 X-IronPort-AV: E=Sophos;i="4.84,534,1355115600"; d="scan'208";a="10807869" Received: from erie.cs.uoguelph.ca (HELO zcs3.mail.uoguelph.ca) ([131.104.91.206]) by esa-annu.net.uoguelph.ca with ESMTP; 24 Jan 2013 21:26:15 -0500 Received: from zcs3.mail.uoguelph.ca (localhost.localdomain [127.0.0.1]) by zcs3.mail.uoguelph.ca (Postfix) with ESMTP id 1FD80B4044; Thu, 24 Jan 2013 21:26:15 -0500 (EST) Date: Thu, 24 Jan 2013 21:26:15 -0500 (EST) From: Rick Macklem To: John Baldwin Message-ID: <1303380973.2342056.1359080775074.JavaMail.root@erie.cs.uoguelph.ca> In-Reply-To: <201301241127.33274.jhb@freebsd.org> Subject: Re: [PATCH] More time cleanups in the NFS code MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Originating-IP: [172.17.91.201] X-Mailer: Zimbra 6.0.10_GA_2692 (ZimbraWebClient - FF3.0 (Win)/6.0.10_GA_2692) 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: Fri, 25 Jan 2013 02:26:23 -0000 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"