On Tue, Dec 22, 2015 at 08:37:05AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> This can take a long time when there are millions of uncertain inodes in badly
> broken filesystems. THe processing is per-ag, the data structures are all
> per-ag, and the load is mostly CPU time spent checking CRCs on each
> uncertaini inode. Parallelising reduced the runtime of this phase on a badly
> broken filesytem from ~30 minutes to under 5 miniutes.
>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
This one seems a bit more scary simply because the amount of work
involved in phase 3, such as inode processing and whatnot. I don't think
that everything in there is necessarily AG local as the commit log
description above implies. On a skim through, I do see that we have
ag_locks[agno].lock for cases that can cross AG boundaries, such as
checking block allocation state (get_bmap()/set_bmap()) of bmapbt blocks
(iiuc?), for example.
So I can't really spot any actual problems, but there's a lot of code
down in there. I'm fine with it as long as testing bears out and this
gets a reasonable amount of soak time such that we can hopefully catch
any serious issues or areas currently lacking sufficient locking:
Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
> repair/phase3.c | 59 ++++++++++++++++++++++++++++++++++++++++++++---------
> repair/protos.h | 2 +-
> repair/xfs_repair.c | 2 +-
> 3 files changed, 51 insertions(+), 12 deletions(-)
>
> diff --git a/repair/phase3.c b/repair/phase3.c
> index 76c9440..0890a27 100644
> --- a/repair/phase3.c
> +++ b/repair/phase3.c
> @@ -28,6 +28,7 @@
> #include "dinode.h"
> #include "progress.h"
> #include "bmap.h"
> +#include "threads.h"
>
> static void
> process_agi_unlinked(
> @@ -87,10 +88,33 @@ process_ags(
> do_inode_prefetch(mp, ag_stride, process_ag_func, false, false);
> }
>
> +static void
> +do_uncertain_aginodes(
> + work_queue_t *wq,
> + xfs_agnumber_t agno,
> + void *arg)
> +{
> + int *count = arg;
> +
> + *count = process_uncertain_aginodes(wq->mp, agno);
> +
> +#ifdef XR_INODE_TRACE
> + fprintf(stderr,
> + "\t\t phase 3 - ag %d process_uncertain_inodes returns %d\n",
> + *count, j);
> +#endif
> +
> + PROG_RPT_INC(prog_rpt_done[agno], 1);
> +}
> +
> void
> -phase3(xfs_mount_t *mp)
> +phase3(
> + struct xfs_mount *mp,
> + int scan_threads)
> {
> - int i, j;
> + int i, j;
> + int *counts;
> + work_queue_t wq;
>
> do_log(_("Phase 3 - for each AG...\n"));
> if (!no_modify)
> @@ -129,20 +153,35 @@ phase3(xfs_mount_t *mp)
> */
> do_log(_(" - process newly discovered inodes...\n"));
> set_progress_msg(PROG_FMT_NEW_INODES, (__uint64_t) glob_agcount);
> +
> + counts = calloc(sizeof(*counts), mp->m_sb.sb_agcount);
> + if (!counts) {
> + do_abort(_("no memory for uncertain inode counts\n"));
> + return;
> + }
> +
> do {
> /*
> * have to loop until no ag has any uncertain
> * inodes
> */
> j = 0;
> - for (i = 0; i < mp->m_sb.sb_agcount; i++) {
> - j += process_uncertain_aginodes(mp, i);
> -#ifdef XR_INODE_TRACE
> - fprintf(stderr,
> - "\t\t phase 3 - process_uncertain_inodes returns %d\n", j);
> -#endif
> - PROG_RPT_INC(prog_rpt_done[i], 1);
> - }
> + memset(counts, 0, mp->m_sb.sb_agcount * sizeof(*counts));
> +
> + create_work_queue(&wq, mp, scan_threads);
> +
> + for (i = 0; i < mp->m_sb.sb_agcount; i++)
> + queue_work(&wq, do_uncertain_aginodes, i, &counts[i]);
> +
> + destroy_work_queue(&wq);
> +
> + /* tally up the counts */
> + for (i = 0; i < mp->m_sb.sb_agcount; i++)
> + j += counts[i];
> +
> } while (j != 0);
> +
> + free(counts);
> +
> print_final_rpt();
> }
> diff --git a/repair/protos.h b/repair/protos.h
> index b113aca..0290420 100644
> --- a/repair/protos.h
> +++ b/repair/protos.h
> @@ -46,7 +46,7 @@ void thread_init(void);
>
> void phase1(struct xfs_mount *);
> void phase2(struct xfs_mount *, int);
> -void phase3(struct xfs_mount *);
> +void phase3(struct xfs_mount *, int);
> void phase4(struct xfs_mount *);
> void phase5(struct xfs_mount *);
> void phase6(struct xfs_mount *);
> diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> index fcdb212..5d5f3aa 100644
> --- a/repair/xfs_repair.c
> +++ b/repair/xfs_repair.c
> @@ -871,7 +871,7 @@ main(int argc, char **argv)
> if (do_prefetch)
> init_prefetch(mp);
>
> - phase3(mp);
> + phase3(mp, phase2_threads);
> timestamp(PHASE_END, 3, NULL);
>
> phase4(mp);
> --
> 2.5.0
>
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
