From owner-freebsd-fs@FreeBSD.ORG Thu Jan 24 08:15:36 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 BC721C9D; Thu, 24 Jan 2013 08:15:36 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail27.syd.optusnet.com.au (mail27.syd.optusnet.com.au [211.29.133.168]) by mx1.freebsd.org (Postfix) with ESMTP id 3D603DE0; Thu, 24 Jan 2013 08:15:35 +0000 (UTC) Received: from c211-30-173-106.carlnfd1.nsw.optusnet.com.au (c211-30-173-106.carlnfd1.nsw.optusnet.com.au [211.30.173.106]) by mail27.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id r0O8FQ9s029583 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 24 Jan 2013 19:15:28 +1100 Date: Thu, 24 Jan 2013 19:15:26 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: John Baldwin Subject: Re: [PATCH] More time cleanups in the NFS code In-Reply-To: <201301231338.39056.jhb@freebsd.org> Message-ID: <20130124184756.O1180@besplex.bde.org> References: <201301231338.39056.jhb@freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.0 cv=MscKcBme c=1 sm=1 a=Kpej93CV1R8A:10 a=kj9zAlcOel0A:10 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=_HDPiWP5c-IA:10 a=Ae79VNAGcdZGe2jW8Y0A:9 a=CjuIK1q_8ugA:10 a=TEtd8y5WR3g2ypngnwZWYw==:117 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 08:15:36 -0000 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)). > 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