From owner-freebsd-fs@FreeBSD.ORG Fri Jan 18 02:23:43 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 7A6D0669; Fri, 18 Jan 2013 02:23:43 +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 E31D6654; Fri, 18 Jan 2013 02:23:42 +0000 (UTC) X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: AqAEAG6x+FCDaFvO/2dsb2JhbABFhkW0CYN/c4IeAQEEASMEUgUWDgoCAg0ZAlkGiCYGqVORdoEjjwOBEwOIYY0riU2GfIMTggY X-IronPort-AV: E=Sophos;i="4.84,488,1355115600"; d="scan'208";a="9745603" Received: from erie.cs.uoguelph.ca (HELO zcs3.mail.uoguelph.ca) ([131.104.91.206]) by esa-annu.net.uoguelph.ca with ESMTP; 17 Jan 2013 21:23:35 -0500 Received: from zcs3.mail.uoguelph.ca (localhost.localdomain [127.0.0.1]) by zcs3.mail.uoguelph.ca (Postfix) with ESMTP id DC8CCB3F44; Thu, 17 Jan 2013 21:23:35 -0500 (EST) Date: Thu, 17 Jan 2013 21:23:35 -0500 (EST) From: Rick Macklem To: John Baldwin Message-ID: <460209850.2108683.1358475815866.JavaMail.root@erie.cs.uoguelph.ca> In-Reply-To: <201301171457.43800.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.203] 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 02:23:43 -0000 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().) 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?). > Index: fs/nfsclient/nfs_clvnops.c > =================================================================== > --- fs/nfsclient/nfs_clvnops.c (revision 245225) > +++ fs/nfsclient/nfs_clvnops.c (working copy) > @@ -3247,7 +3247,7 @@ > */ > mtx_lock(&np->n_mtx); > np->n_flag |= NACC; > - getnanotime(&np->n_atim); > + vfs_timestamp(&np->n_atim); > mtx_unlock(&np->n_mtx); > error = fifo_specops.vop_read(ap); > return error; > @@ -3266,7 +3266,7 @@ > */ > mtx_lock(&np->n_mtx); > np->n_flag |= NUPD; > - getnanotime(&np->n_mtim); > + vfs_timestamp(&np->n_mtim); > mtx_unlock(&np->n_mtx); > return(fifo_specops.vop_write(ap)); > } > @@ -3286,7 +3286,7 @@ > > mtx_lock(&np->n_mtx); > if (np->n_flag & (NACC | NUPD)) { > - getnanotime(&ts); > + vfs_timestamp(&ts); > if (np->n_flag & NACC) > np->n_atim = ts; > if (np->n_flag & NUPD) > Index: fs/nfsserver/nfs_nfsdport.c > =================================================================== > --- fs/nfsserver/nfs_nfsdport.c (revision 245225) > +++ fs/nfsserver/nfs_nfsdport.c (working copy) > @@ -1476,7 +1476,7 @@ > struct vattr va; > > VATTR_NULL(&va); > - getnanotime(&va.va_mtime); > + vfs_timestamp(&va.va_mtime); > (void) VOP_SETATTR(vp, &va, cred); > (void) nfsvno_getattr(vp, nvap, cred, p, 1); > } > @@ -2248,7 +2248,6 @@ > { > u_int32_t *tl; > struct nfsv2_sattr *sp; > - struct timeval curtime; > int error = 0, toclient = 0; > > switch (nd->nd_flag & (ND_NFSV2 | ND_NFSV3 | ND_NFSV4)) { > @@ -2307,9 +2306,7 @@ > toclient = 1; > break; > case NFSV3SATTRTIME_TOSERVER: > - NFSGETTIME(&curtime); > - nvap->na_atime.tv_sec = curtime.tv_sec; > - nvap->na_atime.tv_nsec = curtime.tv_usec * 1000; > + vfs_timestamp(&nvap->na_atime); > nvap->na_vaflags |= VA_UTIMES_NULL; > break; > }; > @@ -2321,9 +2318,7 @@ > nvap->na_vaflags &= ~VA_UTIMES_NULL; > break; > case NFSV3SATTRTIME_TOSERVER: > - NFSGETTIME(&curtime); > - nvap->na_mtime.tv_sec = curtime.tv_sec; > - nvap->na_mtime.tv_nsec = curtime.tv_usec * 1000; > + vfs_timestamp(&nvap->na_mtime); > if (!toclient) > nvap->na_vaflags |= VA_UTIMES_NULL; > break; > @@ -2353,7 +2348,6 @@ > u_char *cp, namestr[NFSV4_SMALLSTR + 1]; > uid_t uid; > gid_t gid; > - struct timeval curtime; > > error = nfsrv_getattrbits(nd, attrbitp, NULL, &retnotsup); > if (error) > @@ -2488,9 +2482,7 @@ > toclient = 1; > attrsum += NFSX_V4TIME; > } else { > - NFSGETTIME(&curtime); > - nvap->na_atime.tv_sec = curtime.tv_sec; > - nvap->na_atime.tv_nsec = curtime.tv_usec * 1000; > + vfs_timestamp(&nvap->na_atime); > nvap->na_vaflags |= VA_UTIMES_NULL; > } > break; > @@ -2515,9 +2507,7 @@ > nvap->na_vaflags &= ~VA_UTIMES_NULL; > attrsum += NFSX_V4TIME; > } else { > - NFSGETTIME(&curtime); > - nvap->na_mtime.tv_sec = curtime.tv_sec; > - nvap->na_mtime.tv_nsec = curtime.tv_usec * 1000; > + vfs_timestamp(&nvap->na_mtime); > if (!toclient) > nvap->na_vaflags |= VA_UTIMES_NULL; > } > Index: nfsclient/nfs_vnops.c > =================================================================== > --- nfsclient/nfs_vnops.c (revision 245225) > +++ nfsclient/nfs_vnops.c (working copy) > @@ -3458,7 +3458,7 @@ > */ > mtx_lock(&np->n_mtx); > np->n_flag |= NACC; > - getnanotime(&np->n_atim); > + vfs_timestamp(&np->n_atim); > mtx_unlock(&np->n_mtx); > error = fifo_specops.vop_read(ap); > return error; > @@ -3477,7 +3477,7 @@ > */ > mtx_lock(&np->n_mtx); > np->n_flag |= NUPD; > - getnanotime(&np->n_mtim); > + vfs_timestamp(&np->n_mtim); > mtx_unlock(&np->n_mtx); > return(fifo_specops.vop_write(ap)); > } > @@ -3497,7 +3497,7 @@ > > mtx_lock(&np->n_mtx); > if (np->n_flag & (NACC | NUPD)) { > - getnanotime(&ts); > + vfs_timestamp(&ts); > if (np->n_flag & NACC) > np->n_atim = ts; > if (np->n_flag & NUPD) > Index: nfsserver/nfs_srvsubs.c > =================================================================== > --- nfsserver/nfs_srvsubs.c (revision 245225) > +++ nfsserver/nfs_srvsubs.c (working copy) > @@ -1393,7 +1393,7 @@ > toclient = 1; > break; > case NFSV3SATTRTIME_TOSERVER: > - getnanotime(&(a)->va_atime); > + vfs_timestamp(&(a)->va_atime); > a->va_vaflags |= VA_UTIMES_NULL; > break; > } > @@ -1409,7 +1409,7 @@ > a->va_vaflags &= ~VA_UTIMES_NULL; > break; > case NFSV3SATTRTIME_TOSERVER: > - getnanotime(&(a)->va_mtime); > + vfs_timestamp(&(a)->va_mtime); > if (toclient == 0) > a->va_vaflags |= VA_UTIMES_NULL; > break; > > -- > John Baldwin Other than nfsdl_modtime, the rest look ok to me, since they are either the times for the special files in the client or timestamps for server file systems. rick