unionfs: simplify writecount management

Use atomics to track the writecount granted to the underlying FS,
and avoid holding the vnode interlock while calling the underling FS'
VOP_ADD_WRITECOUNT().  This also fixes a WITNESS warning about nesting
the same lock type.  Also add comments explaining why we need to track
the writecount on the unionfs vnode in the first place.  Finally,
simplify writecount management to only use the upper vnode and assert
that we shouldn't have an active writecount on the lower vnode through
unionfs.

Reviewed by:	kib
Differential Revision:	https://reviews.freebsd.org/D33611

2 files changed, 38 insertions, 23 deletions

diff --git a/sys/fs/unionfs/union_subr.c b/sys/fs/unionfs/union_subr.c
index 551553fd8b20..9a706d79f9f7 100644
--- a/sys/fs/unionfs/union_subr.c
+++ b/sys/fs/unionfs/union_subr.c
@@ -57,6 +57,8 @@
 #include <sys/taskqueue.h>
 #include <sys/resourcevar.h>
 
+#include <machine/atomic.h>
+
 #include <security/mac/mac_framework.h>
 
 #include <vm/uma.h>
@@ -435,6 +437,7 @@ unionfs_noderem(struct vnode *vp)
 	struct vnode   *uvp;
 	struct vnode   *dvp;
 	int		count;
+	int		writerefs;
 
 	KASSERT(vp->v_vnlock->lk_recurse == 0,
 	    ("%s: vnode %p locked recursively", __func__, vp));
@@ -454,13 +457,6 @@ unionfs_noderem(struct vnode *vp)
 	vp->v_vnlock = &(vp->v_lock);
 	vp->v_data = NULL;
 	vp->v_object = NULL;
-	if (vp->v_writecount > 0) {
-		if (uvp != NULL)
-			VOP_ADD_WRITECOUNT(uvp, -vp->v_writecount);
-		else if (lvp != NULL)
-			VOP_ADD_WRITECOUNT(lvp, -vp->v_writecount);
-	} else if (vp->v_writecount < 0)
-		vp->v_writecount = 0;
 	if (unp->un_hashtbl != NULL) {
 		/*
 		 * Clear out any cached child vnodes.  This should only
@@ -479,6 +475,19 @@ unionfs_noderem(struct vnode *vp)
 	}
 	VI_UNLOCK(vp);
 
+	writerefs = atomic_load_int(&vp->v_writecount);
+	VNASSERT(writerefs >= 0, vp,
+	    ("%s: write count %d, unexpected text ref", __func__, writerefs));
+	/*
+	 * If we were opened for write, we leased the write reference
+	 * to the lower vnode.  If this is a reclamation due to the
+	 * forced unmount, undo the reference now.
+	 */
+	if (writerefs > 0) {
+		VNASSERT(uvp != NULL, vp,
+		    ("%s: write reference without upper vnode", __func__));
+		VOP_ADD_WRITECOUNT(uvp, -writerefs);
+	}
 	if (lvp != NULLVP)
 		VOP_UNLOCK(lvp);
 	if (uvp != NULLVP)
@@ -805,6 +814,8 @@ unionfs_node_update(struct unionfs_node *unp, struct vnode *uvp,
 	ASSERT_VOP_ELOCKED(uvp, __func__);
 	dvp = unp->un_dvp;
 
+	VNASSERT(vp->v_writecount == 0, vp,
+	    ("%s: non-zero writecount", __func__));
 	/*
 	 * Uppdate the upper vnode's lock state to match the lower vnode,
 	 * and then switch the unionfs vnode's lock to the upper vnode.
diff --git a/sys/fs/unionfs/union_vnops.c b/sys/fs/unionfs/union_vnops.c
index 6cedfa3d142d..dc02e20f376f 100644
--- a/sys/fs/unionfs/union_vnops.c
+++ b/sys/fs/unionfs/union_vnops.c
@@ -61,6 +61,8 @@
 
 #include <fs/unionfs/union.h>
 
+#include <machine/atomic.h>
+
 #include <vm/vm.h>
 #include <vm/vm_extern.h>
 #include <vm/vm_object.h>
@@ -2523,26 +2525,28 @@ unionfs_add_writecount(struct vop_add_writecount_args *ap)
 {
 	struct vnode *tvp, *vp;
 	struct unionfs_node *unp;
-	int error;
+	int error, writerefs;
 
 	vp = ap->a_vp;
 	unp = VTOUNIONFS(vp);
-	tvp = unp->un_uppervp != NULL ? unp->un_uppervp : unp->un_lowervp;
-	VI_LOCK(vp);
+	tvp = unp->un_uppervp;
+	KASSERT(tvp != NULL,
+	    ("%s: adding write ref without upper vnode", __func__));
+	error = VOP_ADD_WRITECOUNT(tvp, ap->a_inc);
+	if (error != 0)
+		return (error);
+	/*
+	 * We need to track the write refs we've passed to the underlying
+	 * vnodes so that we can undo them in case we are forcibly unmounted.
+	 */
+	writerefs = atomic_fetchadd_int(&vp->v_writecount, ap->a_inc);
 	/* text refs are bypassed to lowervp */
-	VNASSERT(vp->v_writecount >= 0, vp, ("wrong null writecount"));
-	VNASSERT(vp->v_writecount + ap->a_inc >= 0, vp,
-	    ("wrong writecount inc %d", ap->a_inc));
-	if (tvp != NULL)
-		error = VOP_ADD_WRITECOUNT(tvp, ap->a_inc);
-	else if (vp->v_writecount < 0)
-		error = ETXTBSY;
-	else
-		error = 0;
-	if (error == 0)
-		vp->v_writecount += ap->a_inc;
-	VI_UNLOCK(vp);
-	return (error);
+	VNASSERT(writerefs >= 0, vp,
+	    ("%s: invalid write count %d", __func__, writerefs));
+	VNASSERT(writerefs + ap->a_inc >= 0, vp,
+	    ("%s: invalid write count inc %d + %d", __func__,
+	    writerefs, ap->a_inc));
+	return (0);
 }
 
 static int