Re: cache_inval

[Matthew Dillon <dillon@xxxxxxxxxxxxxxxxxxxx>]

    Ok Richard, here is a patch I've worked up to fix cache_inval().  It
    now no longer disconnects the namecache records from the topology, it
    simply sets them to an unresolved state (recursively if CINV_CHILDREN is
    specified).  CINV_DESTROY tells it to invalidate the name of the
    passed ncp so cache_nlookup() no longer see it.  CINV_DESTROY is not
    recursive. 

    cache_inval() now does CINV_SELF automatically (that is, sets the passed
    ncp to an unresolved state), and the flag has been removed.

    cache_inval(..., CINV_SELF, CINV_CHILDREN) should now do what you need
    it to do.  Note that the ncp you pass into cache_inval() MUST be locked,
    and will remain locked on return.

    Also, it turns out that I misspoke in my last posting... the recursion is
    *NOT* mandatory because the parent link is left intact.  It was only 
    mandatory when the parent is unlinked (which no longer occurs anywhere).

    Anyone else who would like to try this patch, it does need testing
    before I commit it.  I don't expect any serious problems but there's
    always the chance of hitting a deadlock situation when the locks are
    messed around with, and I definitely mess around with the locks here.

						-Matt

Index: kern/vfs_cache.c
===================================================================
RCS file: /cvs/src/sys/kern/vfs_cache.c,v
retrieving revision 1.42
diff -u -r1.42 vfs_cache.c
--- kern/vfs_cache.c	12 Nov 2004 00:09:24 -0000	1.42
+++ kern/vfs_cache.c	17 Nov 2004 01:41:40 -0000
@@ -565,13 +565,33 @@
 }
 
 /*
- * Invalidate portions of a namecache entry.  The passed ncp should be
- * referenced and locked but we might not adhere to that rule during the
- * old api -> new api transition period.
- *
- * CINV_PARENT		- disconnect the ncp from its parent
- * CINV_SELF		- same as cache_setunresolved(ncp)
- * CINV_CHILDREN	- disconnect children of the ncp from the ncp
+ * Invalidate portions of the namecache topology given a starting entry.
+ * The passed ncp is set to an unresolved state and:
+ *
+ * The passed ncp must be locked.
+ *
+ * CINV_DESTROY		- Set a flag in the passed ncp entry indicating
+ *			  that the physical underlying nodes have been 
+ *			  destroyed... as in deleted.  For example, when
+ *			  a directory is removed.  This will cause record
+ *			  lookups on the name to no longer be able to find
+ *			  the record and tells the resolver to return failure
+ *			  rather then trying to resolve through the parent.
+ *
+ *			  The topology itself, including ncp->nc_name,
+ *			  remains intact.
+ *
+ *			  This only applies to the passed ncp, if CINV_CHILDREN
+ *			  is specified the children are not flagged.
+ *
+ * CINV_CHILDREN	- Set all children (recursively) to an unresolved
+ *			  state as well.
+ *
+ *			  Note that this will also have the side effect of
+ *			  cleaning out any unreferenced nodes in the topology
+ *			  from the leaves up as the recursion backs out.
+ *
+ * Note that the topology for any referenced nodes remains intact.
  */
 void
 cache_inval(struct namecache *ncp, int flags)
@@ -579,59 +599,53 @@
 	struct namecache *kid;
 	struct namecache *nextkid;
 
-	if (flags & CINV_SELF)
-		cache_setunresolved(ncp);
-	if (flags & CINV_PARENT)
-		cache_unlink_parent(ncp);
+	KKASSERT(ncp->nc_exlocks);
+again:
+	cache_setunresolved(ncp);
+	if (flags & CINV_DESTROY)
+		ncp->nc_flag |= NCF_DESTROYED;
 
-	/*
-	 * Children are invalidated when the parent is destroyed.  This
-	 * basically disconnects the children from the parent.  Anyone
-	 * CD'd into a child will no longer be able to ".." back up.
-	 *
-	 * Any unresolved or negative cache-hit children with a ref count
-	 * of 0 must be immediately and recursively destroyed or this
-	 * disconnection may leave them dangling forever.  XXX this recursion
-	 * could run the kernel out of stack, the children should be placed
-	 * on a to-destroy list instead.
-	 */
-	if (flags & CINV_CHILDREN) {
-		if ((kid = TAILQ_FIRST(&ncp->nc_list)) != NULL)
-			cache_hold(kid);
+	if ((flags & CINV_CHILDREN) && 
+	    (kid = TAILQ_FIRST(&ncp->nc_list)) != NULL
+	) {
+		cache_hold(kid);
+		cache_unlock(ncp);
 		while (kid) {
 			if ((nextkid = TAILQ_NEXT(kid, nc_entry)) != NULL)
 				cache_hold(nextkid);
-			if (kid->nc_refs == 0 &&
-			    ((kid->nc_flag & NCF_UNRESOLVED) || 
-			     kid->nc_vp == NULL)
+			if ((kid->nc_flag & NCF_UNRESOLVED) == 0 ||
+			    TAILQ_FIRST(&kid->nc_list)
 			) {
-				cache_inval(kid, CINV_PARENT);
+				cache_lock(kid);
+				cache_inval(kid, flags & ~CINV_DESTROY);
+				cache_unlock(kid);
 			}
-			cache_unlink_parent(kid);
 			cache_drop(kid);
 			kid = nextkid;
 		}
+		cache_lock(ncp);
+
+		/*
+		 * Someone could have gotten in there while ncp was unlocked,
+		 * retry if so.
+		 */
+		if ((ncp->nc_flag & NCF_UNRESOLVED) == 0)
+			goto again;
 	}
 }
 
+/*
+ * Invalidate a vnode's namecache associations.
+ */
 void
 cache_inval_vp(struct vnode *vp, int flags)
 {
 	struct namecache *ncp;
 
-	if (flags & CINV_SELF) {
-		while ((ncp = TAILQ_FIRST(&vp->v_namecache)) != NULL) {
-			cache_hold(ncp);
-			KKASSERT((ncp->nc_flag & NCF_UNRESOLVED) == 0);
-			cache_inval(ncp, flags);
-			cache_drop(ncp);
-		}
-	} else {
-		TAILQ_FOREACH(ncp, &vp->v_namecache, nc_vnode) {
-			cache_hold(ncp);
-			cache_inval(ncp, flags);
-			cache_drop(ncp);
-		}
+	while ((ncp = TAILQ_FIRST(&vp->v_namecache)) != NULL) {
+		cache_get(ncp);
+		cache_inval(ncp, flags);
+		cache_put(ncp);
 	}
 }
 
@@ -642,6 +656,10 @@
  * destroyed by the rename operation, e.g. renaming over an empty directory),
  * and all children of fncp will be moved to tncp.
  *
+ * XXX the disconnection could pose a problem, check code paths to make
+ * sure any code that blocks can handle the parent being changed out from
+ * under it.  Maybe we should lock the children (watch out for deadlocks) ?
+ *
  * After we return the caller has the option of calling cache_setvp() if
  * the vnode of the new target ncp is known.
  *
@@ -1185,11 +1203,13 @@
 
 		/*
 		 * Break out if we find a matching entry.  Note that
-		 * UNRESOLVED entries may match.
+		 * UNRESOLVED entries may match, but DESTROYED entries
+		 * do not.
 		 */
 		if (ncp->nc_parent == par &&
 		    ncp->nc_nlen == nlc->nlc_namelen &&
-		    bcmp(ncp->nc_name, nlc->nlc_nameptr, ncp->nc_nlen) == 0
+		    bcmp(ncp->nc_name, nlc->nlc_nameptr, ncp->nc_nlen) == 0 &&
+		    (ncp->nc_flag & NCF_DESTROYED) == 0
 		) {
 			if (cache_get_nonblock(ncp) == 0) {
 				if (new_ncp)
@@ -1300,6 +1320,7 @@
 	 *	- due to NFS being stupid about tracking the namespace and
 	 *	  destroys the namespace for entire directories quite often.
 	 *	- due to forced unmounts.
+	 *	- due to an rmdir (parent will be marked DESTROYED)
 	 *
 	 * When this occurs we have to track the chain backwards and resolve
 	 * it, looping until the resolver catches up to the current node.  We
@@ -1309,6 +1330,14 @@
 	 * many nodes to resolve the ncp.
 	 */
 	while (ncp->nc_parent->nc_vp == NULL) {
+		/*
+		 * This case can occur if a process is CD'd into a
+		 * directory which is then rmdir'd.  If the parent is marked
+		 * destroyed there is no point trying to resolve it.
+		 */
+		if (ncp->nc_parent->nc_flag & NCF_DESTROYED)
+			return(ENOENT);
+
 		par = ncp->nc_parent;
 		while (par->nc_parent && par->nc_parent->nc_vp == NULL)
 			par = par->nc_parent;
@@ -1549,7 +1578,7 @@
 {
 	static u_long nextid;
 
-	cache_inval_vp(vp, CINV_PARENT | CINV_SELF | CINV_CHILDREN);
+	cache_inval_vp(vp, CINV_DESTROY | CINV_CHILDREN);
 
 	/*
 	 * Calculate a new unique id for ".." handling
Index: kern/vfs_default.c
===================================================================
RCS file: /cvs/src/sys/kern/vfs_default.c,v
retrieving revision 1.22
diff -u -r1.22 vfs_default.c
--- kern/vfs_default.c	12 Nov 2004 00:09:24 -0000	1.22
+++ kern/vfs_default.c	17 Nov 2004 01:20:02 -0000
@@ -959,7 +959,7 @@
 		 * back out.
 		 */
 		if (error == 0)
-			cache_inval(ncp, CINV_SELF|CINV_PARENT);
+			cache_inval(ncp, CINV_DESTROY);
 	}
 	if (vp) {
 		if (dvp == vp)
Index: kern/vfs_nlookup.c
===================================================================
RCS file: /cvs/src/sys/kern/vfs_nlookup.c,v
retrieving revision 1.7
diff -u -r1.7 vfs_nlookup.c
--- kern/vfs_nlookup.c	12 Nov 2004 00:09:24 -0000	1.7
+++ kern/vfs_nlookup.c	17 Nov 2004 01:25:31 -0000
@@ -357,10 +357,16 @@
 		ncp = cache_get(ncp);
 	    } else {
 		while ((ncp->nc_flag & NCF_MOUNTPT) && ncp != nd->nl_rootncp) {
+		    if (ncp->nc_parent->nc_flag & NCF_DESTROYED)
+			break;
 		    ncp = ncp->nc_parent;	/* get to underlying node */
 		    KKASSERT(ncp != NULL && 1);
 		}
 		if (ncp != nd->nl_rootncp) {
+			if (ncp->nc_parent->nc_flag & NCF_DESTROYED) {
+				error = EINVAL;
+				break;
+			}
 			ncp = ncp->nc_parent;
 			if (ncp == NULL) {
 				error = EINVAL;
Index: kern/vfs_subr.c
===================================================================
RCS file: /cvs/src/sys/kern/vfs_subr.c,v
retrieving revision 1.47
diff -u -r1.47 vfs_subr.c
--- kern/vfs_subr.c	12 Nov 2004 10:58:59 -0000	1.47
+++ kern/vfs_subr.c	17 Nov 2004 00:58:35 -0000
@@ -827,7 +827,7 @@
 	/*
 	 * Scrap the vfs cache
 	 */
-	cache_inval_vp(vp, CINV_SELF);
+	cache_inval_vp(vp, 0);
 
 	/*
 	 * Check to see if the vnode is in use. If so we have to reference it
Index: sys/namecache.h
===================================================================
RCS file: /cvs/src/sys/sys/namecache.h,v
retrieving revision 1.15
diff -u -r1.15 namecache.h
--- sys/namecache.h	12 Nov 2004 00:09:27 -0000	1.15
+++ sys/namecache.h	17 Nov 2004 01:03:30 -0000
@@ -130,13 +130,14 @@
 #define NCF_UNUSED080	0x0080
 #define NCF_ISSYMLINK	0x0100	/* represents a symlink */
 #define NCF_ISDIR	0x0200	/* represents a directory */
+#define NCF_DESTROYED	0x0400	/* name association is considered destroyed */
 
 /*
  * cache_inval[_vp]() flags
  */
-#define CINV_PARENT	0x0001	/* disconnect from parent in namecache */
-#define CINV_SELF	0x0002	/* disconnect vp from namecache */
-#define CINV_CHILDREN	0x0004	/* disconnect children in namecache */
+#define CINV_DESTROY	0x0001	/* flag so cache_nlookup ignores the ncp */
+#define CINV_UNUSED02	0x0002
+#define CINV_CHILDREN	0x0004	/* recursively set children to unresolved */
 
 #ifdef _KERNEL
 
Index: vfs/nfs/nfs_nqlease.c
===================================================================
RCS file: /cvs/src/sys/vfs/nfs/nfs_nqlease.c,v
retrieving revision 1.20
diff -u -r1.20 nfs_nqlease.c
--- vfs/nfs/nfs_nqlease.c	12 Nov 2004 00:09:37 -0000	1.20
+++ vfs/nfs/nfs_nqlease.c	17 Nov 2004 00:59:57 -0000
@@ -1062,7 +1062,7 @@
 					if (np->n_flag & NQNFSEVICTED) {
 						if (vp->v_type == VDIR)
 							nfs_invaldir(vp);
-						cache_inval_vp(vp, CINV_SELF);
+						cache_inval_vp(vp, 0);
 						(void) nfs_vinvalbuf(vp,
 						       V_SAVE, td, 0);
 						np->n_flag &= ~NQNFSEVICTED;
Index: vfs/nfs/nfs_socket.c
===================================================================
RCS file: /cvs/src/sys/vfs/nfs/nfs_socket.c,v
retrieving revision 1.21
diff -u -r1.21 nfs_socket.c
--- vfs/nfs/nfs_socket.c	12 Nov 2004 00:09:37 -0000	1.21
+++ vfs/nfs/nfs_socket.c	17 Nov 2004 01:00:19 -0000
@@ -1166,7 +1166,7 @@
 			 * lookup cache, just in case.
 			 */
 			if (error == ESTALE)
-				cache_inval_vp(vp, CINV_SELF);
+				cache_inval_vp(vp, CINV_CHILDREN);
 			if (nmp->nm_flag & NFSMNT_NFSV3) {
 				*mrp = mrep;
 				*mdp = md;