Re: [PATCH v1.1 3/3] xfs: make inode unlinked bucket recovery work with quotacheck

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

On Thu, Sep 07, 2023 at 05:11:53PM +1000, Dave Chinner wrote:
> On Tue, Sep 05, 2023 at 09:33:03AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Teach quotacheck to reload the unlinked inode lists when walking the
> > inode table.  This requires extra state handling, since it's possible
> > that a reloaded inode will get inactivated before quotacheck tries to
> > scan it; in this case, we need to ensure that the reloaded inode does
> > not have dquots attached when it is freed.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> > v1.1: s/CONFIG_QUOTA/CONFIG_XFS_QUOTA/ and fix tracepoint flags decoding
> > ---
> >  fs/xfs/xfs_inode.c |   12 +++++++++---
> >  fs/xfs/xfs_inode.h |    5 ++++-
> >  fs/xfs/xfs_mount.h |   10 +++++++++-
> >  fs/xfs/xfs_qm.c    |    7 +++++++
> >  4 files changed, 29 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index 56f6bde6001b..22af7268169b 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -1743,9 +1743,13 @@ xfs_inactive(
> >  	     ip->i_df.if_nextents > 0 || ip->i_delayed_blks > 0))
> >  		truncate = 1;
> >  
> > -	error = xfs_qm_dqattach(ip);
> > -	if (error)
> > -		goto out;
> > +	if (xfs_iflags_test(ip, XFS_IQUOTAUNCHECKED)) {
> > +		xfs_qm_dqdetach(ip);
> > +	} else {
> > +		error = xfs_qm_dqattach(ip);
> > +		if (error)
> > +			goto out;
> > +	}
> 
> That needs a comment - I'm not going to remember why sometimes we
> detatch dquots instead of attach them here....

	/*
	 * If this inode is being inactivated during a quotacheck and
	 * has not yet been scanned by quotacheck, we /must/ remove the
	 * dquots from the inode before inactivation changes the block
	 * and inode counts.  Most probably this is a result of
	 * reloading the incore iunlinked list to purge unrecovered
	 * unlinked inodes.
	 */

How does that sound?

> ....
> > diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> > index 6abcc34fafd8..7256090c3895 100644
> > --- a/fs/xfs/xfs_qm.c
> > +++ b/fs/xfs/xfs_qm.c
> > @@ -1160,6 +1160,10 @@ xfs_qm_dqusage_adjust(
> >  	if (error)
> >  		return error;
> >  
> > +	error = xfs_inode_reload_unlinked(ip);
> > +	if (error)
> > +		goto error0;
> 
> Same comment here about doing millions of transaction create/cancel
> for inodes that have non-zero link counts....
> 
> Also, same comment here about shutting down on reload error because
> the irele() call will inactivate the inode and try to remove it from
> the unlinked list....

Ok, fixed this callsite too.

Thank you for looking at this series!

--D

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