Re: [PATCH V2] xfs_repair: fix progress reporting

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

On Mon, May 18, 2020 at 08:29:40PM -0500, Eric Sandeen wrote:
> The Fixes: commit tried to avoid a segfault in case the progress timer
> went off before the first message type had been set up, but this
> had the net effect of short-circuiting the pthread start routine,
> and so the timer didn't get set up at all and we lost all fine-grained
> progress reporting.
> 
> The initial problem occurred when log zeroing took more time than the
> timer interval.
> 
> So, make a new log zeroing progress item and initialize it when we first
> set up the timer thread, to be sure that if the timer goes off while we
> are still zeroing the log, it will be initialized and correct.
> 
> (We can't offer fine-grained status on log zeroing, so it'll go from
> zero to $LOGBLOCKS with nothing in between, but it's unlikely that log
> zeroing will take so long that this really matters.)

Fixable in a separate patch if anyone else is motivated <wink>...

> Reported-by: Leonardo Vaz <lvaz@redhat.com>
> Fixes: 7f2d6b811755 ("xfs_repair: avoid segfault if reporting progre...")
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
> 
> diff --git a/repair/phase2.c b/repair/phase2.c
> index 40ea2f84..952ac4a5 100644
> --- a/repair/phase2.c
> +++ b/repair/phase2.c
> @@ -120,6 +120,9 @@ zero_log(
>  			do_error(_("failed to clear log"));
>  	}
>  
> +	/* And we are now magically complete! */
> +	PROG_RPT_INC(prog_rpt_done[0], mp->m_sb.sb_logblocks);
> +
>  	/*
>  	 * Finally, seed the max LSN from the current state of the log if this
>  	 * is a v5 filesystem.
> @@ -160,7 +163,10 @@ phase2(
>  
>  	/* Zero log if applicable */
>  	do_log(_("        - zero log...\n"));
> +
> +	set_progress_msg(PROG_FMT_ZERO_LOG, (uint64_t)mp->m_sb.sb_logblocks);
>  	zero_log(mp);
> +	print_final_rpt();
>  
>  	do_log(_("        - scan filesystem freespace and inode maps...\n"));
>  
> diff --git a/repair/progress.c b/repair/progress.c
> index 5ee08229..e5a9c1ef 100644
> --- a/repair/progress.c
> +++ b/repair/progress.c
> @@ -49,35 +49,37 @@ typedef struct progress_rpt_s {
>  
>  static
>  progress_rpt_t progress_rpt_reports[] = {
> -{FMT1, N_("scanning filesystem freespace"),			/*  0 */
> +{FMT1, N_("zeroing log"),					/*  0 */
> +	&rpt_fmts[FMT1], &rpt_types[TYPE_BLOCK]},
> +{FMT1, N_("scanning filesystem freespace"),			/*  1 */
>  	&rpt_fmts[FMT1], &rpt_types[TYPE_AG]},
> -{FMT1, N_("scanning agi unlinked lists"),			/*  1 */
> +{FMT1, N_("scanning agi unlinked lists"),			/*  2 */
>  	&rpt_fmts[FMT1], &rpt_types[TYPE_AG]},
> -{FMT2, N_("check uncertain AG inodes"),				/*  2 */
> +{FMT2, N_("check uncertain AG inodes"),				/*  3 */
>  	&rpt_fmts[FMT2], &rpt_types[TYPE_AGI_BUCKET]},
> -{FMT1, N_("process known inodes and inode discovery"),		/*  3 */
> +{FMT1, N_("process known inodes and inode discovery"),		/*  4 */
>  	&rpt_fmts[FMT1], &rpt_types[TYPE_INODE]},
> -{FMT1, N_("process newly discovered inodes"),			/*  4 */
> +{FMT1, N_("process newly discovered inodes"),			/*  5 */
>  	&rpt_fmts[FMT1], &rpt_types[TYPE_AG]},
> -{FMT1, N_("setting up duplicate extent list"),			/*  5 */
> +{FMT1, N_("setting up duplicate extent list"),			/*  6 */
>  	&rpt_fmts[FMT1], &rpt_types[TYPE_AG]},
> -{FMT1, N_("initialize realtime bitmap"),			/*  6 */
> +{FMT1, N_("initialize realtime bitmap"),			/*  7 */
>  	&rpt_fmts[FMT1], &rpt_types[TYPE_BLOCK]},
> -{FMT1, N_("reset realtime bitmaps"),				/*  7 */
> +{FMT1, N_("reset realtime bitmaps"),				/*  8 */
>  	&rpt_fmts[FMT1], &rpt_types[TYPE_AG]},
> -{FMT1, N_("check for inodes claiming duplicate blocks"),	/*  8 */
> +{FMT1, N_("check for inodes claiming duplicate blocks"),	/*  9 */
>  	&rpt_fmts[FMT1], &rpt_types[TYPE_INODE]},
> -{FMT1, N_("rebuild AG headers and trees"),	 		/*  9 */
> +{FMT1, N_("rebuild AG headers and trees"),	 		/* 10 */
>  	&rpt_fmts[FMT1], &rpt_types[TYPE_AG]},
> -{FMT1, N_("traversing filesystem"),				/* 10 */
> +{FMT1, N_("traversing filesystem"),				/* 12 */
>  	&rpt_fmts[FMT1], &rpt_types[TYPE_AG]},
> -{FMT2, N_("traversing all unattached subtrees"),		/* 11 */
> +{FMT2, N_("traversing all unattached subtrees"),		/* 12 */
>  	&rpt_fmts[FMT2], &rpt_types[TYPE_DIR]},
> -{FMT2, N_("moving disconnected inodes to lost+found"),		/* 12 */
> +{FMT2, N_("moving disconnected inodes to lost+found"),		/* 13 */
>  	&rpt_fmts[FMT2], &rpt_types[TYPE_INODE]},
> -{FMT1, N_("verify and correct link counts"),			/* 13 */
> +{FMT1, N_("verify and correct link counts"),			/* 14 */
>  	&rpt_fmts[FMT1], &rpt_types[TYPE_AG]},
> -{FMT1, N_("verify link counts"),				/* 14 */
> +{FMT1, N_("verify link counts"),				/* 15 */
>  	&rpt_fmts[FMT1], &rpt_types[TYPE_AG]}
>  };
>  
> @@ -125,7 +127,8 @@ init_progress_rpt (void)
>  	 */
>  
>  	pthread_mutex_init(&global_msgs.mutex, NULL);
> -	global_msgs.format = NULL;
> +	/* Make sure the format is set to the first phase and not NULL */
> +	global_msgs.format = &progress_rpt_reports[PROG_FMT_ZERO_LOG];
>  	global_msgs.count = glob_agcount;
>  	global_msgs.interval = report_interval;
>  	global_msgs.done   = prog_rpt_done;
> diff --git a/repair/progress.h b/repair/progress.h
> index 9de9eb72..2c1690db 100644
> --- a/repair/progress.h
> +++ b/repair/progress.h
> @@ -8,26 +8,27 @@
>  #define	PHASE_END		1
>  
>  
> -#define	PROG_FMT_SCAN_AG 	0	/* Phase 2 */
> +#define	PROG_FMT_ZERO_LOG	0	/* Phase 2 */
> +#define	PROG_FMT_SCAN_AG 	1
>  
> -#define	PROG_FMT_AGI_UNLINKED 	1	/* Phase 3 */
> -#define	PROG_FMT_UNCERTAIN      2
> -#define	PROG_FMT_PROCESS_INO	3
> -#define	PROG_FMT_NEW_INODES	4
> +#define	PROG_FMT_AGI_UNLINKED 	2	/* Phase 3 */
> +#define	PROG_FMT_UNCERTAIN      3
> +#define	PROG_FMT_PROCESS_INO	4
> +#define	PROG_FMT_NEW_INODES	5
>  
> -#define	PROG_FMT_DUP_EXTENT	5	/* Phase 4 */
> -#define	PROG_FMT_INIT_RTEXT	6
> -#define	PROG_FMT_RESET_RTBM	7
> -#define	PROG_FMT_DUP_BLOCKS	8
> +#define	PROG_FMT_DUP_EXTENT	6	/* Phase 4 */
> +#define	PROG_FMT_INIT_RTEXT	7
> +#define	PROG_FMT_RESET_RTBM	8
> +#define	PROG_FMT_DUP_BLOCKS	9
>  
> -#define	PROG_FMT_REBUILD_AG	9	/* Phase 5 */
> +#define	PROG_FMT_REBUILD_AG	10	/* Phase 5 */
>  
> -#define	PROG_FMT_TRAVERSAL	10	/* Phase 6 */
> -#define	PROG_FMT_TRAVERSSUB	11
> -#define	PROG_FMT_DISCONINODE	12
> +#define	PROG_FMT_TRAVERSAL	11	/* Phase 6 */
> +#define	PROG_FMT_TRAVERSSUB	12
> +#define	PROG_FMT_DISCONINODE	13
>  
> -#define	PROGRESS_FMT_CORR_LINK	13	/* Phase 7 */
> -#define	PROGRESS_FMT_VRFY_LINK 	14
> +#define	PROGRESS_FMT_CORR_LINK	14	/* Phase 7 */
> +#define	PROGRESS_FMT_VRFY_LINK 	15
>  
>  #define	DURATION_BUF_SIZE	512
>  
> 
>