Cleaning up vgone.

[Ian Dowse]

In message <20050310040417.A20708 at mail.chesapeake.net>, Jeff Roberson writes:
>On Thu, 10 Mar 2005, Jeff Roberson wrote:
>> Prior to this, vgone() would set XLOCK and then do a LK_DRAIN to make
>> sure there were no callers waiting in VOP_LOCK so that they would always
>> see the VI_XLOCK and know that the vnode had changed identities.  Now,
>> vgone sets XLOCK, and all lockers who use vget() and vn_lock() check for
>
>This should be "vgone sets VI_DOOMED" which now means "the vnode has been
>dissociated from it's filesystem".

I think one of the other original reasons for the XLOCK/LK_DRAIN
code was filesystems that used shared locks such as NFS, since
locking the vnode did not provide exclusive access. But there are
no more shared locking filesystems now and we are not going to
support this again, right?

Is there a new potential race where the vnode could be reused and
so have the VI_DOOMED flag cleared before a thread waiting for the
original vnode wakes up and sees the VI_DOOMED flag?

>> The patch is available at http://www.chesapeake.net/~jroberson/vgone.diff

BTW, I believe the vgonel() code can be further simplified since
the addition of the VI_DOINGINACT flag some time ago. Below is a
patch extracted from some local changes that I've been using locally
for quite a long time without problems. The new vbusy() call addresses
one case where it appeared a vnode could be picked up for reuse
while it was being recycled, but that may be impossible now. The
main part of the patch works by letting the VI_DOINGINACT flag take
the place of the non-zero usecount.

Ian

Index: vfs_subr.c
===================================================================
RCS file: /dump/FreeBSD-CVS/src/sys/kern/vfs_subr.c,v
retrieving revision 1.581
diff -u -r1.581 vfs_subr.c
--- vfs_subr.c	17 Feb 2005 10:49:50 -0000	1.581
+++ vfs_subr.c	18 Jan 2005 03:54:52 -0000
@@ -2279,7 +2308,6 @@
 void
 vgonel(struct vnode *vp, struct thread *td)
 {
-	int active;
 
 	/*
 	 * If a vgone (or vclean) is already in progress,
@@ -2291,18 +2319,11 @@
 
 	vx_lock(vp);
 
+	/* The vnode must not be on the free list while being cleaned. */
+	if (vp->v_iflag & VI_FREE)
+		vbusy(vp);
 	/*
-	 * Check to see if the vnode is in use. If so we have to reference it
-	 * before we clean it out so that its count cannot fall to zero and
-	 * generate a race against ourselves to recycle it.
-	 */
-	if ((active = vp->v_usecount))
-		v_incr_usecount(vp, 1);
-
-	/*
-	 * Even if the count is zero, the VOP_INACTIVE routine may still
-	 * have the object locked while it cleans it out. The VOP_LOCK
-	 * ensures that the VOP_INACTIVE routine is done with its work.
+	 * Lock the vnode, draining so that we wait on shared locks too.
 	 * For active vnodes, it ensures that no other activity can
 	 * occur while the underlying object is being cleaned out.
 	 */
@@ -2328,7 +2349,7 @@
 	 * deactivated before being reclaimed. Note that the
 	 * VOP_INACTIVE will unlock the vnode.
 	 */
-	if (active) {
+	if (vp->v_usecount) {
 		VOP_CLOSE(vp, FNONBLOCK, NOCRED, td);
 		VI_LOCK(vp);
 		if ((vp->v_iflag & VI_DOINGINACT) == 0) {
@@ -2352,28 +2373,6 @@
 	if (VOP_RECLAIM(vp, td))
 		panic("vclean: cannot reclaim");
 
-	VNASSERT(vp->v_object == NULL, vp,
-	    ("vop_reclaim left v_object vp=%p, tag=%s", vp, vp->v_tag));
-
-	if (active) {
-		/*
-		 * Inline copy of vrele() since VOP_INACTIVE
-		 * has already been called.
-		 */
-		VI_LOCK(vp);
-		v_incr_usecount(vp, -1);
-		if (vp->v_usecount <= 0) {
-#ifdef INVARIANTS
-			if (vp->v_usecount < 0 || vp->v_writecount != 0) {
-				vprint("vclean: bad ref count", vp);
-				panic("vclean: ref cnt");
-			}
-#endif
-			if (VSHOULDFREE(vp))
-				vfree(vp);
-		}
-		VI_UNLOCK(vp);
-	}
 	/*
 	 * Delete from old mount point vnode list.
 	 */