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

Oops. I did make sure the comment is there. :) Interestingly this
still would have worked fine enough as most of the problem is worked
around by 2/2. Anyhow, fixed.

thanks

On 3/26/22, Mitchell Horne <mhorne@freebsd.org> wrote:
>
>
> 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.
>


-- 
Mateusz Guzik <mjguzik gmail.com>