Re: git: c35ec1efdcb2 - main - vfs: [1/2] fix stalls in vnode reclaim by not requeieing from vnlru

On 3/10/22 05:42, Mateusz Guzik wrote:
> The branch main has been updated by mjg:
> 
> URL: https://cgit.FreeBSD.org/src/commit/?id=c35ec1efdcb2978bc3b6a0098c2b412be8d33e39
> 
> commit c35ec1efdcb2978bc3b6a0098c2b412be8d33e39
> Author:     Mateusz Guzik <mjg@FreeBSD.org>
> AuthorDate: 2022-03-07 10:33:59 +0000
> Commit:     Mateusz Guzik <mjg@FreeBSD.org>
> CommitDate: 2022-03-10 09:41:50 +0000
> 
>      vfs: [1/2] fix stalls in vnode reclaim by not requeieing from vnlru
>      
>      Reported by:    pho
> ---
>   sys/kern/vfs_subr.c | 55 +++++++++++++++++++++++++++++++++++++++++++----------
>   1 file changed, 45 insertions(+), 10 deletions(-)
> 
> diff --git a/sys/kern/vfs_subr.c b/sys/kern/vfs_subr.c
> index afafd02d92b9..436323873f7f 100644
> --- a/sys/kern/vfs_subr.c
> +++ b/sys/kern/vfs_subr.c
> @@ -113,6 +113,8 @@ static void	vn_seqc_init(struct vnode *);
>   static void	vn_seqc_write_end_free(struct vnode *vp);
>   static void	vgonel(struct vnode *);
>   static bool	vhold_recycle_free(struct vnode *);
> +static void	vdropl_recycle(struct vnode *vp);
> +static void	vdrop_recycle(struct vnode *vp);
>   static void	vfs_knllock(void *arg);
>   static void	vfs_knlunlock(void *arg);
>   static void	vfs_knl_assert_lock(void *arg, int what);
> @@ -1207,11 +1209,11 @@ restart:
>   		mtx_unlock(&vnode_list_mtx);
>   
>   		if (vn_start_write(vp, &mp, V_NOWAIT) != 0) {
> -			vdrop(vp);
> +			vdrop_recycle(vp);
>   			goto next_iter_unlocked;
>   		}
>   		if (VOP_LOCK(vp, LK_EXCLUSIVE|LK_NOWAIT) != 0) {
> -			vdrop(vp);
> +			vdrop_recycle(vp);
>   			vn_finished_write(mp);
>   			goto next_iter_unlocked;
>   		}
> @@ -1222,14 +1224,14 @@ restart:
>   		    (vp->v_object != NULL && vp->v_object->handle == vp &&
>   		    vp->v_object->resident_page_count > trigger)) {
>   			VOP_UNLOCK(vp);
> -			vdropl(vp);
> +			vdropl_recycle(vp);
>   			vn_finished_write(mp);
>   			goto next_iter_unlocked;
>   		}
>   		counter_u64_add(recycles_count, 1);
>   		vgonel(vp);
>   		VOP_UNLOCK(vp);
> -		vdropl(vp);
> +		vdropl_recycle(vp);
>   		vn_finished_write(mp);
>   		done++;
>   next_iter_unlocked:
> @@ -1613,7 +1615,7 @@ vtryrecycle(struct vnode *vp)
>   		CTR2(KTR_VFS,
>   		    "%s: impossible to recycle, vp %p lock is already held",
>   		    __func__, vp);
> -		vdrop(vp);
> +		vdrop_recycle(vp);
>   		return (EWOULDBLOCK);
>   	}
>   	/*
> @@ -1624,7 +1626,7 @@ vtryrecycle(struct vnode *vp)
>   		CTR2(KTR_VFS,
>   		    "%s: impossible to recycle, cannot start the write for %p",
>   		    __func__, vp);
> -		vdrop(vp);
> +		vdrop_recycle(vp);
>   		return (EBUSY);
>   	}
>   	/*
> @@ -1636,7 +1638,7 @@ vtryrecycle(struct vnode *vp)
>   	VI_LOCK(vp);
>   	if (vp->v_usecount) {
>   		VOP_UNLOCK(vp);
> -		vdropl(vp);
> +		vdropl_recycle(vp);
>   		vn_finished_write(vnmp);
>   		CTR2(KTR_VFS,
>   		    "%s: impossible to recycle, %p is already referenced",
> @@ -1648,7 +1650,7 @@ vtryrecycle(struct vnode *vp)
>   		vgonel(vp);
>   	}
>   	VOP_UNLOCK(vp);
> -	vdropl(vp);
> +	vdropl_recycle(vp);
>   	vn_finished_write(vnmp);
>   	return (0);
>   }
> @@ -3598,8 +3600,8 @@ vdrop(struct vnode *vp)
>   	vdropl(vp);
>   }
>   
> -void
> -vdropl(struct vnode *vp)
> +static void __always_inline
> +vdropl_impl(struct vnode *vp, bool enqueue)
>   {

Hi,

It seems like enqueue is completely unused within the function. Was 
there a hunk missing from the final commit?

Cheers,
Mitchell

>   
>   	ASSERT_VI_LOCKED(vp, __func__);
> @@ -3627,6 +3629,39 @@ vdropl(struct vnode *vp)
>   	vdbatch_enqueue(vp);
>   }
>   
> +void
> +vdropl(struct vnode *vp)
> +{
> +
> +	vdropl_impl(vp, true);
> +}
> +
> +/*
> + * vdrop a vnode when recycling
> + *
> + * This is a special case routine only to be used when recycling, differs from
> + * regular vdrop by not requeieing the vnode on LRU.
> + *
> + * Consider a case where vtryrecycle continuously fails with all vnodes (due to
> + * e.g., frozen writes on the filesystem), filling the batch and causing it to
> + * be requeued. Then vnlru will end up revisiting the same vnodes. This is a
> + * loop which can last for as long as writes are frozen.
> + */
> +static void
> +vdropl_recycle(struct vnode *vp)
> +{
> +
> +	vdropl_impl(vp, false);
> +}
> +
> +static void
> +vdrop_recycle(struct vnode *vp)
> +{
> +
> +	VI_LOCK(vp);
> +	vdropl_recycle(vp);
> +}
> +
>   /*
>    * Call VOP_INACTIVE on the vnode and manage the DOINGINACT and OWEINACT
>    * flags.  DOINGINACT prevents us from recursing in calls to vinactive.