Re: [PATCH 4/5] xfs_scrub: separate internal metadata scrub functions

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Tue, Sep 10, 2019 at 10:19:33AM +1000, Dave Chinner wrote:
> On Thu, Sep 05, 2019 at 08:33:47PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Refactor xfs_scrub_metadata into two functions -- one to make a single
> > call xfs_check_metadata, and the second retains the loop logic.  The
> > name is a little easy to confuse with other functions, so rename it to
> > reflect what it actually does: scrub all internal metadata of a given
> > class (AG header, AG metadata, FS metadata).  No functional changes.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Minor nit:
> 
> > +/* Scrub non-inode metadata, saving corruption reports for later. */
> > +static int
> > +xfs_scrub_meta(
> > +	struct scrub_ctx		*ctx,
> > +	unsigned int			type,
> > +	xfs_agnumber_t			agno,
> > +	struct xfs_action_list		*alist)
> > +{
> > +	struct xfs_scrub_metadata	meta = {
> > +		.sm_type		= type,
> > +		.sm_agno		= agno,
> > +	};
> 
> This should be called xfs_scrub_meta_type() because it only
> scrubs the specific type passed into it....

Ok.

> >  /* Scrub metadata, saving corruption reports for later. */
> >  static bool
> > -xfs_scrub_metadata(
> > +xfs_scrub_meta_type(
> >  	struct scrub_ctx		*ctx,
> >  	enum xfrog_scrub_type		scrub_type,
> >  	xfs_agnumber_t			agno,
> >  	struct xfs_action_list		*alist)
> >  {
> > -	struct xfs_scrub_metadata	meta = {0};
> >  	const struct xfrog_scrub_descr	*sc;
> > -	enum check_outcome		fix;
> > -	int				type;
> > +	unsigned int			type;
> >  
> >  	sc = xfrog_scrubbers;
> >  	for (type = 0; type < XFS_SCRUB_TYPE_NR; type++, sc++) {
> > +		int			ret;
> > +
> 
> And this should be called xfs_scrub_all_metadata() because it
> walks across all the metadata types in the filesystem and calls
> xfs_scrub_meta_type() for each type to scrub them one by one....

Ok.  I think I'll update the comments for both to make it clearer what
"type" means.

--D

> Other than that, it looks fine.
> 
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com