Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 20 Jan 2012 20:02:01 +0000 (UTC)
From:      John Baldwin <jhb@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r230394 - in head/sys: fs/nfsclient kern nfsclient sys
Message-ID:  <201201202002.q0KK21M6056471@svn.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: jhb
Date: Fri Jan 20 20:02:01 2012
New Revision: 230394
URL: http://svn.freebsd.org/changeset/base/230394

Log:
  Close a race in NFS lookup processing that could result in stale name cache
  entries on one client when a directory was renamed on another client.  The
  root cause for the stale entry being trusted is that each per-vnode nfsnode
  structure has a single 'n_ctime' timestamp used to validate positive name
  cache entries.  However, if there are multiple entries for a single vnode,
  they all share a single timestamp.  To fix this, extend the name cache
  to allow filesystems to optionally store a timestamp value in each name
  cache entry.  The NFS clients now fetch the timestamp associated with
  each name cache entry and use that to validate cache hits instead of the
  timestamps previously stored in the nfsnode.  Another part of the fix is
  that the NFS clients now use timestamps from the post-op attributes of
  RPCs when adding name cache entries rather than pulling the timestamps out
  of the file's attribute cache.  The latter is subject to races with other
  lookups updating the attribute cache concurrently.  Some more details:
  - Add a variant of nfsm_postop_attr() to the old NFS client that can return
    a vattr structure with a copy of the post-op attributes.
  - Handle lookups of "." as a special case in the NFS clients since the name
    cache does not store name cache entries for ".", so we cannot get a
    useful timestamp.  It didn't really make much sense to recheck the
    attributes on the the directory to validate the namecache hit for "."
    anyway.
  - ABI compat shims for the name cache routines are present in this commit
    so that it is safe to MFC.
  
  MFC after:	2 weeks

Modified:
  head/sys/fs/nfsclient/nfs_clrpcops.c
  head/sys/fs/nfsclient/nfs_clvnops.c
  head/sys/fs/nfsclient/nfsnode.h
  head/sys/kern/vfs_cache.c
  head/sys/nfsclient/nfs_subs.c
  head/sys/nfsclient/nfs_vnops.c
  head/sys/nfsclient/nfsm_subs.h
  head/sys/nfsclient/nfsnode.h
  head/sys/sys/vnode.h

Modified: head/sys/fs/nfsclient/nfs_clrpcops.c
==============================================================================
--- head/sys/fs/nfsclient/nfs_clrpcops.c	Fri Jan 20 19:18:11 2012	(r230393)
+++ head/sys/fs/nfsclient/nfs_clrpcops.c	Fri Jan 20 20:02:01 2012	(r230394)
@@ -3317,8 +3317,9 @@ nfsrpc_readdirplus(vnode_t vp, struct ui
 				    ndp->ni_vp = newvp;
 				    NFSCNHASH(cnp, HASHINIT);
 				    if (cnp->cn_namelen <= NCHNAMLEN) {
-					np->n_ctime = np->n_vattr.na_ctime;
-					cache_enter(ndp->ni_dvp,ndp->ni_vp,cnp);
+					cache_enter_time(ndp->ni_dvp,
+					    ndp->ni_vp, cnp,
+					    &nfsva.na_ctime);
 				    }
 				    if (unlocknewvp)
 					vput(newvp);

Modified: head/sys/fs/nfsclient/nfs_clvnops.c
==============================================================================
--- head/sys/fs/nfsclient/nfs_clvnops.c	Fri Jan 20 19:18:11 2012	(r230393)
+++ head/sys/fs/nfsclient/nfs_clvnops.c	Fri Jan 20 20:02:01 2012	(r230394)
@@ -1016,12 +1016,12 @@ nfs_lookup(struct vop_lookup_args *ap)
 	struct vnode *newvp;
 	struct nfsmount *nmp;
 	struct nfsnode *np, *newnp;
-	int error = 0, attrflag, dattrflag, ltype;
+	int error = 0, attrflag, dattrflag, ltype, ncticks;
 	struct thread *td = cnp->cn_thread;
 	struct nfsfh *nfhp;
 	struct nfsvattr dnfsva, nfsva;
 	struct vattr vattr;
-	struct timespec dmtime;
+	struct timespec nctime;
 	
 	*vpp = NULLVP;
 	if ((flags & ISLASTCN) && (mp->mnt_flag & MNT_RDONLY) &&
@@ -1042,11 +1042,24 @@ nfs_lookup(struct vop_lookup_args *ap)
 
 	if ((error = VOP_ACCESS(dvp, VEXEC, cnp->cn_cred, td)) != 0)
 		return (error);
-	error = cache_lookup(dvp, vpp, cnp);
+	error = cache_lookup_times(dvp, vpp, cnp, &nctime, &ncticks);
 	if (error > 0 && error != ENOENT)
 		return (error);
 	if (error == -1) {
 		/*
+		 * Lookups of "." are special and always return the
+		 * current directory.  cache_lookup() already handles
+		 * associated locking bookkeeping, etc.
+		 */
+		if (cnp->cn_namelen == 1 && cnp->cn_nameptr[0] == '.') {
+			/* XXX: Is this really correct? */
+			if (cnp->cn_nameiop != LOOKUP &&
+			    (flags & ISLASTCN))
+				cnp->cn_flags |= SAVENAME;
+			return (0);
+		}
+
+		/*
 		 * We only accept a positive hit in the cache if the
 		 * change time of the file matches our cached copy.
 		 * Otherwise, we discard the cache entry and fallback
@@ -1073,7 +1086,7 @@ nfs_lookup(struct vop_lookup_args *ap)
 		}
 		if (nfscl_nodeleg(newvp, 0) == 0 ||
 		    (VOP_GETATTR(newvp, &vattr, cnp->cn_cred) == 0 &&
-		    timespeccmp(&vattr.va_ctime, &newnp->n_ctime, ==))) {
+		    timespeccmp(&vattr.va_ctime, &nctime, ==))) {
 			NFSINCRGLOBAL(newnfsstats.lookupcache_hits);
 			if (cnp->cn_nameiop != LOOKUP &&
 			    (flags & ISLASTCN))
@@ -1092,36 +1105,21 @@ nfs_lookup(struct vop_lookup_args *ap)
 		/*
 		 * We only accept a negative hit in the cache if the
 		 * modification time of the parent directory matches
-		 * our cached copy.  Otherwise, we discard all of the
-		 * negative cache entries for this directory. We also
-		 * only trust -ve cache entries for less than
-		 * nm_negative_namecache_timeout seconds.
+		 * the cached copy in the name cache entry.
+		 * Otherwise, we discard all of the negative cache
+		 * entries for this directory.  We also only trust
+		 * negative cache entries for up to nm_negnametimeo
+		 * seconds.
 		 */
-		if ((u_int)(ticks - np->n_dmtime_ticks) <
-		    (nmp->nm_negnametimeo * hz) &&
+		if ((u_int)(ticks - ncticks) < (nmp->nm_negnametimeo * hz) &&
 		    VOP_GETATTR(dvp, &vattr, cnp->cn_cred) == 0 &&
-		    timespeccmp(&vattr.va_mtime, &np->n_dmtime, ==)) {
+		    timespeccmp(&vattr.va_mtime, &nctime, ==)) {
 			NFSINCRGLOBAL(newnfsstats.lookupcache_hits);
 			return (ENOENT);
 		}
 		cache_purge_negative(dvp);
-		mtx_lock(&np->n_mtx);
-		timespecclear(&np->n_dmtime);
-		mtx_unlock(&np->n_mtx);
 	}
 
-	/*
-	 * Cache the modification time of the parent directory in case
-	 * the lookup fails and results in adding the first negative
-	 * name cache entry for the directory.  Since this is reading
-	 * a single time_t, don't bother with locking.  The
-	 * modification time may be a bit stale, but it must be read
-	 * before performing the lookup RPC to prevent a race where
-	 * another lookup updates the timestamp on the directory after
-	 * the lookup RPC has been performed on the server but before
-	 * n_dmtime is set at the end of this function.
-	 */
-	dmtime = np->n_vattr.na_mtime;
 	error = 0;
 	newvp = NULLVP;
 	NFSINCRGLOBAL(newnfsstats.lookupcache_misses);
@@ -1157,30 +1155,22 @@ nfs_lookup(struct vop_lookup_args *ap)
 			return (EJUSTRETURN);
 		}
 
-		if ((cnp->cn_flags & MAKEENTRY) && cnp->cn_nameiop != CREATE) {
+		if ((cnp->cn_flags & MAKEENTRY) && cnp->cn_nameiop != CREATE &&
+		    dattrflag) {
 			/*
-			 * Maintain n_dmtime as the modification time
-			 * of the parent directory when the oldest -ve
-			 * name cache entry for this directory was
-			 * added.  If a -ve cache entry has already
-			 * been added with a newer modification time
-			 * by a concurrent lookup, then don't bother
-			 * adding a cache entry.  The modification
-			 * time of the directory might have changed
-			 * due to the file this lookup failed to find
-			 * being created.  In that case a subsequent
-			 * lookup would incorrectly use the entry
-			 * added here instead of doing an extra
-			 * lookup.
+			 * Cache the modification time of the parent
+			 * directory from the post-op attributes in
+			 * the name cache entry.  The negative cache
+			 * entry will be ignored once the directory
+			 * has changed.  Don't bother adding the entry
+			 * if the directory has already changed.
 			 */
 			mtx_lock(&np->n_mtx);
-			if (timespeccmp(&np->n_dmtime, &dmtime, <=)) {
-				if (!timespecisset(&np->n_dmtime)) {
-					np->n_dmtime = dmtime;
-					np->n_dmtime_ticks = ticks;
-				}
+			if (timespeccmp(&np->n_vattr.na_mtime,
+			    &dnfsva.na_mtime, ==)) {
 				mtx_unlock(&np->n_mtx);
-				cache_enter(dvp, NULL, cnp);
+				cache_enter_time(dvp, NULL, cnp,
+				    &dnfsva.na_mtime);
 			} else
 				mtx_unlock(&np->n_mtx);
 		}
@@ -1279,9 +1269,8 @@ nfs_lookup(struct vop_lookup_args *ap)
 	if (cnp->cn_nameiop != LOOKUP && (flags & ISLASTCN))
 		cnp->cn_flags |= SAVENAME;
 	if ((cnp->cn_flags & MAKEENTRY) &&
-	    (cnp->cn_nameiop != DELETE || !(flags & ISLASTCN))) {
-		np->n_ctime = np->n_vattr.na_vattr.va_ctime;
-		cache_enter(dvp, newvp, cnp);
+	    (cnp->cn_nameiop != DELETE || !(flags & ISLASTCN)) && attrflag) {
+		cache_enter_time(dvp, newvp, cnp, &nfsva.na_ctime);
 	}
 	*vpp = newvp;
 	return (0);

Modified: head/sys/fs/nfsclient/nfsnode.h
==============================================================================
--- head/sys/fs/nfsclient/nfsnode.h	Fri Jan 20 19:18:11 2012	(r230393)
+++ head/sys/fs/nfsclient/nfsnode.h	Fri Jan 20 20:02:01 2012	(r230394)
@@ -99,9 +99,6 @@ struct nfsnode {
 	time_t			n_attrstamp;	/* Attr. cache timestamp */
 	struct nfs_accesscache	n_accesscache[NFS_ACCESSCACHESIZE];
 	struct timespec		n_mtime;	/* Prev modify time. */
-	struct timespec		n_ctime;	/* Prev create time. */
-	struct timespec		n_dmtime;	/* Prev dir modify time. */
-	int			n_dmtime_ticks;	/* Tick of -ve cache entry */
 	struct nfsfh		*n_fhp;		/* NFS File Handle */
 	struct vnode		*n_vnode;	/* associated vnode */
 	struct vnode		*n_dvp;		/* parent vnode */

Modified: head/sys/kern/vfs_cache.c
==============================================================================
--- head/sys/kern/vfs_cache.c	Fri Jan 20 19:18:11 2012	(r230393)
+++ head/sys/kern/vfs_cache.c	Fri Jan 20 20:02:01 2012	(r230394)
@@ -97,6 +97,8 @@ struct	namecache {
 	TAILQ_ENTRY(namecache) nc_dst;	/* destination vnode list */
 	struct	vnode *nc_dvp;		/* vnode of parent of name */
 	struct	vnode *nc_vp;		/* vnode the name refers to */
+	struct	timespec nc_time;	/* timespec provided by fs */
+	int	nc_ticks;		/* ticks value when entry was added */
 	u_char	nc_flag;		/* flag bits */
 	u_char	nc_nlen;		/* length of name */
 	char	nc_name[0];		/* segment name + nul */
@@ -394,10 +396,12 @@ cache_zap(ncp)
  */
 
 int
-cache_lookup(dvp, vpp, cnp)
+cache_lookup_times(dvp, vpp, cnp, tsp, ticksp)
 	struct vnode *dvp;
 	struct vnode **vpp;
 	struct componentname *cnp;
+	struct timespec *tsp;
+	int *ticksp;
 {
 	struct namecache *ncp;
 	uint32_t hash;
@@ -422,6 +426,10 @@ retry_wlocked:
 			dothits++;
 			SDT_PROBE(vfs, namecache, lookup, hit, dvp, ".",
 			    *vpp, 0, 0);
+			if (tsp != NULL)
+				timespecclear(tsp);
+			if (ticksp != NULL)
+				*ticksp = ticks;
 			goto success;
 		}
 		if (cnp->cn_namelen == 2 && cnp->cn_nameptr[1] == '.') {
@@ -440,19 +448,22 @@ retry_wlocked:
 				CACHE_WUNLOCK();
 				return (0);
 			}
-			if (dvp->v_cache_dd->nc_flag & NCF_ISDOTDOT)
-				*vpp = dvp->v_cache_dd->nc_vp;
+			ncp = dvp->v_cache_dd;
+			if (ncp->nc_flag & NCF_ISDOTDOT)
+				*vpp = ncp->nc_vp;
 			else
-				*vpp = dvp->v_cache_dd->nc_dvp;
+				*vpp = ncp->nc_dvp;
 			/* Return failure if negative entry was found. */
-			if (*vpp == NULL) {
-				ncp = dvp->v_cache_dd;
+			if (*vpp == NULL)
 				goto negative_success;
-			}
 			CTR3(KTR_VFS, "cache_lookup(%p, %s) found %p via ..",
 			    dvp, cnp->cn_nameptr, *vpp);
 			SDT_PROBE(vfs, namecache, lookup, hit, dvp, "..",
 			    *vpp, 0, 0);
+			if (tsp != NULL)
+				*tsp = ncp->nc_time;
+			if (ticksp != NULL)
+				*ticksp = ncp->nc_ticks;
 			goto success;
 		}
 	}
@@ -499,6 +510,10 @@ retry_wlocked:
 		    dvp, cnp->cn_nameptr, *vpp, ncp);
 		SDT_PROBE(vfs, namecache, lookup, hit, dvp, ncp->nc_name,
 		    *vpp, 0, 0);
+		if (tsp != NULL)
+			*tsp = ncp->nc_time;
+		if (ticksp != NULL)
+			*ticksp = ncp->nc_ticks;
 		goto success;
 	}
 
@@ -530,6 +545,10 @@ negative_success:
 		cnp->cn_flags |= ISWHITEOUT;
 	SDT_PROBE(vfs, namecache, lookup, hit_negative, dvp, ncp->nc_name,
 	    0, 0, 0);
+	if (tsp != NULL)
+		*tsp = ncp->nc_time;
+	if (ticksp != NULL)
+		*ticksp = ncp->nc_ticks;
 	CACHE_WUNLOCK();
 	return (ENOENT);
 
@@ -616,10 +635,11 @@ unlock:
  * Add an entry to the cache.
  */
 void
-cache_enter(dvp, vp, cnp)
+cache_enter_time(dvp, vp, cnp, tsp)
 	struct vnode *dvp;
 	struct vnode *vp;
 	struct componentname *cnp;
+	struct timespec *tsp;
 {
 	struct namecache *ncp, *n2;
 	struct nchashhead *ncpp;
@@ -692,6 +712,11 @@ cache_enter(dvp, vp, cnp)
 	ncp->nc_vp = vp;
 	ncp->nc_dvp = dvp;
 	ncp->nc_flag = flag;
+	if (tsp != NULL)
+		ncp->nc_time = *tsp;
+	else
+		timespecclear(&ncp->nc_time);
+	ncp->nc_ticks = ticks;
 	len = ncp->nc_nlen = cnp->cn_namelen;
 	hash = fnv_32_buf(cnp->cn_nameptr, len, FNV1_32_INIT);
 	strlcpy(ncp->nc_name, cnp->cn_nameptr, len + 1);
@@ -708,6 +733,8 @@ cache_enter(dvp, vp, cnp)
 		if (n2->nc_dvp == dvp &&
 		    n2->nc_nlen == cnp->cn_namelen &&
 		    !bcmp(n2->nc_name, cnp->cn_nameptr, n2->nc_nlen)) {
+			n2->nc_time = ncp->nc_time;
+			n2->nc_ticks = ncp->nc_ticks;
 			CACHE_WUNLOCK();
 			cache_free(ncp);
 			return;
@@ -1280,6 +1307,29 @@ vn_commname(struct vnode *vp, char *buf,
 	return (0);
 }
 
+/* ABI compat shims for old kernel modules. */
+#undef cache_enter
+#undef cache_lookup
+
+void	cache_enter(struct vnode *dvp, struct vnode *vp,
+	    struct componentname *cnp);
+int	cache_lookup(struct vnode *dvp, struct vnode **vpp,
+	    struct componentname *cnp);
+
+void
+cache_enter(struct vnode *dvp, struct vnode *vp, struct componentname *cnp)
+{
+
+	cache_enter_time(dvp, vp, cnp, NULL);
+}
+
+int
+cache_lookup(struct vnode *dvp, struct vnode **vpp, struct componentname *cnp)
+{
+
+	return (cache_lookup_times(dvp, vpp, cnp, NULL, NULL));
+}
+
 /*
  * This function updates path string to vnode's full global path
  * and checks the size of the new path string against the pathlen argument.

Modified: head/sys/nfsclient/nfs_subs.c
==============================================================================
--- head/sys/nfsclient/nfs_subs.c	Fri Jan 20 19:18:11 2012	(r230393)
+++ head/sys/nfsclient/nfs_subs.c	Fri Jan 20 20:02:01 2012	(r230394)
@@ -978,8 +978,8 @@ nfsm_loadattr_xx(struct vnode **v, struc
 }
 
 int
-nfsm_postop_attr_xx(struct vnode **v, int *f, struct mbuf **md,
-		    caddr_t *dpos)
+nfsm_postop_attr_xx(struct vnode **v, int *f, struct vattr *va,
+		    struct mbuf **md, caddr_t *dpos)
 {
 	u_int32_t *tl;
 	int t1;
@@ -990,7 +990,7 @@ nfsm_postop_attr_xx(struct vnode **v, in
 		return EBADRPC;
 	*f = fxdr_unsigned(int, *tl);
 	if (*f != 0) {
-		t1 = nfs_loadattrcache(&ttvp, md, dpos, NULL, 1);
+		t1 = nfs_loadattrcache(&ttvp, md, dpos, va, 1);
 		if (t1 != 0) {
 			*f = 0;
 			return t1;
@@ -1020,7 +1020,7 @@ nfsm_wcc_data_xx(struct vnode **v, int *
 				  VTONFS(*v)->n_mtime.tv_nsec == fxdr_unsigned(u_int32_t, *(tl + 3))); 
 		mtx_unlock(&(VTONFS(*v))->n_mtx);
 	}
-	t1 = nfsm_postop_attr_xx(v, &ttattrf, md, dpos);
+	t1 = nfsm_postop_attr_xx(v, &ttattrf, NULL, md, dpos);
 	if (t1)
 		return t1;
 	if (*f)

Modified: head/sys/nfsclient/nfs_vnops.c
==============================================================================
--- head/sys/nfsclient/nfs_vnops.c	Fri Jan 20 19:18:11 2012	(r230393)
+++ head/sys/nfsclient/nfs_vnops.c	Fri Jan 20 20:02:01 2012	(r230394)
@@ -913,7 +913,7 @@ nfs_lookup(struct vop_lookup_args *ap)
 	struct vnode **vpp = ap->a_vpp;
 	struct mount *mp = dvp->v_mount;
 	struct vattr vattr;
-	struct timespec dmtime;
+	struct timespec nctime;
 	int flags = cnp->cn_flags;
 	struct vnode *newvp;
 	struct nfsmount *nmp;
@@ -922,7 +922,7 @@ nfs_lookup(struct vop_lookup_args *ap)
 	long len;
 	nfsfh_t *fhp;
 	struct nfsnode *np, *newnp;
-	int error = 0, attrflag, fhsize, ltype;
+	int error = 0, attrflag, dattrflag, fhsize, ltype, ncticks;
 	int v3 = NFS_ISV3(dvp);
 	struct thread *td = cnp->cn_thread;
 
@@ -938,11 +938,24 @@ nfs_lookup(struct vop_lookup_args *ap)
 		*vpp = NULLVP;
 		return (error);
 	}
-	error = cache_lookup(dvp, vpp, cnp);
+	error = cache_lookup_times(dvp, vpp, cnp, &nctime, &ncticks);
 	if (error > 0 && error != ENOENT)
 		return (error);
 	if (error == -1) {
 		/*
+		 * Lookups of "." are special and always return the
+		 * current directory.  cache_lookup() already handles
+		 * associated locking bookkeeping, etc.
+		 */
+		if (cnp->cn_namelen == 1 && cnp->cn_nameptr[0] == '.') {
+			/* XXX: Is this really correct? */
+			if (cnp->cn_nameiop != LOOKUP &&
+			    (flags & ISLASTCN))
+				cnp->cn_flags |= SAVENAME;
+			return (0);
+		}
+
+		/*
 		 * We only accept a positive hit in the cache if the
 		 * change time of the file matches our cached copy.
 		 * Otherwise, we discard the cache entry and fallback
@@ -968,7 +981,7 @@ nfs_lookup(struct vop_lookup_args *ap)
 			mtx_unlock(&newnp->n_mtx);
 		}
 		if (VOP_GETATTR(newvp, &vattr, cnp->cn_cred) == 0 &&
-		    timespeccmp(&vattr.va_ctime, &newnp->n_ctime, ==)) {
+		    timespeccmp(&vattr.va_ctime, &nctime, ==)) {
 			nfsstats.lookupcache_hits++;
 			if (cnp->cn_nameiop != LOOKUP &&
 			    (flags & ISLASTCN))
@@ -987,36 +1000,22 @@ nfs_lookup(struct vop_lookup_args *ap)
 		/*
 		 * We only accept a negative hit in the cache if the
 		 * modification time of the parent directory matches
-		 * our cached copy.  Otherwise, we discard all of the
-		 * negative cache entries for this directory. We also
-		 * only trust -ve cache entries for less than
-		 * nm_negative_namecache_timeout seconds.
+		 * the cached copy in the name cache entry.
+		 * Otherwise, we discard all of the negative cache
+		 * entries for this directory.  We also only trust
+		 * negative cache entries for up to nm_negnametimeo
+		 * seconds.
 		 */
-		if ((u_int)(ticks - np->n_dmtime_ticks) <
-		    (nmp->nm_negnametimeo * hz) &&
+		if ((u_int)(ticks - ncticks) < (nmp->nm_negnametimeo * hz) &&
 		    VOP_GETATTR(dvp, &vattr, cnp->cn_cred) == 0 &&
-		    timespeccmp(&vattr.va_mtime, &np->n_dmtime, ==)) {
+		    timespeccmp(&vattr.va_mtime, &nctime, ==)) {
 			nfsstats.lookupcache_hits++;
 			return (ENOENT);
 		}
 		cache_purge_negative(dvp);
-		mtx_lock(&np->n_mtx);
-		timespecclear(&np->n_dmtime);
-		mtx_unlock(&np->n_mtx);
 	}
 
-	/*
-	 * Cache the modification time of the parent directory in case
-	 * the lookup fails and results in adding the first negative
-	 * name cache entry for the directory.  Since this is reading
-	 * a single time_t, don't bother with locking.  The
-	 * modification time may be a bit stale, but it must be read
-	 * before performing the lookup RPC to prevent a race where
-	 * another lookup updates the timestamp on the directory after
-	 * the lookup RPC has been performed on the server but before
-	 * n_dmtime is set at the end of this function.
-	 */
-	dmtime = np->n_vattr.va_mtime;
+	attrflag = dattrflag = 0;
 	error = 0;
 	newvp = NULLVP;
 	nfsstats.lookupcache_misses++;
@@ -1031,7 +1030,7 @@ nfs_lookup(struct vop_lookup_args *ap)
 	nfsm_request(dvp, NFSPROC_LOOKUP, cnp->cn_thread, cnp->cn_cred);
 	if (error) {
 		if (v3) {
-			nfsm_postop_attr(dvp, attrflag);
+			nfsm_postop_attr_va(dvp, dattrflag, &vattr);
 			m_freem(mrep);
 		}
 		goto nfsmout;
@@ -1127,16 +1126,17 @@ nfs_lookup(struct vop_lookup_args *ap)
 		}
 	}
 	if (v3) {
-		nfsm_postop_attr(newvp, attrflag);
-		nfsm_postop_attr(dvp, attrflag);
-	} else
-		nfsm_loadattr(newvp, NULL);
+		nfsm_postop_attr_va(newvp, attrflag, &vattr);
+		nfsm_postop_attr(dvp, dattrflag);
+	} else {
+		nfsm_loadattr(newvp, &vattr);
+		attrflag = 1;
+	}
 	if (cnp->cn_nameiop != LOOKUP && (flags & ISLASTCN))
 		cnp->cn_flags |= SAVENAME;
 	if ((cnp->cn_flags & MAKEENTRY) &&
-	    (cnp->cn_nameiop != DELETE || !(flags & ISLASTCN))) {
-		np->n_ctime = np->n_vattr.va_ctime;
-		cache_enter(dvp, newvp, cnp);
+	    (cnp->cn_nameiop != DELETE || !(flags & ISLASTCN)) && attrflag) {
+		cache_enter_time(dvp, newvp, cnp, &vattr.va_ctime);
 	}
 	*vpp = newvp;
 	m_freem(mrep);
@@ -1164,30 +1164,22 @@ nfsmout:
 			return (EJUSTRETURN);
 		}
 
-		if ((cnp->cn_flags & MAKEENTRY) && cnp->cn_nameiop != CREATE) {
+		if ((cnp->cn_flags & MAKEENTRY) && cnp->cn_nameiop != CREATE &&
+		    dattrflag) {
 			/*
-			 * Maintain n_dmtime as the modification time
-			 * of the parent directory when the oldest -ve
-			 * name cache entry for this directory was
-			 * added.  If a -ve cache entry has already
-			 * been added with a newer modification time
-			 * by a concurrent lookup, then don't bother
-			 * adding a cache entry.  The modification
-			 * time of the directory might have changed
-			 * due to the file this lookup failed to find
-			 * being created.  In that case a subsequent
-			 * lookup would incorrectly use the entry
-			 * added here instead of doing an extra
-			 * lookup.
+			 * Cache the modification time of the parent
+			 * directory from the post-op attributes in
+			 * the name cache entry.  The negative cache
+			 * entry will be ignored once the directory
+			 * has changed.  Don't bother adding the entry
+			 * if the directory has already changed.
 			 */
 			mtx_lock(&np->n_mtx);
-			if (timespeccmp(&np->n_dmtime, &dmtime, <=)) {
-				if (!timespecisset(&np->n_dmtime)) {
-					np->n_dmtime = dmtime;
-					np->n_dmtime_ticks = ticks;
-				}
+			if (timespeccmp(&np->n_vattr.va_mtime,
+			    &vattr.va_mtime, ==)) {
 				mtx_unlock(&np->n_mtx);
-				cache_enter(dvp, NULL, cnp);
+				cache_enter_time(dvp, NULL, cnp,
+				    &vattr.va_mtime);
 			} else
 				mtx_unlock(&np->n_mtx);
 		}
@@ -2473,6 +2465,7 @@ nfs_readdirplusrpc(struct vnode *vp, str
 	nfsuint64 cookie;
 	struct nfsmount *nmp = VFSTONFS(vp->v_mount);
 	struct nfsnode *dnp = VTONFS(vp), *np;
+	struct vattr vattr;
 	nfsfh_t *fhp;
 	u_quad_t fileno;
 	int error = 0, tlen, more_dirs = 1, blksiz = 0, doit, bigenough = 1, i;
@@ -2653,18 +2646,13 @@ nfs_readdirplusrpc(struct vnode *vp, str
 				dpos = dpossav1;
 				mdsav2 = md;
 				md = mdsav1;
-				nfsm_loadattr(newvp, NULL);
+				nfsm_loadattr(newvp, &vattr);
 				dpos = dpossav2;
 				md = mdsav2;
-				dp->d_type =
-				    IFTODT(VTTOIF(np->n_vattr.va_type));
+				dp->d_type = IFTODT(VTTOIF(vattr.va_type));
 				ndp->ni_vp = newvp;
-				/*
-				 * Update n_ctime so subsequent lookup
-				 * doesn't purge entry.
-				 */
-				np->n_ctime = np->n_vattr.va_ctime;
-			        cache_enter(ndp->ni_dvp, ndp->ni_vp, cnp);
+			        cache_enter_time(ndp->ni_dvp, ndp->ni_vp, cnp,
+				    &vattr.va_ctime);
 			    }
 			} else {
 			    /* Just skip over the file handle */

Modified: head/sys/nfsclient/nfsm_subs.h
==============================================================================
--- head/sys/nfsclient/nfsm_subs.h	Fri Jan 20 19:18:11 2012	(r230393)
+++ head/sys/nfsclient/nfsm_subs.h	Fri Jan 20 20:02:01 2012	(r230394)
@@ -152,8 +152,8 @@ int	nfsm_getfh_xx(nfsfh_t **f, int *s, i
 	    caddr_t *dpos);
 int	nfsm_loadattr_xx(struct vnode **v, struct vattr *va, struct mbuf **md,
 	    caddr_t *dpos);
-int	nfsm_postop_attr_xx(struct vnode **v, int *f, struct mbuf **md,
-	    caddr_t *dpos);
+int	nfsm_postop_attr_xx(struct vnode **v, int *f, struct vattr *va,
+	    struct mbuf **md, caddr_t *dpos);
 int	nfsm_wcc_data_xx(struct vnode **v, int *f, struct mbuf **md,
 	    caddr_t *dpos);
 
@@ -181,7 +181,14 @@ do { \
 #define	nfsm_postop_attr(v, f) \
 do { \
 	int32_t t1; \
-	t1 = nfsm_postop_attr_xx(&v, &f, &md, &dpos); \
+	t1 = nfsm_postop_attr_xx(&v, &f, NULL, &md, &dpos);	\
+	nfsm_dcheck(t1, mrep); \
+} while (0)
+
+#define	nfsm_postop_attr_va(v, f, va)		\
+do { \
+	int32_t t1; \
+	t1 = nfsm_postop_attr_xx(&v, &f, va, &md, &dpos);	\
 	nfsm_dcheck(t1, mrep); \
 } while (0)
 

Modified: head/sys/nfsclient/nfsnode.h
==============================================================================
--- head/sys/nfsclient/nfsnode.h	Fri Jan 20 19:18:11 2012	(r230393)
+++ head/sys/nfsclient/nfsnode.h	Fri Jan 20 20:02:01 2012	(r230394)
@@ -104,9 +104,6 @@ struct nfsnode {
 	time_t			n_attrstamp;	/* Attr. cache timestamp */
 	struct nfs_accesscache	n_accesscache[NFS_ACCESSCACHESIZE];
 	struct timespec		n_mtime;	/* Prev modify time. */
-	struct timespec		n_ctime;	/* Prev create time. */
-	struct timespec		n_dmtime;	/* Prev dir modify time. */
-	int			n_dmtime_ticks;	/* Tick of -ve cache entry */
 	nfsfh_t			*n_fhp;		/* NFS File Handle */
 	struct vnode		*n_vnode;	/* associated vnode */
 	struct vnode		*n_dvp;		/* parent vnode */

Modified: head/sys/sys/vnode.h
==============================================================================
--- head/sys/sys/vnode.h	Fri Jan 20 19:18:11 2012	(r230393)
+++ head/sys/sys/vnode.h	Fri Jan 20 20:02:01 2012	(r230394)
@@ -578,10 +578,14 @@ struct vattr;
 struct vnode;
 
 /* cache_* may belong in namei.h. */
-void	cache_enter(struct vnode *dvp, struct vnode *vp,
-	    struct componentname *cnp);
-int	cache_lookup(struct vnode *dvp, struct vnode **vpp,
-	    struct componentname *cnp);
+#define	cache_enter(dvp, vp, cnp)					\
+	cache_enter_time(dvp, vp, cnp, NULL)
+void	cache_enter_time(struct vnode *dvp, struct vnode *vp,
+	    struct componentname *cnp, struct timespec *tsp);
+#define	cache_lookup(dvp, vpp, cnp)					\
+	cache_lookup_times(dvp, vpp, cnp, NULL, NULL)
+int	cache_lookup_times(struct vnode *dvp, struct vnode **vpp,
+	    struct componentname *cnp, struct timespec *tsp, int *ticksp);
 void	cache_purge(struct vnode *vp);
 void	cache_purge_negative(struct vnode *vp);
 void	cache_purgevfs(struct mount *mp);



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201201202002.q0KK21M6056471>