On 1/13/15 2:08 PM, Brian Foster wrote:
> 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 <bfoster@xxxxxxxxxx>
This looks fine too. I can't see any reason to do the strided check,
even if it works...
Reviewed-by: Eric Sandeen <sandeen@xxxxxxxxxx>
> ---
> repair/globals.h | 2 --
> repair/sb.c | 63 ++++++++++++++++++--------------------------------------
> 2 files changed, 20 insertions(+), 45 deletions(-)
>
> diff --git a/repair/globals.h b/repair/globals.h
> index 6207ca1..f386686 100644
> --- a/repair/globals.h
> +++ b/repair/globals.h
> @@ -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 */
> diff --git a/repair/sb.c b/repair/sb.c
> index dc154f7..a663728 100644
> --- a/repair/sb.c
> +++ b/repair/sb.c
> @@ -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++;
> }
> }
>
> @@ -867,6 +845,5 @@ verify_set_primary_sb(xfs_sb_t *rsb,
> out_free_list:
> free_geo(list);
> free(sb);
> - free(checked);
> return retval;
> }
>
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
