Skip site navigation (1)Skip section navigation (2) 
Date:      Tue, 29 Jul 2014 16:42:34 +0000 (UTC)
From:      Konstantin Belousov <kib@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r269244 - head/sys/kern
Message-ID:  <201407291642.s6TGgY8t089547@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help 
Author: kib
Date: Tue Jul 29 16:42:34 2014
New Revision: 269244
URL: http://svnweb.freebsd.org/changeset/base/269244

Log:
  Remove one-time use macros which check for the vnode lifecycle.  More,
  some parts of the checks are in fact redundand in the surrounding
  code, and it is more clear what the conditions are by direct testing
  of the flags.  Two of the three macros were only used in assertions.
  
  In vnlru_free(), all relevant parts of vholdl() were already inlined,
  except the increment of v_holdcnt itself.  Do not call vholdl() to do
  the increment as well, this allows to make assertions in
  vholdl()/vhold() more strict.
  
  In v_incr_usecount(), call vholdl() before incrementing other ref
  counters.  The change is no-op, but it makes less surprising to see
  the vnode state in debugger if interrupted inside v_incr_usecount().
  
  Tested by:	pho
  Sponsored by:	The FreeBSD Foundation
  MFC after:	1 week

Modified:
  head/sys/kern/vfs_subr.c

Modified: head/sys/kern/vfs_subr.c
==============================================================================
--- head/sys/kern/vfs_subr.c	Tue Jul 29 15:01:29 2014	(r269243)
+++ head/sys/kern/vfs_subr.c	Tue Jul 29 16:42:34 2014	(r269244)
@@ -275,14 +275,6 @@ static int vnlru_nowhere;
 SYSCTL_INT(_debug, OID_AUTO, vnlru_nowhere, CTLFLAG_RW,
     &vnlru_nowhere, 0, "Number of times the vnlru process ran without success");
 
-/*
- * Macros to control when a vnode is freed and recycled.  All require
- * the vnode interlock.
- */
-#define VCANRECYCLE(vp) (((vp)->v_iflag & VI_FREE) && !(vp)->v_holdcnt)
-#define VSHOULDFREE(vp) (!((vp)->v_iflag & VI_FREE) && !(vp)->v_holdcnt)
-#define VSHOULDBUSY(vp) (((vp)->v_iflag & VI_FREE) && (vp)->v_holdcnt)
-
 /* Shift count for (uintptr_t)vp to initialize vp->v_hash. */
 static int vnsz2log;
 
@@ -849,11 +841,21 @@ vnlru_free(int count)
 			TAILQ_INSERT_TAIL(&vnode_free_list, vp, v_actfreelist);
 			continue;
 		}
-		VNASSERT(VCANRECYCLE(vp), vp,
-		    ("vp inconsistent on freelist"));
+		VNASSERT((vp->v_iflag & VI_FREE) != 0 && vp->v_holdcnt == 0,
+		    vp, ("vp inconsistent on freelist"));
+
+		/*
+		 * The clear of VI_FREE prevents activation of the
+		 * vnode.  There is no sense in putting the vnode on
+		 * the mount point active list, only to remove it
+		 * later during recycling.  Inline the relevant part
+		 * of vholdl(), to avoid triggering assertions or
+		 * activating.
+		 */
 		freevnodes--;
 		vp->v_iflag &= ~VI_FREE;
-		vholdl(vp);
+		vp->v_holdcnt++;
+
 		mtx_unlock(&vnode_free_list_mtx);
 		VI_UNLOCK(vp);
 		vtryrecycle(vp);
@@ -2042,13 +2044,13 @@ v_incr_usecount(struct vnode *vp)
 {
 
 	CTR2(KTR_VFS, "%s: vp %p", __func__, vp);
+	vholdl(vp);
 	vp->v_usecount++;
 	if (vp->v_type == VCHR && vp->v_rdev != NULL) {
 		dev_lock();
 		vp->v_rdev->si_usecount++;
 		dev_unlock();
 	}
-	vholdl(vp);
 }
 
 /*
@@ -2325,11 +2327,18 @@ vholdl(struct vnode *vp)
 	struct mount *mp;
 
 	CTR2(KTR_VFS, "%s: vp %p", __func__, vp);
+#ifdef INVARIANTS
+	/* getnewvnode() calls v_incr_usecount() without holding interlock. */
+	if (vp->v_type != VNON || vp->v_data != NULL) {
+		ASSERT_VI_LOCKED(vp, "vholdl");
+		VNASSERT(vp->v_holdcnt > 0 || (vp->v_iflag & VI_FREE) != 0,
+		    vp, ("vholdl: free vnode is held"));
+	}
+#endif
 	vp->v_holdcnt++;
-	if (!VSHOULDBUSY(vp))
+	if ((vp->v_iflag & VI_FREE) == 0)
 		return;
-	ASSERT_VI_LOCKED(vp, "vholdl");
-	VNASSERT((vp->v_iflag & VI_FREE) != 0, vp, ("vnode not free"));
+	VNASSERT(vp->v_holdcnt == 1, vp, ("vholdl: wrong hold count"));
 	VNASSERT(vp->v_op != NULL, vp, ("vholdl: vnode already reclaimed."));
 	/*
 	 * Remove a vnode from the free list, mark it as in use,
@@ -2392,7 +2401,7 @@ vdropl(struct vnode *vp)
 		    ("vdropl: vnode already reclaimed."));
 		VNASSERT((vp->v_iflag & VI_FREE) == 0, vp,
 		    ("vnode already free"));
-		VNASSERT(VSHOULDFREE(vp), vp,
+		VNASSERT(vp->v_holdcnt == 0, vp,
 		    ("vdropl: freeing when we shouldn't"));
 		active = vp->v_iflag & VI_ACTIVE;
 		vp->v_iflag &= ~VI_ACTIVE;

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