svn commit: r355536 - head/sys/kern

[Mateusz Guzik]

Author: mjg
Date: Sun Dec  8 21:13:07 2019
New Revision: 355536
URL: https://svnweb.freebsd.org/changeset/base/355536

Log:
  vfs: clean up vputx a little
  
  1. replace hand-rolled macros for operation type with enum
  2. unlock the vnode in vput itself, there is no need to branch on it. existence
  of VPUTX_VPUT remains significant in that the inactive variant adds LK_NOWAIT
  to locking request.
  3. remove the useless v_usecount assertion. few lines above the checks if
  v_usecount > 0 and leaves. should the value be negative, refcount would fail.
  4. the CTR return vnode %p to the freelist is incorrect as vdrop may find the
  vnode with holdcnt > 1. if the like should exist, it should be moved there
  5. no need to error = 0 for everyone
  
  Reviewed by:	kib, jeff (previous version)
  Differential Revision:	https://reviews.freebsd.org/D22718

Modified:
  head/sys/kern/vfs_subr.c

Modified: head/sys/kern/vfs_subr.c
==============================================================================
--- head/sys/kern/vfs_subr.c	Sun Dec  8 21:12:33 2019	(r355535)
+++ head/sys/kern/vfs_subr.c	Sun Dec  8 21:13:07 2019	(r355536)
@@ -2968,9 +2968,7 @@ vrefcnt(struct vnode *vp)
 	return (vp->v_usecount);
 }
 
-#define	VPUTX_VRELE	1
-#define	VPUTX_VPUT	2
-#define	VPUTX_VUNREF	3
+enum vputx_op { VPUTX_VRELE, VPUTX_VPUT, VPUTX_VUNREF };
 
 /*
  * Decrement the use and hold counts for a vnode.
@@ -2978,17 +2976,13 @@ vrefcnt(struct vnode *vp)
  * See an explanation near vget() as to why atomic operation is safe.
  */
 static void
-vputx(struct vnode *vp, int func)
+vputx(struct vnode *vp, enum vputx_op func)
 {
 	int error;
 
 	KASSERT(vp != NULL, ("vputx: null vp"));
 	if (func == VPUTX_VUNREF)
 		ASSERT_VOP_LOCKED(vp, "vunref");
-	else if (func == VPUTX_VPUT)
-		ASSERT_VOP_LOCKED(vp, "vput");
-	else
-		KASSERT(func == VPUTX_VRELE, ("vputx: wrong func"));
 	ASSERT_VI_UNLOCKED(vp, __func__);
 	VNASSERT(vp->v_holdcnt > 0 && vp->v_usecount > 0, vp,
 	    ("%s: wrong ref counts", __func__));
@@ -2996,19 +2990,6 @@ vputx(struct vnode *vp, int func)
 	CTR2(KTR_VFS, "%s: vp %p", __func__, vp);
 
 	/*
-	 * It is an invariant that all VOP_* calls operate on a held vnode.
-	 * We may be only having an implicit hold stemming from our usecount,
-	 * which we are about to release. If we unlock the vnode afterwards we
-	 * open a time window where someone else dropped the last usecount and
-	 * proceeded to free the vnode before our unlock finished. For this
-	 * reason we unlock the vnode early. This is a little bit wasteful as
-	 * it may be the vnode is exclusively locked and inactive processing is
-	 * needed, in which case we are adding work.
-	 */
-	if (func == VPUTX_VPUT)
-		VOP_UNLOCK(vp, 0);
-
-	/*
 	 * We want to hold the vnode until the inactive finishes to
 	 * prevent vgone() races.  We drop the use count here and the
 	 * hold count below when we're done.
@@ -3034,15 +3015,6 @@ vputx(struct vnode *vp, int func)
 		return;
 	}
 
-	error = 0;
-
-	if (vp->v_usecount != 0) {
-		vn_printf(vp, "vputx: usecount not zero for vnode ");
-		panic("vputx: usecount not zero");
-	}
-
-	CTR2(KTR_VFS, "%s: return vnode %p to the freelist", __func__, vp);
-
 	/*
 	 * Check if the fs wants to perform inactive processing. Note we
 	 * may be only holding the interlock, in which case it is possible
@@ -3071,6 +3043,7 @@ vputx(struct vnode *vp, int func)
 		VI_LOCK(vp);
 		break;
 	case VPUTX_VUNREF:
+		error = 0;
 		if (VOP_ISLOCKED(vp) != LK_EXCLUSIVE) {
 			error = VOP_LOCK(vp, LK_TRYUPGRADE | LK_INTERLOCK);
 			VI_LOCK(vp);
@@ -3103,11 +3076,21 @@ vrele(struct vnode *vp)
  * Release an already locked vnode.  This give the same effects as
  * unlock+vrele(), but takes less time and avoids releasing and
  * re-aquiring the lock (as vrele() acquires the lock internally.)
+ *
+ * It is an invariant that all VOP_* calls operate on a held vnode.
+ * We may be only having an implicit hold stemming from our usecount,
+ * which we are about to release. If we unlock the vnode afterwards we
+ * open a time window where someone else dropped the last usecount and
+ * proceeded to free the vnode before our unlock finished. For this
+ * reason we unlock the vnode early. This is a little bit wasteful as
+ * it may be the vnode is exclusively locked and inactive processing is
+ * needed, in which case we are adding work.
  */
 void
 vput(struct vnode *vp)
 {
 
+	VOP_UNLOCK(vp, 0);
 	vputx(vp, VPUTX_VPUT);
 }