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

[Dave Chinner <david@fromorbit.com>]

On Wed, Sep 19, 2018 at 07:44:05AM -0700, Darrick J. Wong wrote:
> On Wed, Sep 19, 2018 at 02:56:16PM +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>
> > ---
> >  mkfs/xfs_mkfs.c | 20 ++++++++++++++++----
> >  1 file changed, 16 insertions(+), 4 deletions(-)
> > 
> > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> > index 2e53c1e8..81d9859a 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,8 +2418,16 @@ open_devices(
> >  	xi->dsize &= sector_mask;
> >  	xi->rtsize &= sector_mask;
> >  	xi->logBBsize &= (uint64_t)-1 << (max(cfg->lsectorlog, 10) - BBSHIFT);
> > +}
> >  
> > -
> > +static void
> > +discard_data(
> 
> Perhaps discard_devices(), since this function can DISCARD more than
> just the data device.
> 
> > +	struct libxfs_xinit	*xi,
> > +	bool			discard)
> > +{
> > +	/*
> > +	 * This function has to be called after libxfs has been initialized.
> > +	 */
> >  	if (!discard)
> >  		return;
> >  
> 
> While we're on the topic, I notice that we skip discard for any device
> that's actually a file.  Seeing as fallocate(PUNCH_HOLE) works on files
> (and block devices), is there a reason why we avoid punching out fs
> image files?

Yeah - preallocated image files shouldn't be punched, because it
defeats the whole purpose of setting up preallocated image files.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com