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

[Dave Chinner <david@fromorbit.com>]

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.

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com