xfsprogs/repair: secondary sb scan cleanups

verify_set_primary_sb() scans the secondary superbocks based on the geometry specified in the primary and determines the most likely correct geometry by tracking how many superblocks are consistent across the set. The most frequent geometry is copied into the primary superblock. The return value is checked by the caller (phase1()) to determine whether a brute force secondary scan is necessary.

This generally occurs when not enough secondary sb's are consistent to declare the geometry correct. If enough secondaries are consistent, verify_set_primary_sb() returns the status of the last secondary sb that was scanned. Corruptions to secondary supers other than the last are thus resolved fine. If the last secondary is corrupt, however, an error is returned to phase1(). This causes a brute force scan even if enough supers were found to repair the last secondary.

Move the initialization of retval to after the sb scan to return an error only if not enough secondary supers were found to declare a correct geometry.

Signed-off-by Brian Foster <[email protected]>

---
 repair/sb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

repair/sb.c +1 -1

index ad27756..dc154f7 100644
@@ -724,7 +724,6 @@ verify_set_primary_sb(xfs_sb_t		*rsb,
 	 * sector size rather than the sector size in @rsb.
 	 */
 	size = NUM_AGH_SECTS * (1 << (XFS_MAX_SECTORSIZE_LOG));
-	retval = 0;
 	list = NULL;
 	num_ok = 0;
 	*sb_modified = 0;

@@ -779,6 +778,7 @@ verify_set_primary_sb(xfs_sb_t		*rsb,
 	/*
 	 * see if we have enough superblocks to bother with
 	 */
+	retval = 0;
 	if (num_ok < num_sbs / 2) {
 		retval = XR_INSUFF_SEC_SB;
 		goto out_free_list;

Quoted 19 lines · Brian Foster

verify_set_primary_sb() scans the secondary superbocks based on the geometry specified in the primary and determines the most likely correct geometry by tracking how many superblocks are consistent across the set. The most frequent geometry is copied into the primary superblock. The return value is checked by the caller (phase1()) to determine whether a brute force secondary scan is necessary.

This generally occurs when not enough secondary sb's are consistent to declare the geometry correct. If enough secondaries are consistent, verify_set_primary_sb() returns the status of the last secondary sb that was scanned. Corruptions to secondary supers other than the last are thus resolved fine. If the last secondary is corrupt, however, an error is returned to phase1(). This causes a brute force scan even if enough supers were found to repair the last secondary.

Move the initialization of retval to after the sb scan to return an error only if not enough secondary supers were found to declare a correct geometry.

Quoted 1 line

Signed-off-by Brian Foster <[email protected]>

Nice. Brute-force scan is awful, doing it when unnecessary stinks! :)

could this be fstest-ed?

Reviewed-by Eric Sandeen <[email protected]>

Quoted 29 lines

---
 repair/sb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

repair/sb.c +1 -2

index ad27756..dc154f7 100644
@@ -724,7 +724,6 @@ verify_set_primary_sb(xfs_sb_t		*rsb,
 	 * sector size rather than the sector size in @rsb.
 	 */
 	size = NUM_AGH_SECTS * (1 << (XFS_MAX_SECTORSIZE_LOG));
-	retval = 0;
 	list = NULL;
 	num_ok = 0;
 	*sb_modified = 0;

@@ -779,6 +778,7 @@ verify_set_primary_sb(xfs_sb_t		*rsb,
 	/*
 	 * see if we have enough superblocks to bother with
 	 */
+	retval = 0;
 	if (num_ok < num_sbs / 2) {
 		retval = XR_INSUFF_SEC_SB;
 		goto out_free_list;

@@ -868,5 +868,5 @@ out_free_list:
 	free_geo(list);
 	free(sb);
 	free(checked);
-	return(retval);

repair/sb.c out_free_list: · line 872

Quoted 3 lines

repair/sb.c +1 -0

+	return retval;
 }

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

verify_set_primary_sb() scans and verifies secondary superblocks based on the primary sb. It currently defines a maximum number of 8 superblocks to scan per iteration. It also implements a strided algorithm that appears intended to ultimately scan every secondary, albeit in a strided order.

Given that the algorithm is written to hit every sb and the stride value is initialized as follows:

	num_sbs = MIN(NUM_SBS, rsb->sb_agcount);
	skip = howmany(num_sbs, rsb->sb_agcount);

... which is guaranteed to be 1 since the howmany() parameters are backwards, the strided algorithm doesn't appear to accomplish anything that can't be done with a simple for loop. In other words, despite the max value and strided algorithm, repair always scans all of the secondary superblocks in incremental order.

Therefore, remove the strided algorithm bits and replace with a simple for loop. As a result of this cleanup, also remove the 'checked' buffer used to track repeated ag visits and the now unused NUM_SBS definition.

Signed-off-by Brian Foster <[email protected]>

---
 repair/globals.h |  2 --
 repair/sb.c      | 63 ++++++++++++++++++--------------------------------------
 2 files changed, 20 insertions(+), 45 deletions(-)

repair/globals.h +0 -2

index 6207ca1..f386686 100644
@@ -57,7 +57,6 @@
 #define XR_LOG2BSIZE_MIN	9	/* min/max fs blocksize (log2) */
 #define XR_LOG2BSIZE_MAX	16	/* 2^XR_* == blocksize */
 
-#define	NUM_SBS			8	/* max # of sbs to verify */
 #define NUM_AGH_SECTS		4	/* # of components in an ag header */
 
 /*

@@ -88,7 +87,6 @@ EXTERN char	*iobuf;			/* large buffer */
 EXTERN int	iobuf_size;
 EXTERN char	*smallbuf;		/* small (1-4 page) buffer */
 EXTERN int	smallbuf_size;
-EXTERN char	*sb_bufs[NUM_SBS];	/* superblock buffers */
 EXTERN int	sbbuf_size;
 
 /* direct I/O info */

repair/sb.c +20 -42

index dc154f7..a663728 100644
@@ -702,20 +702,11 @@ verify_set_primary_sb(xfs_sb_t		*rsb,
 	xfs_sb_t	*sb;
 	fs_geo_list_t	*list;
 	fs_geo_list_t	*current;
-	char		*checked;
 	xfs_agnumber_t	agno;
 	int		num_sbs;
-	int		skip;
 	int		size;
 	int		num_ok;
 	int		retval;
-	int		round;
-
-	/*
-	 * select the number of secondaries to try for
-	 */
-	num_sbs = MIN(NUM_SBS, rsb->sb_agcount);
-	skip = howmany(num_sbs, rsb->sb_agcount);
 
 	/*
 	 * We haven't been able to validate the sector size yet properly

@@ -727,51 +718,38 @@ verify_set_primary_sb(xfs_sb_t		*rsb,
 	list = NULL;
 	num_ok = 0;
 	*sb_modified = 0;
+	num_sbs = rsb->sb_agcount;
 
 	sb = (xfs_sb_t *) alloc_ag_buf(size);
-	checked = calloc(rsb->sb_agcount, sizeof(char));
-	if (!checked) {
-		do_error(_("calloc failed in verify_set_primary_sb\n"));
-		exit(1);
-	}
 
 	/*
 	 * put the primary sb geometry info onto the geometry list
 	 */
-	checked[sb_index] = 1;
 	get_sb_geometry(&geo, rsb);
 	list = add_geo(list, &geo, sb_index);
 
 	/*
-	 * grab N secondaries.  check them off as we get them
-	 * so we only process each one once
+	 * scan the secondaries and check them off as we get them so we only
+	 * process each one once
 	 */
-	for (round = 0; round < skip; round++)  {
-		for (agno = round; agno < rsb->sb_agcount; agno += skip)  {
-			if (checked[agno])
-				continue;
+	for (agno = 1; agno < rsb->sb_agcount; agno++) {
+		off = (xfs_off_t)agno * rsb->sb_agblocks << rsb->sb_blocklog;
 
-			off = (xfs_off_t)agno * rsb->sb_agblocks << rsb->sb_blocklog;
-
-			checked[agno] = 1;
-			retval = get_sb(sb, off, size, agno);
-			if (retval == XR_EOF)
-				goto out_free_list;
-
-			if (retval == XR_OK) {
-				/*
-				 * save away geometry info.
-				 * don't bother checking the sb
-				 * against the agi/agf as the odds
-				 * of the sb being corrupted in a way
-				 * that it is internally consistent
-				 * but not consistent with the rest
-				 * of the filesystem is really really low.
-				 */
-				get_sb_geometry(&geo, sb);
-				list = add_geo(list, &geo, agno);
-				num_ok++;
-			}
+		retval = get_sb(sb, off, size, agno);
+		if (retval == XR_EOF)
+			goto out_free_list;
+
+		if (retval == XR_OK) {
+			/*
+			 * save away geometry info. don't bother checking the
+			 * sb against the agi/agf as the odds of the sb being
+			 * corrupted in a way that it is internally consistent
+			 * but not consistent with the rest of the filesystem is
+			 * really really low.
+			 */
+			get_sb_geometry(&geo, sb);
+			list = add_geo(list, &geo, agno);
+			num_ok++;
 		}
 	}