Date: Thu, 17 Jan 2013 21:23:35 -0500 (EST) From: Rick Macklem <rmacklem@uoguelph.ca> To: John Baldwin <jhb@freebsd.org> Cc: freebsd-fs@freebsd.org, Rick Macklem <rmacklem@freebsd.org> Subject: Re: [PATCH] Use vfs_timestamp() instead of getnanotime() in NFS Message-ID: <460209850.2108683.1358475815866.JavaMail.root@erie.cs.uoguelph.ca> In-Reply-To: <201301171457.43800.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?460209850.2108683.1358475815866.JavaMail.root>