Date: Thu, 24 Jan 2013 19:15:26 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: John Baldwin <jhb@FreeBSD.org> Cc: Rick Macklem <rmacklem@FreeBSD.org>, bde@FreeBSD.org, fs@FreeBSD.org Subject: Re: [PATCH] More time cleanups in the NFS code Message-ID: <20130124184756.O1180@besplex.bde.org> In-Reply-To: <201301231338.39056.jhb@freebsd.org> References: <201301231338.39056.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130124184756.O1180>