Re: [PATCH 1/2 v2] mkfs: discard only after all validations

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

On Thu, Sep 20, 2018 at 11:20:06AM +0200, Jan Tulak wrote:
> Discard should happen only when everything has been validated, just
> before we start writing to the device. If it happens earlier, it is
> possible that mkfs will abort, but managed to already wipe data. This
> patch moves the discard to the latest possible moment.
> 
> Signed-off-by: Jan Tulak <jtulak@redhat.com>

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

--D

> ---
> CHANGES:
> v2
> * discard_data -> discard_devices
> * move the discard condition outside of the function
> 
> ---
>  mkfs/xfs_mkfs.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 2e53c1e8..c97c69e8 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -2389,8 +2389,7 @@ _("log stripe unit (%d bytes) is too large (maximum is 256KiB)\n"
>  static void
>  open_devices(
>  	struct mkfs_params	*cfg,
> -	struct libxfs_xinit	*xi,
> -	bool			discard)
> +	struct libxfs_xinit	*xi)
>  {
>  	uint64_t		sector_mask;
>  
> @@ -2419,10 +2418,15 @@ open_devices(
>  	xi->dsize &= sector_mask;
>  	xi->rtsize &= sector_mask;
>  	xi->logBBsize &= (uint64_t)-1 << (max(cfg->lsectorlog, 10) - BBSHIFT);
> +}
>  
> -
> -	if (!discard)
> -		return;
> +static void
> +discard_devices(
> +	struct libxfs_xinit	*xi)
> +{
> +	/*
> +	 * This function has to be called after libxfs has been initialized.
> +	 */
>  
>  	if (!xi->disfile)
>  		discard_blocks(xi->ddev, xi->dsize);
> @@ -3901,7 +3905,7 @@ main(
>  	/*
>  	 * Open and validate the device configurations
>  	 */
> -	open_devices(&cfg, &xi, (discard && !dry_run));
> +	open_devices(&cfg, &xi);
>  	validate_datadev(&cfg, &cli);
>  	validate_logdev(&cfg, &cli, &logfile);
>  	validate_rtdev(&cfg, &cli, &rtfile);
> @@ -3952,6 +3956,12 @@ main(
>  			exit(0);
>  	}
>  
> +	/*
> +	 * All values have been validated, discard the old device layout.
> +	 */
> +	if (discard && !dry_run)
> +		discard_devices(&xi);
> +
>  	/*
>  	 * we need the libxfs buffer cache from here on in.
>  	 */
> -- 
> 2.18.0
>