Re: [PATCH 1/3] xfs: manage inode DONTCACHE status at irele time

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

On Tue, Nov 15, 2022 at 02:13:18PM +1100, Dave Chinner wrote:
> On Sun, Oct 02, 2022 at 11:20:29AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Right now, there are statements scattered all over the online fsck
> > codebase about how we can't use XFS_IGET_DONTCACHE because of concerns
> > about scrub's unusual practice of releasing inodes with transactions
> > held.
> > 
> > However, iget is the wrong place to handle this -- the DONTCACHE state
> > doesn't matter at all until we try to *release* the inode, and here we
> > get things wrong in multiple ways:
> > 
> > First, if we /do/ have a transaction, we must NOT drop the inode,
> > because the inode could have dirty pages, dropping the inode will
> > trigger writeback, and writeback can trigger a nested transaction.
> > 
> > Second, if the inode already had an active reference and the DONTCACHE
> > flag set, the icache hit when scrub grabs another ref will not clear
> > DONTCACHE.  This is sort of by design, since DONTCACHE is now used to
> > initiate cache drops so that sysadmins can change a file's access mode
> > between pagecache and DAX.
> > 
> > Third, if we do actually have the last active reference to the inode, we
> > can set DONTCACHE to avoid polluting the cache.  This is the /one/ case
> > where we actually want that flag.
> > 
> > Create an xchk_irele helper to encode all that logic and switch the
> > online fsck code to use it.  Since this now means that nearly all
> > scrubbers use the same xfs_iget flags, we can wrap them too.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> 
> Ok, I can see what needs to be done here. It seems a bit fragile,
> but I don't see a better way at the moment.
> 
> That said...
> 
> > diff --git a/fs/xfs/scrub/parent.c b/fs/xfs/scrub/parent.c
> > index ab182a5cd0c0..38ea04e66468 100644
> > --- a/fs/xfs/scrub/parent.c
> > +++ b/fs/xfs/scrub/parent.c
> > @@ -131,7 +131,6 @@ xchk_parent_validate(
> >  	xfs_ino_t		dnum,
> >  	bool			*try_again)
> >  {
> > -	struct xfs_mount	*mp = sc->mp;
> >  	struct xfs_inode	*dp = NULL;
> >  	xfs_nlink_t		expected_nlink;
> >  	xfs_nlink_t		nlink;
> > @@ -168,7 +167,7 @@ xchk_parent_validate(
> >  	 * -EFSCORRUPTED or -EFSBADCRC then the parent is corrupt which is a
> >  	 *  cross referencing error.  Any other error is an operational error.
> >  	 */
> > -	error = xfs_iget(mp, sc->tp, dnum, XFS_IGET_UNTRUSTED, 0, &dp);
> > +	error = xchk_iget(sc, dnum, &dp);
> >  	if (error == -EINVAL || error == -ENOENT) {
> >  		error = -EFSCORRUPTED;
> >  		xchk_fblock_process_error(sc, XFS_DATA_FORK, 0, &error);
> > @@ -253,7 +252,7 @@ xchk_parent_validate(
> >  out_unlock:
> >  	xfs_iunlock(dp, XFS_IOLOCK_SHARED);
> >  out_rele:
> > -	xfs_irele(dp);
> > +	xchk_irele(sc, dp);
> >  out:
> >  	return error;
> >  }
> 
> Didn't you miss a couple of cases here? THe current upstream code
> looks like:
> 
> .......
> 237         /* Drat, parent changed.  Try again! */
> 238         if (dnum != dp->i_ino) {
> 239                 xfs_irele(dp);
> 240                 *try_again = true;
> 241                 return 0;
> 242         }
> 243         xfs_irele(dp);
> 244
> 245         /*
> 246          * '..' didn't change, so check that there was only one entry
> 247          * for us in the parent.
> 248          */
> 249         if (nlink != expected_nlink)
> 250                 xchk_fblock_set_corrupt(sc, XFS_DATA_FORK, 0);
> 251         return error;
> 252
> 253 out_unlock:
> 254         xfs_iunlock(dp, XFS_IOLOCK_SHARED);
> 255 out_rele:
> 256         xfs_irele(dp);
> 257 out:
> 258         return error;
> 259 }
> 
> So it looks like you missed the conversion at lines 239 and 243. Of
> course, these may have been removed in a prior patchset I've looked
> at and forgotten about, but on the surface this looks like missed
> conversions.

Actually, I probably missed it because one of the follow-on fixpatches
in the v23.1 patchbomb removes it entirely:
https://lore.kernel.org/linux-xfs/166473483278.1084804.14032671424392139245.stgit@magnolia/

--D

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