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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx
