Cleaning up vgone.

[Jeff Roberson]

On Thu, 10 Mar 2005, Ian Dowse wrote:

> 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?

According to Kirk, XLOCK was introduced because the vnode lock was not
always in struct vnode, and so you couldn't rely on it after VOP_RECLAIM
was called.  This is no longer the case.  A hypothetical filesystem
which does not use the standard vnode lock would now have to lock the
vnode lock in RECLAIM, setup its ops vector to use the default vnode lock,
wait for all waiters to transfer to the vnode lock by releasing the
original lock, and then return from RECLAIM.  Shared locks are ok, as long
as you have an exclusive lock when you call vgone().

>
> 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?
>

In all cases callers should hold a reference while they're waiting on a
lock for that vnode to succeed.  After they acquire the lock, they should
see the VI_DOOMED flag, and eventually drop their reference allowing the
vnode to be recycled.

> >> 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.

I'd much rather get rid of DOINGINACT and simply vhold the vnode so it
doesn't have a 0 ref count.  It seems more natural to me to use a
reference than special case another flag.  I like the idea of the patch
below, but it conflicts with the patch I just posted as I removed the
DOINGINACT check from VSHOULDFREE.

>
> 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.
>  	 */
>