On Wed, Dec 06, 2023 at 09:58:58PM -0800, Christoph Hellwig wrote:
> On Wed, Dec 06, 2023 at 06:43:16PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@xxxxxxxxxx>
> >
> > Christoph asked for stronger protections against online repair zapping a
> > fork to get the inode to load vs. other threads trying to access the
> > partially repaired file. Do this by adding a special "[DA]FORK_ZAPPED"
> > inode health flag whenever repair zaps a fork, and sprinkling checks for
> > that flag into the various file operations for things that don't like
> > handling an unexpected zero-extents fork.
> >
> > In practice xfs_scrub will scrub and fix the forks almost immediately
> > after zapping them, so the window is very small.
>
> This probably should be before the previous two patches, and the
> reordering seems easy enough.
Done.
> We should also have a blurb in the commit log and code that this flag
> right now is in-memory only and thus the zapped forks can leak through
> an unmount or crash.
Fixed. The last paragraph now reads:
"In practice xfs_scrub will scrub and fix the forks almost immediately
after zapping them, so the window is very small. However, if a crash or
unmount should occur, we can still detect these zapped inode forks by
looking for a zero-extents fork when data was expected."
> Otherwise looks good:
>
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
Actually, I found a couple more bugs -- the checks for
XFS_SICK_INO_*FORK_ZAPPED in bmap.c that control setting the CORRUPT
flag should not do that if we're revalidating after a repair. I decided
they should also be hoisted up to the xchk_bmap caller to avoid
splitting the logic:
/* Scrub an inode's data fork. */
int
xchk_bmap_data(
struct xfs_scrub *sc)
{
int error;
/* Ignore old state if we're revalidating after a repair. */
if (!(sc->flags & XREP_ALREADY_FIXED) &&
xfs_inode_has_sickness(sc->ip, XFS_SICK_INO_DFORK_ZAPPED)) {
xchk_ino_set_corrupt(sc, sc->ip->i_ino);
return 0;
}
error = xchk_bmap(sc, XFS_DATA_FORK);
if (error)
return error;
/* If the data fork is clean, it is clearly not zapped. */
xchk_mark_healthy_if_clean(sc, XFS_SICK_INO_DFORK_ZAPPED);
return 0;
}
--D
