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

[Dave Chinner <david@fromorbit.com>]

On Mon, Mar 22, 2021 at 05:01:11PM -0700, Darrick J. Wong wrote:
> 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?

Yes Please!

-Dave.
-- 
Dave Chinner
david@fromorbit.com