Re: [PATCH] mkfs: default to CRC enabled filesystems

[Dave Chinner <david@fromorbit.com>]

On Thu, Mar 19, 2015 at 09:55:25AM -0500, Eric Sandeen wrote:
> On 3/18/15 6:22 PM, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > It's time to change the mkfs defaults to enable CRCs for all new
> > filesystems. While there, also enable the free inode btree by
> > default, too, as that functioanlity has also had long enough to make
> 
> "functionality" ;)
> 
> > it into distro kernels, too.
> 
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> 
> > ---
> >  mkfs/xfs_mkfs.c | 21 ++++++++++-----------
> >  1 file changed, 10 insertions(+), 11 deletions(-)
> > 
> > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> > index 484e7a8..d7e70fa 100644
> > --- a/mkfs/xfs_mkfs.c
> > +++ b/mkfs/xfs_mkfs.c
> > @@ -1024,8 +1024,8 @@ main(
> >  	force_overwrite = 0;
> >  	worst_freelist = 0;
> >  	lazy_sb_counters = 1;
> > -	crcs_enabled = 0;
> > -	finobt = 0;
> > +	crcs_enabled = 1;
> > +	finobt = 1;
> >  	memset(&fsx, 0, sizeof(fsx));
> >  
> >  	memset(&xi, 0, sizeof(xi));
> > @@ -1866,15 +1866,14 @@ _("V2 attribute format always enabled on CRC enabled filesytems\n"));
> >  _("32 bit Project IDs always enabled on CRC enabled filesytems\n"));
> >  			usage();
> >  		}
> > -	}
> > -
> > -	/*
> > -	 * The kernel doesn't currently support crc=0,finobt=1 filesystems.
> > -	 * Catch it here, disable finobt and warn the user.
> > -	 */
> > -	if (finobt && !crcs_enabled) {
> > -		fprintf(stderr,
> > -_("warning: finobt not supported without CRC support, disabled.\n"));
> > +	} else {
> > +		/*
> > +		 * The kernel doesn't currently support crc=0,finobt=1
> > +		 * filesystems. If crcs are not enabled, the user has
> > +		 * explicitly turned them off, so just silently turn off finobt
> > +		 * so that we don't issue unnecessary warnings when non-default
> > +		 * options are selected.
> > +		 */
> >  		finobt = 0;
> >  	}
> 
> Problem here is that if both are explicitly specified, one is ignored, rather
> than letting the user know they've selected an invalid set of options:

Yup, I explicitly made that choice: turning off CRCs immediately
turns off all functionality dependent on it. Especially as the
number of errors being thrown by xfstests when run with
MKFS_OPTIONS="-m crc=0".

> # mkfs/mkfs.xfs -dfile,name=fsfile,size=1g -m crc=0,finobt=1
> meta-data=fsfile                 isize=256    agcount=4, agsize=65536 blks
>          =                       sectsz=512   attr=2, projid32bit=1
>          =                       crc=0        finobt=0
> ...

> This might require a "finobtflag" to keep track of whether it's user-specified,
> as we do with other options?

I *hate* the profusion of flags in mkfs just to detect this sort of
thing. This is a clear case where "do what I mean" rather than "do
what I say" is the prefered behaviour - the current code is a
horrible mess because it tries handle every weird combination of "do
what I say" with some error message.

I'll change it to add the stupid error message back in and go and
write all the patches for xfstests not to fail because we changed
mkfs defaults...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs