Re: [PATCH 2/3] xfs_scrub: perform media scans of entire devices

[Allison Collins <allison.henderson@oracle.com>]

Looks OK:
Reviewed-by: Allison Collins <allison.henderson@oracle.com>

On 9/5/19 8:40 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Add a new feature to xfs_scrub where specifying multiple -x will cause
> it to perform a media scan of the entire disk, not just the file data
> areas.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>   man/man8/xfs_scrub.8 |    3 +++
>   scrub/phase6.c       |   60 ++++++++++++++++++++++++++++++++++++++++++++++----
>   scrub/phase7.c       |    5 ++++
>   scrub/xfs_scrub.c    |    4 ++-
>   scrub/xfs_scrub.h    |    1 +
>   5 files changed, 66 insertions(+), 7 deletions(-)
> 
> 
> diff --git a/man/man8/xfs_scrub.8 b/man/man8/xfs_scrub.8
> index 18948a4e..872a088c 100644
> --- a/man/man8/xfs_scrub.8
> +++ b/man/man8/xfs_scrub.8
> @@ -97,6 +97,9 @@ Prints the version number and exits.
>   .TP
>   .B \-x
>   Read all file data extents to look for disk errors.
> +If this option is given more than once, scrub all disk contents.
> +If this option is given more than twice, report errors even if they have not
> +yet caused data loss.
>   .B xfs_scrub
>   will issue O_DIRECT reads to the block device directly.
>   If the block device is a SCSI disk, it will instead issue READ VERIFY commands
> diff --git a/scrub/phase6.c b/scrub/phase6.c
> index c50fb8fb..7bfb856a 100644
> --- a/scrub/phase6.c
> +++ b/scrub/phase6.c
> @@ -167,7 +167,9 @@ report_data_loss(
>   	int				ret;
>   
>   	/* Only report errors for real extents. */
> -	if (bmap->bm_flags & (BMV_OF_PREALLOC | BMV_OF_DELALLOC))
> +	if (scrub_data < 3 && (bmap->bm_flags & BMV_OF_PREALLOC))
> +		return true;
> +	if (bmap->bm_flags & BMV_OF_DELALLOC)
>   		return true;
>   
>   	if (fsx->fsx_xflags & FS_XFLAG_REALTIME)
> @@ -355,7 +357,7 @@ ioerr_fsmap_report(
>   	uint64_t		err_off;
>   
>   	/* Don't care about unwritten extents. */
> -	if (map->fmr_flags & FMR_OF_PREALLOC)
> +	if (scrub_data < 3 && (map->fmr_flags & FMR_OF_PREALLOC))
>   		return true;
>   
>   	if (err_physical > map->fmr_physical)
> @@ -602,6 +604,49 @@ clean_pool(
>   	return ret;
>   }
>   
> +/* Schedule an entire disk for read verification. */
> +static int
> +verify_entire_disk(
> +	struct read_verify_pool		*rvp,
> +	struct disk			*disk,
> +	struct media_verify_state	*vs)
> +{
> +	return read_verify_schedule_io(rvp, 0, disk->d_size, vs);
> +}
> +
> +/* Scan every part of every disk. */
> +static bool
> +verify_all_disks(
> +	struct scrub_ctx		*ctx,
> +	struct media_verify_state	*vs)
> +{
> +	int				ret;
> +
> +	ret = verify_entire_disk(vs->rvp_data, ctx->datadev, vs);
> +	if (ret) {
> +		str_liberror(ctx, ret, _("scheduling datadev verify"));
> +		return false;
> +	}
> +
> +	if (ctx->logdev) {
> +		ret = verify_entire_disk(vs->rvp_log, ctx->logdev, vs);
> +		if (ret) {
> +			str_liberror(ctx, ret, _("scheduling logdev verify"));
> +			return false;
> +		}
> +	}
> +
> +	if (ctx->rtdev) {
> +		ret = verify_entire_disk(vs->rvp_realtime, ctx->rtdev, vs);
> +		if (ret) {
> +			str_liberror(ctx, ret, _("scheduling rtdev verify"));
> +			return false;
> +		}
> +	}
> +
> +	return true;
> +}
> +
>   /*
>    * Read verify all the file data blocks in a filesystem.  Since XFS doesn't
>    * do data checksums, we trust that the underlying storage will pass back
> @@ -657,7 +702,11 @@ xfs_scan_blocks(
>   			goto out_logpool;
>   		}
>   	}
> -	moveon = xfs_scan_all_spacemaps(ctx, xfs_check_rmap, &vs);
> +
> +	if (scrub_data > 1)
> +		moveon = verify_all_disks(ctx, &vs);
> +	else
> +		moveon = xfs_scan_all_spacemaps(ctx, xfs_check_rmap, &vs);
>   	if (!moveon)
>   		goto out_rtpool;
>   
> @@ -729,8 +778,9 @@ xfs_estimate_verify_work(
>   	if (!moveon)
>   		return moveon;
>   
> -	*items = cvt_off_fsb_to_b(&ctx->mnt,
> -			(d_blocks - d_bfree) + (r_blocks - r_bfree));
> +	*items = cvt_off_fsb_to_b(&ctx->mnt, d_blocks + r_blocks);
> +	if (scrub_data == 1)
> +		*items -= cvt_off_fsb_to_b(&ctx->mnt, d_bfree + r_bfree);
>   	*nr_threads = disk_heads(ctx->datadev);
>   	*rshift = 20;
>   	return moveon;
> diff --git a/scrub/phase7.c b/scrub/phase7.c
> index bc959f5b..570ceb3f 100644
> --- a/scrub/phase7.c
> +++ b/scrub/phase7.c
> @@ -255,6 +255,11 @@ _("%.*f%s inodes counted; %.*f%s inodes checked.\n"),
>   		double		b1, b2;
>   		char		*b1u, *b2u;
>   
> +		if (scrub_data > 1) {
> +			used_data = cvt_off_fsb_to_b(&ctx->mnt, d_blocks);
> +			used_rt = cvt_off_fsb_to_b(&ctx->mnt, r_blocks);
> +		}
> +
>   		b1 = auto_space_units(used_data + used_rt, &b1u);
>   		b2 = auto_space_units(ctx->bytes_checked, &b2u);
>   		fprintf(stdout,
> diff --git a/scrub/xfs_scrub.c b/scrub/xfs_scrub.c
> index 2d554340..46876522 100644
> --- a/scrub/xfs_scrub.c
> +++ b/scrub/xfs_scrub.c
> @@ -139,7 +139,7 @@ unsigned int			force_nr_threads;
>   bool				verbose;
>   
>   /* Should we scrub the data blocks? */
> -static bool			scrub_data;
> +int				scrub_data;
>   
>   /* Size of a memory page. */
>   long				page_size;
> @@ -666,7 +666,7 @@ main(
>   			fflush(stdout);
>   			return SCRUB_RET_SUCCESS;
>   		case 'x':
> -			scrub_data = true;
> +			scrub_data++;
>   			break;
>   		case '?':
>   			/* fall through */
> diff --git a/scrub/xfs_scrub.h b/scrub/xfs_scrub.h
> index 54876acb..6558bad7 100644
> --- a/scrub/xfs_scrub.h
> +++ b/scrub/xfs_scrub.h
> @@ -21,6 +21,7 @@ extern bool			want_fstrim;
>   extern bool			stderr_isatty;
>   extern bool			stdout_isatty;
>   extern bool			is_service;
> +extern int			scrub_data;
>   
>   enum scrub_mode {
>   	SCRUB_MODE_DRY_RUN,
>