Re: [PATCH 3/3] xfs: retain the AGI when we can't iget an inode to scrub the core

[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>
> 
> xchk_get_inode is not quite the right function to be calling from the
> inode scrubber setup function.  The common get_inode function either
> gets an inode and installs it in the scrub context, or it returns an
> error code explaining what happened.  This is acceptable for most file
> scrubbers because it is not in their scope to fix corruptions in the
> inode core and fork areas that cause iget to fail.
> 
> Dealing with these problems is within the scope of the inode scrubber,
> however.  If iget fails with EFSCORRUPTED, we need to xchk_inode to flag
> that as corruption.  Since we can't get our hands on an incore inode, we
> need to hold the AGI to prevent inode allocation activity so that
> nothing changes in the inode metadata.
> 
> Looking ahead to the inode core repair patches, we will also need to
> hold the AGI buffer into xrep_inode so that we can make modifications to
> the xfs_dinode structure without any other thread swooping in to
> allocate or free the inode.
> 
> Adapt the xchk_get_inode into xchk_setup_inode since this is a one-off
> use case where the error codes we check for are a little different, and
> the return state is much different from the common function.

The code look fine, but...

... doesn't this mean that xchk_setup_inode() and xchk_get_inode()
now are almost identical apart from the xchk_prepare_iscrub() bits?
This kinda looks like a lot of duplicated but subtly different code
- does xchk_get_inode() still need all that complexity if we are now
doing it in xchk_setup_inode()? If it does, why does
xchk_setup_inode() need to duplicate the code?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com