from jtk@kolvir.arlington.ma.us:

numerous stability fixes related to locking.

1 files changed, 144 insertions, 17 deletions

diff --git a/sys/miscfs/union/union_vnops.c b/sys/miscfs/union/union_vnops.c
index d921b568f8c..fa4482484e5 100644
--- a/sys/miscfs/union/union_vnops.c
+++ b/sys/miscfs/union/union_vnops.c
@@ -201,8 +201,40 @@ union_lookup(ap)
 	 */
 	if (upperdvp != NULLVP) {
 		FIXUP(dun);
+		/*
+		 * If we're doing `..' in the underlying filesystem,
+		 * we must drop our lock on the union node before
+		 * going up the tree in the lower file system--if we block
+		 * on the lowervp lock, and that's held by someone else
+		 * coming down the tree and who's waiting for our lock,
+		 * we would be hosed.
+		 */
+		if (cnp->cn_flags & ISDOTDOT) {
+			/* retain lock on underlying VP: */
+			dun->un_flags |= UN_KLOCK;
+			VOP_UNLOCK(dvp);
+		}
 		uerror = union_lookup1(um->um_uppervp, &upperdvp,
 					&uppervp, cnp);
+		if (cnp->cn_flags & ISDOTDOT) {
+			if (dun->un_uppervp == upperdvp) {
+				/*
+				 * we got the underlying bugger back locked...
+				 * now take back the union node lock.  Since we
+				 * hold the uppervp lock, we can diddle union
+				 * locking flags at will. :)
+				 */
+				dun->un_flags |= UN_ULOCK;
+			}
+			/*
+			 * if upperdvp got swapped out, it means we did
+			 * some mount point magic, and we do not have
+			 * dun->un_uppervp locked currently--so we get it
+			 * locked here (don't set the UN_ULOCK flag).
+			 */
+			VOP_LOCK(dvp);
+		}
+
 		/*if (uppervp == upperdvp)
 			dun->un_flags |= UN_KLOCK;*/
 
@@ -248,6 +280,12 @@ union_lookup(ap)
 			saved_cred = cnp->cn_cred;
 			cnp->cn_cred = um->um_cred;
 		}
+		/*
+		 * we shouldn't have to worry about locking interactions
+		 * between the lower layer and our union layer (w.r.t.
+		 * `..' processing) because we don't futz with lowervp
+		 * locks in the union-node instantiation code path.
+		 */
 		lerror = union_lookup1(um->um_lowervp, &lowerdvp,
 				&lowervp, cnp);
 		if (um->um_op == UNMNT_BELOW)
@@ -319,6 +357,11 @@ union_lookup(ap)
 	/* case 2. */
 	if (uerror != 0 /* && (lerror == 0) */ ) {
 		if (lowervp->v_type == VDIR) { /* case 2b. */
+			/*
+			 * We may be racing another process to make the
+			 * upper-level shadow directory.  Be careful with
+			 * locks/etc!
+			 */
 			dun->un_flags &= ~UN_ULOCK;
 			VOP_UNLOCK(upperdvp);
 			uerror = union_mkshadow(um, upperdvp, cnp, &uppervp);
@@ -350,6 +393,13 @@ union_lookup(ap)
 		if (*ap->a_vpp != dvp)
 			if (!lockparent || !(cnp->cn_flags & ISLASTCN))
 				VOP_UNLOCK(dvp);
+		if (cnp->cn_namelen == 1 &&
+			cnp->cn_nameptr[0] == '.' &&
+			*ap->a_vpp != dvp) {
+			panic("union_lookup returning. (%x) not same as startdir (%x)",
+			    ap->a_vpp, dvp);
+		}
+
 	}
 
 	return (error);
@@ -537,15 +587,14 @@ union_close(ap)
 		--un->un_openl;
 		vp = un->un_lowervp;
 	}
-
-#ifdef	DIAGNOSTIC
+#ifdef UNION_DIAGNOSTIC
 	/*
-	 * We should never encounter a vnode with both upper and
-	 * lower vnodes NULL.
+	 * A stranded union node may end up here with both vnodes NULL,
+	 * in which case we don't do anything.
 	 */
 	if (vp == NULLVP) {
-		vprint("empty union vnode", vp);
-		panic("union_close empty vnode");
+	    vprint("empty union vnode", vp);
+	    panic("union_close empty vnode");
 	}
 #endif
 
@@ -941,6 +990,10 @@ union_remove(ap)
 	return (error);
 }
 
+/* a_vp: directory in which to link
+   a_tdvp: new target of the link
+   a_cnp: name for the link
+   */
 int
 union_link(ap)
 	struct vop_link_args /* {
@@ -956,21 +1009,56 @@ union_link(ap)
 
 	un = VTOUNION(ap->a_vp);
 
+#ifdef DIAGNOSTIC
+	if (!(ap->a_cnp->cn_flags & LOCKPARENT)) {
+		printf("union_link called without LOCKPARENT set!\n");
+		error = EIO; /* need some error code for "caller is a bozo" */
+	} else
+#endif
 	if (ap->a_vp->v_op != ap->a_tdvp->v_op) {
 		tdvp = ap->a_tdvp;
 	} else {
 		struct union_node *tdun = VTOUNION(ap->a_tdvp);
 		if (tdun->un_uppervp == NULLVP) {
+			/*
+			 * needs to be copied up before we can link it.
+			 */
 			VOP_LOCK(ap->a_tdvp);
 			if (un->un_uppervp == tdun->un_dirvp) {
-				un->un_flags &= ~UN_ULOCK;
-				VOP_UNLOCK(un->un_uppervp);
+				VOP_UNLOCK(ap->a_vp);
 			}
 			error = union_copyup(tdun, 1, ap->a_cnp->cn_cred,
 						ap->a_cnp->cn_proc);
 			if (un->un_uppervp == tdun->un_dirvp) {
-				VOP_LOCK(un->un_uppervp);
-				un->un_flags |= UN_ULOCK;
+				/* During copyup, we dropped the lock on the
+				 * dir and invalidated any saved namei lookup
+				 * state for the directory we'll be entering
+				 * the link in.  We need to re-run the lookup
+				 * in that directory to reset any state needed
+				 * for VOP_LINK.
+				 * Call relookup on the union-layer to
+				 * reset the state.
+				 */
+				vp  = NULLVP;
+				if (un->un_uppervp == NULLVP ||
+				    /*
+				     * relookup starts with an unlocked node,
+				     * and since LOCKPARENT is set returns
+				     * the starting directory locked.
+				     */
+				    (error = relookup(ap->a_vp,
+						      &vp, ap->a_cnp))) {
+					vrele(ap->a_vp);
+					VOP_UNLOCK(ap->a_tdvp);
+					return EROFS;
+				}
+				if (vp != NULLVP) {
+					/* The name we want to create has
+					   mysteriously appeared (a race?) */
+					error = EEXIST;
+					VOP_UNLOCK(ap->a_tdvp);
+					goto croak;
+				}
 			}
 			VOP_UNLOCK(ap->a_tdvp);
 		}
@@ -982,6 +1070,7 @@ union_link(ap)
 		error = EROFS;
 
 	if (error) {
+croak:
 		vput(ap->a_vp);
 		return (error);
 	}
@@ -1011,6 +1100,7 @@ union_rename(ap)
 	struct vnode *fvp = ap->a_fvp;
 	struct vnode *tdvp = ap->a_tdvp;
 	struct vnode *tvp = ap->a_tvp;
+	struct union_node *unfile = (struct union_node *)0;
 
 	if (fdvp->v_op == union_vnodeop_p) {	/* always true */
 		struct union_node *un = VTOUNION(fdvp);
@@ -1031,19 +1121,19 @@ union_rename(ap)
 	}
 
 	if (fvp->v_op == union_vnodeop_p) {	/* always true */
-		struct union_node *un = VTOUNION(fvp);
-		if (un->un_uppervp == NULLVP) {
+		unfile = VTOUNION(fvp);
+		if (unfile->un_uppervp == NULLVP) {
 			/* XXX: should do a copyup */
 			error = EXDEV;
 			goto bad;
 		}
 
-		if (un->un_lowervp != NULLVP)
+		if (unfile->un_lowervp != NULLVP)
 			ap->a_fcnp->cn_flags |= DOWHITEOUT;
 
-		fvp = un->un_uppervp;
+		fvp = unfile->un_uppervp;
 		VREF(fvp);
-		vrele(ap->a_fvp);
+		/* vrele(ap->a_fvp); */ /* hold for later */
 	}
 
 	if (tdvp->v_op == union_vnodeop_p) {
@@ -1056,7 +1146,7 @@ union_rename(ap)
 			 * directory.
 			 */
 			error = EXDEV;
-			goto bad;
+			goto badrele;
 		}
 
 		tdvp = un->un_uppervp;
@@ -1073,11 +1163,24 @@ union_rename(ap)
 			VREF(tvp);
 			un->un_flags |= UN_KLOCK;
 		}
+#if 0
+		/* XXX should we toss from the cache? */
+		if (un->un_flags & UN_CACHED) {
+			un->un_flags &= ~UN_CACHED;
+			LIST_REMOVE(un, un_cache);
+		}
+#endif
 		vput(ap->a_tvp);
 	}
 
-	return (VOP_RENAME(fdvp, fvp, ap->a_fcnp, tdvp, tvp, ap->a_tcnp));
+	error = VOP_RENAME(fdvp, fvp, ap->a_fcnp, tdvp, tvp, ap->a_tcnp);
+	if (!error && unfile)
+	    union_removed_upper(unfile);
+	vrele(ap->a_fvp);
+	return error;
 
+badrele:
+	vrele(ap->a_fvp);
 bad:
 	vrele(fdvp);
 	vrele(fvp);
@@ -1358,6 +1461,12 @@ start:
 	if (un->un_uppervp != NULLVP) {
 		if (((un->un_flags & UN_ULOCK) == 0) &&
 		    (vp->v_usecount != 0)) {
+			/*
+			 * We MUST always use the order of: take upper
+			 * vp lock, manipulate union node flags, drop
+			 * upper vp lock.  This code must not be an
+			 * exception.
+			 */
 			VOP_LOCK(un->un_uppervp);
 			un->un_flags |= UN_ULOCK;
 		}
@@ -1391,6 +1500,18 @@ start:
 	return (0);
 }
 
+/*
+ * When operations want to vput() a union node yet retain a lock on
+ * the upper VP (say, to do some further operations like link(),
+ * mkdir(), ...), they set UN_KLOCK on the union node, then call
+ * vput() which calls VOP_UNLOCK() and comes here.  union_unlock()
+ * unlocks the union node (leaving the upper VP alone), clears the
+ * KLOCK flag, and then returns to vput().  The caller then does whatever
+ * is left to do with the upper VP, and insures that it gets unlocked.
+ *
+ * If UN_KLOCK isn't set, then the upper VP is unlocked here.
+ */
+
 int
 union_unlock(ap)
 	struct vop_lock_args *ap;
@@ -1464,6 +1585,12 @@ union_print(ap)
 		vprint("uppervp", UPPERVP(vp));
 	if (LOWERVP(vp))
 		vprint("lowervp", LOWERVP(vp));
+	if (VTOUNION(vp)->un_dircache) {
+		struct vnode **vpp;
+
+		for (vpp = VTOUNION(vp)->un_dircache; *vpp != NULLVP; vpp++)
+			vprint("dircache:", *vpp);
+	}
 	return (0);
 }