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.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
