From owner-freebsd-fs@FreeBSD.ORG Thu Jan 24 15:12:12 2013 Return-Path: Delivered-To: fs@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id 489E92D5; Thu, 24 Jan 2013 15:12:12 +0000 (UTC) (envelope-from rmacklem@uoguelph.ca) Received: from esa-jnhn.mail.uoguelph.ca (esa-jnhn.mail.uoguelph.ca [131.104.91.44]) by mx1.freebsd.org (Postfix) with ESMTP id CB322E4C; Thu, 24 Jan 2013 15:12:11 +0000 (UTC) X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: AqAEAMVOAVGDaFvO/2dsb2JhbABEhkW4GHOCHgEBAQMBAQEBIAQnIAsFFg4KAgINGQIpAQkmBggHBAEcBIdzBgyrGpJogSOLcIJVgRMDiGGKfYIugRyPLIMWgVE1 X-IronPort-AV: E=Sophos;i="4.84,530,1355115600"; d="scan'208";a="13432171" Received: from erie.cs.uoguelph.ca (HELO zcs3.mail.uoguelph.ca) ([131.104.91.206]) by esa-jnhn.mail.uoguelph.ca with ESMTP; 24 Jan 2013 10:12:10 -0500 Received: from zcs3.mail.uoguelph.ca (localhost.localdomain [127.0.0.1]) by zcs3.mail.uoguelph.ca (Postfix) with ESMTP id 35541B3F18; Thu, 24 Jan 2013 10:12:10 -0500 (EST) Date: Thu, 24 Jan 2013 10:12:10 -0500 (EST) From: Rick Macklem To: Bruce Evans Message-ID: <1868504817.2310318.1359040330206.JavaMail.root@erie.cs.uoguelph.ca> In-Reply-To: <20130124184756.O1180@besplex.bde.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.202] 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: Thu, 24 Jan 2013 15:12:12 -0000 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@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-fs > To unsubscribe, send any mail to "freebsd-fs-unsubscribe@freebsd.org"