svn commit: r306512 - in head/sys: kern sys

[Bryan Drewery]

On 9/30/2016 10:27 AM, Mateusz Guzik wrote:
> Author: mjg
> Date: Fri Sep 30 17:27:17 2016
> New Revision: 306512
> URL: https://svnweb.freebsd.org/changeset/base/306512
> 
> Log:
>   vfs: batch free vnodes in per-mnt lists
>   
>   Previously free vnodes would always by directly returned to the global
>   LRU list. With this change up to mnt_free_list_batch vnodes are collected
>   first.
>   
>   syncer runs always return the batch regardless of its size.
>   
>   While vnodes on per-mnt lists are not counted as free, they can be
>   returned in case of vnode shortage.
>   
>   Reviewed by:	kib
>   Tested by:	pho
> 
> Modified:
>   head/sys/kern/vfs_mount.c
>   head/sys/kern/vfs_subr.c
>   head/sys/sys/mount.h
>   head/sys/sys/vnode.h
> 
...
> @@ -2753,17 +2824,25 @@ _vhold(struct vnode *vp, bool locked)
>  	 * Remove a vnode from the free list, mark it as in use,
>  	 * and put it on the active list.
>  	 */
> -	mtx_lock(&vnode_free_list_mtx);
> -	TAILQ_REMOVE(&vnode_free_list, vp, v_actfreelist);
> -	freevnodes--;
> -	vp->v_iflag &= ~VI_FREE;
> +	mp = vp->v_mount;
> +	mtx_lock(&mp->mnt_listmtx);
                  ^^

> +	if ((vp->v_mflag & VMP_TMPMNTFREELIST) != 0) {
> +		TAILQ_REMOVE(&mp->mnt_tmpfreevnodelist, vp, v_actfreelist);
> +		mp->mnt_tmpfreevnodelistsize--;
> +		vp->v_mflag &= ~VMP_TMPMNTFREELIST;
> +	} else {
> +		mtx_lock(&vnode_free_list_mtx);
> +		TAILQ_REMOVE(&vnode_free_list, vp, v_actfreelist);
> +		freevnodes--;
> +		mtx_unlock(&vnode_free_list_mtx);
> +	}
>  	KASSERT((vp->v_iflag & VI_ACTIVE) == 0,
>  	    ("Activating already active vnode"));
> +	vp->v_iflag &= ~VI_FREE;
>  	vp->v_iflag |= VI_ACTIVE;
> -	mp = vp->v_mount;
>  	TAILQ_INSERT_HEAD(&mp->mnt_activevnodelist, vp, v_actfreelist);
>  	mp->mnt_activevnodelistsize++;
> -	mtx_unlock(&vnode_free_list_mtx);
> +	mtx_unlock(&mp->mnt_listmtx);
>  	refcount_acquire(&vp->v_holdcnt);
>  	if (!locked)
>  		VI_UNLOCK(vp);
> @@ -2819,21 +2898,25 @@ _vdrop(struct vnode *vp, bool locked)
>  		if ((vp->v_iflag & VI_OWEINACT) == 0) {
>  			vp->v_iflag &= ~VI_ACTIVE;
>  			mp = vp->v_mount;
> -			mtx_lock(&vnode_free_list_mtx);
> +			mtx_lock(&mp->mnt_listmtx);
                                  ^^

If code runs getnewvnode() and then immediately runs vhold() or vrele(),
without first running insmntque(vp, mp), then vp->v_mount is NULL here
and the lock/freelist dereferencing just panic.
Locking the vnode and then using vgone() and vput() on it avoids the
issue since it marks the vnode VI_DOOMED and instead frees it in
_vdrop() rather than try to re-add it to its NULL per-mount free list.

I'm not sure what the right thing here is.  Is it a requirement to
insmntque() a new vnode immediately, or to vgone() it before releasing
it if was just returned from getnewvnode()?

Perhaps these cases should assert that v_mount is not NULL or handle the
odd case and just free the vnode instead, or can it still just add to
the global vnode_free_list if mp is NULL?

The old code handled the case fine since the freelist was global and not
per-mount.  'mp' was only dereferenced below if the vnode had been
active, which was not the case for my example.


>  			if (active) {
>  				TAILQ_REMOVE(&mp->mnt_activevnodelist, vp,
>  				    v_actfreelist);
>  				mp->mnt_activevnodelistsize--;
>  			}
> -			TAILQ_INSERT_TAIL(&vnode_free_list, vp,
> +			TAILQ_INSERT_TAIL(&mp->mnt_tmpfreevnodelist, vp,
>  			    v_actfreelist);
> -			freevnodes++;
> +			mp->mnt_tmpfreevnodelistsize++;
>  			vp->v_iflag |= VI_FREE;
> -			mtx_unlock(&vnode_free_list_mtx);
> +			vp->v_mflag |= VMP_TMPMNTFREELIST;
> +			VI_UNLOCK(vp);
> +			if (mp->mnt_tmpfreevnodelistsize >= mnt_free_list_batch)
> +				vnlru_return_batch_locked(mp);
> +			mtx_unlock(&mp->mnt_listmtx);
>  		} else {
> +			VI_UNLOCK(vp);
>  			atomic_add_long(&free_owe_inact, 1);
>  		}
> -		VI_UNLOCK(vp);
>  		return;
>  	}
>  	/*


-- 
Regards,
Bryan Drewery

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freebsd.org/pipermail/svn-src-head/attachments/20170824/497927b6/attachment.sig>