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

[Dave Chinner <david@fromorbit.com>]

On Thu, Sep 07, 2023 at 11:34:41AM -0700, Darrick J. Wong wrote:
> 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?

LGTM.

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