On Thu, Nov 17, 2022 at 12:15:48PM +1100, Dave Chinner wrote:
> On Tue, Nov 15, 2022 at 06:49:14PM -0800, Darrick J. Wong wrote:
> > On Tue, Nov 15, 2022 at 03:08:16PM +1100, Dave Chinner wrote:
> > > On Sun, Oct 02, 2022 at 11:20:29AM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <djwong@xxxxxxxxxx>
> > > >
> > > > 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?
> >
> > Yes, they're /nearly/ identical in the helper functions they call, but
> > they're not so similar in intent and how they handle @error values:
> >
> > xchk_setup_inode prepares to check or repair an inode record, so it must
> > continue the scrub operation even if the inode/inobt verifiers cause
> > xfs_iget to return EFSCORRUPTED. This is done by attaching the locked
> > AGI buffer to the scrub transaction and returning 0 to move on to the
> > actual scrub. (Later, the online inode repair code will also want the
> > xfs_imap structure so that it can reset the ondisk xfs_dinode
> > structure.)
> >
> > xchk_get_inode retrieves an inode on behalf of a scrubber that operates
> > on an incore inode -- data/attr/cow forks, directories, xattrs,
> > symlinks, parent pointers, etc. If the inode/inobt verifiers fail and
> > xfs_iget returns EFSCORRUPTED, we want to exit to userspace (because the
> > caller should be fix the inode first) and drop everything we acquired
> > along the way.
> >
> > A behavior common to both functions is that it's possible that xfs_scrub
> > asked for a scrub-by-handle concurrent with the inode being freed or the
> > passed-in inumber is invalid. In this case, we call xfs_imap to see if
> > the inobt index thinks the inode is allocated, and return ENOENT
> > ("nothing to check here") to userspace if this is not the case. The
> > imap lookup is why both functions call xchk_iget_agi.
>
> Ok, so given all this, all I really want then is better names for
> the functions, as "setup" and "get" don't convey any of this. :)
>
> Perhaps xchk_setup_inode() -> xchk_iget_for_record_check() and
I'd rather make this function static to inode.c and export a const
global struct xchk_meta_ops pointing to this function. There's really
no need for the external declaration aside from populating the
meta_scrub_ops table in scrub.c. The reason why I haven't done that
already is that doing that cleanup will likely cause ~23 merge conflicts
all the way down the branch as I add online repair functions. Perhaps
the next time I make a branchwide change.
Second, xchk_setup_inode doesn't necessarily return a cached inode,
which is what most iget functions do -- if the read fails, it'll lock
the AGI buffer to the scrub transaction.
I haven't any strong objections to renaming this
xchk_setup_inode_record, if that's what's needed to get this patchset
through review.
> xchk_get_inode() -> xchk_iget_for_scrubbing(). This gives an
> indication taht they are being used for different purposes, and the
> implementation is tailored to the requirements of those specific
> operations....
I'll make this change, however.
--D
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
