From owner-freebsd-fs@FreeBSD.ORG Fri Jan 18 23:36:27 2013 Return-Path: Delivered-To: freebsd-fs@freebsd.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id 3A8F7956; Fri, 18 Jan 2013 23:36:27 +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 C38ADE18; Fri, 18 Jan 2013 23:36:26 +0000 (UTC) X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: AqAEAJDb+VCDaFvO/2dsb2JhbABEhkW0D4N8c4IeAQEFIwRSGw4KAgINGQJZBogsqlKRaYEjjwOBEwOIYY0riU2GfIMTggY X-IronPort-AV: E=Sophos;i="4.84,495,1355115600"; d="scan'208";a="12685634" Received: from erie.cs.uoguelph.ca (HELO zcs3.mail.uoguelph.ca) ([131.104.91.206]) by esa-jnhn.mail.uoguelph.ca with ESMTP; 18 Jan 2013 18:36:20 -0500 Received: from zcs3.mail.uoguelph.ca (localhost.localdomain [127.0.0.1]) by zcs3.mail.uoguelph.ca (Postfix) with ESMTP id 2D267B3F0D; Fri, 18 Jan 2013 18:36:20 -0500 (EST) Date: Fri, 18 Jan 2013 18:36:20 -0500 (EST) From: Rick Macklem To: John Baldwin Message-ID: <1309255502.2141506.1358552180122.JavaMail.root@erie.cs.uoguelph.ca> In-Reply-To: <201301181212.41321.jhb@freebsd.org> Subject: Re: [PATCH] Use vfs_timestamp() instead of getnanotime() in NFS 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: freebsd-fs@freebsd.org, Rick Macklem 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, 18 Jan 2013 23:36:27 -0000 John Baldwin wrote: > On Friday, January 18, 2013 1:19:29 am Bruce Evans wrote: > > On Thu, 17 Jan 2013, Rick Macklem wrote: > > > > > John Baldwin wrote: > > >> On Tuesday, January 15, 2013 2:58:42 pm John Baldwin wrote: > > >>> Fixing NFS to properly use vfs_timestamp() seems to be a larger > > >>> project. > > >> > > >> Actually, I have a patch that I think does this below. It builds, > > >> have > > >> not > > >> yet booted it (but will do so in a bit). > > >> > > >> Index: fs/nfsclient/nfs_clstate.c > > >> =================================================================== > > >> --- fs/nfsclient/nfs_clstate.c (revision 245225) > > >> +++ fs/nfsclient/nfs_clstate.c (working copy) > > >> @@ -4611,7 +4611,7 @@ > > >> } > > >> dp = nfscl_finddeleg(clp, np->n_fhp->nfh_fh, np->n_fhp->nfh_len); > > >> if (dp != NULL && (dp->nfsdl_flags & NFSCLDL_WRITE)) { > > >> - NFSGETNANOTIME(&dp->nfsdl_modtime); > > >> + vfs_timestamp(&dp->nfsdl_modtime); > > >> dp->nfsdl_flags |= NFSCLDL_MODTIMESET; > > >> } > > >> NFSUNLOCKCLSTATE(); > > > Not sure about this case. Although nfsdl_modtime is being set for > > > local > > > use, it replaces the mtime returned by the NFS server while the > > > delegation > > > is in use. Ideally it would be the same resolution as the NFS > > > server, but > > > that resolution isn't known to the client. (It is often better > > > than 1sec, > > > which is the default for vfs_timestamp().) > > > > The patch seems about right except for this. > > > > > I'd be tempted to leave it (although the function used by the > > > macro might > > > need to be changed, since Bruce mentions getnanotime() isn't > > > supposed to > > > be used?). > > > > For maximal precision and accuracy, it nanotime() should be used. > > I'm > > not sure if you need to be at least as precise and accurate as the > > server. > > Having them synced to nanoseconds accuracy is impossible, but > > getnanotime() gives <= 1/HZ of accuracy and it is easy for them to > > be > > synced with more accuracy than that. Then the extra accuracy can be > > seen in server timestamps if the server is FreeBSD and uses > > vfs_timestamp() > > with a either microtime() or nanotime(). > > I've certainly seen NFS servers use much more finely-grained VFS > timestamps > (e.g. Isilon nodes run with vfs.timestamp_precision of 2 or 3 so they > give > more precise timestamps than just getnanotime()). OTOH, clock drift > between > the client and server could easily screw this up. I will leave this > as-is > for now and just commit the vfs_timestamp() changes first. > > > Further style fixes: > > - remove the NFSGETNANOTIME() macro. It is only used in the above, > > and > > in 3 other instances where its use is bogus because only the > > seconds > > part is used. The `time_second' global gives seconds part with > > the > > same (in)accuracy as getnanotime(). If you want maximal accuracy > > for just the seconds part, then bintime() should be used (this is > > slightly faster than microtime() and nanotime(). > > (get*time()'s seconds part is the same as time_second. This > > inaccurate since it lags bintime()'s seconds part by up to 1/HZ > > seconds (so it differs by a full second for an everage of 1 one > > in every HZ readings). The difference is visible if one reader, > > say make(1) reads the time using bintime() while another > > reader, > > say vfs_timestamp() reads the time using getbintime().) > > Yes, I wondered if I could replace those with time_second. The same > is true of the remaining uses of NFSGETTIME() as they also only use > the > seconds portion. > Those macros are just cruft left over from when the code was written to be portable between various BSDen. And what they were mapped to was just something that seemed to work. Feel free to replace them with whatever makes sense, rick > -- > John Baldwin