diff mbox series

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

Message ID 156774082719.2643094.12163874100429393033.stgit@magnolia (mailing list archive)
State Superseded
Headers show
Series xfsprogs: scrub filesystem summary counters | expand

Commit Message

Darrick J. Wong Sept. 6, 2019, 3:33 a.m. UTC 
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>
---
 scrub/scrub.c |   95 ++++++++++++++++++++++++++++-----------------------------
 1 file changed, 47 insertions(+), 48 deletions(-)

Comments

Dave Chinner Sept. 10, 2019, 12:19 a.m. UTC | #1 
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....

>  /* 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....

Other than that, it looks fine.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

Cheers,

Dave.
Darrick J. Wong Sept. 16, 2019, 9:26 p.m. UTC | #2 
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
diff mbox series

Patch

diff --git a/scrub/scrub.c b/scrub/scrub.c
index 083ed9a1..b1927f38 100644
--- a/scrub/scrub.c
+++ b/scrub/scrub.c
@@ -276,47 +276,64 @@  xfs_scrub_save_repair(
 	return true;
 }
 
+/* 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,
+	};
+	enum check_outcome		fix;
+
+	background_sleep();
+
+	/* Check the item. */
+	fix = xfs_check_metadata(ctx, &meta, false);
+	progress_add(1);
+
+	switch (fix) {
+	case CHECK_ABORT:
+		return ECANCELED;
+	case CHECK_REPAIR:
+		if (!xfs_scrub_save_repair(ctx, alist, &meta))
+			return ENOMEM;
+		/* fall through */
+	case CHECK_DONE:
+		return 0;
+	default:
+		/* CHECK_RETRY should never happen. */
+		abort();
+	}
+}
+
 /* 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;
+
 		if (sc->type != scrub_type)
 			continue;
 		if (sc->flags & XFROG_SCRUB_DESCR_SUMMARY)
 			continue;
 
-		meta.sm_type = type;
-		meta.sm_flags = 0;
-		meta.sm_agno = agno;
-		background_sleep();
-
-		/* Check the item. */
-		fix = xfs_check_metadata(ctx, &meta, false);
-		progress_add(1);
-		switch (fix) {
-		case CHECK_ABORT:
+		ret = xfs_scrub_meta(ctx, type, agno, alist);
+		if (ret)
 			return false;
-		case CHECK_REPAIR:
-			if (!xfs_scrub_save_repair(ctx, alist, &meta))
-				return false;
-			/* fall through */
-		case CHECK_DONE:
-			continue;
-		case CHECK_RETRY:
-			abort();
-			break;
-		}
 	}
 
 	return true;
@@ -332,28 +349,10 @@  xfs_scrub_primary_super(
 	struct scrub_ctx		*ctx,
 	struct xfs_action_list		*alist)
 {
-	struct xfs_scrub_metadata	meta = {
-		.sm_type = XFS_SCRUB_TYPE_SB,
-	};
-	enum check_outcome		fix;
-
-	/* Check the item. */
-	fix = xfs_check_metadata(ctx, &meta, false);
-	switch (fix) {
-	case CHECK_ABORT:
-		return false;
-	case CHECK_REPAIR:
-		if (!xfs_scrub_save_repair(ctx, alist, &meta))
-			return false;
-		/* fall through */
-	case CHECK_DONE:
-		return true;
-	case CHECK_RETRY:
-		abort();
-		break;
-	}
+	int				ret;
 
-	return true;
+	ret = xfs_scrub_meta(ctx, XFS_SCRUB_TYPE_SB, 0, alist);
+	return ret == 0;
 }
 
 /* Scrub each AG's header blocks. */
@@ -363,7 +362,7 @@  xfs_scrub_ag_headers(
 	xfs_agnumber_t			agno,
 	struct xfs_action_list		*alist)
 {
-	return xfs_scrub_metadata(ctx, XFROG_SCRUB_TYPE_AGHEADER, agno, alist);
+	return xfs_scrub_meta_type(ctx, XFROG_SCRUB_TYPE_AGHEADER, agno, alist);
 }
 
 /* Scrub each AG's metadata btrees. */
@@ -373,7 +372,7 @@  xfs_scrub_ag_metadata(
 	xfs_agnumber_t			agno,
 	struct xfs_action_list		*alist)
 {
-	return xfs_scrub_metadata(ctx, XFROG_SCRUB_TYPE_PERAG, agno, alist);
+	return xfs_scrub_meta_type(ctx, XFROG_SCRUB_TYPE_PERAG, agno, alist);
 }
 
 /* Scrub whole-FS metadata btrees. */
@@ -382,7 +381,7 @@  xfs_scrub_fs_metadata(
 	struct scrub_ctx		*ctx,
 	struct xfs_action_list		*alist)
 {
-	return xfs_scrub_metadata(ctx, XFROG_SCRUB_TYPE_FS, 0, alist);
+	return xfs_scrub_meta_type(ctx, XFROG_SCRUB_TYPE_FS, 0, alist);
 }
 
 /* How many items do we have to check? */