Re: [PATCH 03/11] xfs: don't reclaim dquots with incore reservations

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Mar 23, 2021 at 10:31:39AM +1100, Dave Chinner wrote:
> On Wed, Mar 10, 2021 at 07:05:57PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > If a dquot has an incore reservation that exceeds the ondisk count, it
> > by definition has active incore state and must not be reclaimed.  Up to
> > this point every inode with an incore dquot reservation has always
> > retained a reference to the dquot so it was never possible for
> > xfs_qm_dquot_isolate to be called on a dquot with active state and zero
> > refcount, but this will soon change.
> > 
> > Deferred inode inactivation is about to reorganize how inodes are
> > inactivated by shunting all that work to a background workqueue.  In
> > order to avoid deadlocks with the quotaoff inode scan and reduce overall
> > memory requirements (since inodes can spend a lot of time waiting for
> > inactivation), inactive inodes will drop their dquot references while
> > they're waiting to be inactivated.
> > 
> > However, inactive inodes can have delalloc extents in the data fork or
> > any extents in the CoW fork.  Either of these contribute to the dquot's
> > incore reservation being larger than the resource count (i.e. they're
> > the reason the dquot still has active incore state), so we cannot allow
> > the dquot to be reclaimed.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> .....
> >  static enum lru_status
> >  xfs_qm_dquot_isolate(
> >  	struct list_head	*item,
> > @@ -427,10 +441,15 @@ xfs_qm_dquot_isolate(
> >  		goto out_miss_busy;
> >  
> >  	/*
> > -	 * This dquot has acquired a reference in the meantime remove it from
> > -	 * the freelist and try again.
> > +	 * Either this dquot has incore reservations or it has acquired a
> > +	 * reference.  Remove it from the freelist and try again.
> > +	 *
> > +	 * Inodes tagged for inactivation drop their dquot references to avoid
> > +	 * deadlocks with quotaoff.  If these inodes have delalloc reservations
> > +	 * in the data fork or any extents in the CoW fork, these contribute
> > +	 * to the dquot's incore block reservation exceeding the count.
> >  	 */
> > -	if (dqp->q_nrefs) {
> > +	if (xfs_dquot_has_incore_resv(dqp) || dqp->q_nrefs) {
> >  		xfs_dqunlock(dqp);
> >  		XFS_STATS_INC(dqp->q_mount, xs_qm_dqwants);
> >  
> 
> This means we can have dquots with no references that aren't on
> the free list and aren't actually referenced by any inode, either.
> 
> So if we now shut down the filesystem, what frees these dquots?
> Are we relying on xfs_qm_dqpurge_all() to find all these dquots
> and xfs_qm_dqpurge() guaranteeing that they are always cleaned
> and freed?

Yes.  Want me to add that to the comment?

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com