[PATCH] mkfs: default to CRC enabled filesystems

[PATCH] mkfs: default to CRC enabled filesystems

[Dave Chinner]

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
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;
 	}
 
-- 
2.0.0

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

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

[Eric Sandeen]

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:

# 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?

-Eric

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

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

[Eric Sandeen]

On 3/19/15 9:55 AM, 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>

Forgot to add; needs manpage updates as well:

"By default, mkfs.xfs will not enable metadata CRCs."
"By default, mkfs.xfs will not create free inode btrees."

-Eric

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

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

[Dave Chinner]

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

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

[Linda Walsh]

Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> It's time to change the mkfs defaults to enable CRCs for all new
> filesystems. 
---
	"Sounds good", but what exactly is CRC'ed?

I.e. all data? Or how much data/second would I expect
to 'need' CRC'ing?

Some quick timing stuff:

> time  dd if=/dev/zero of=1G bs=1M count=1024 oflag=direct    
1024+0 records in
1024+0 records out
1073741824 bytes (1.1 GB) copied, 1.76947 s, 607 MB/s
1.77sec 0.00usr 0.50sys (28.38% cpu)

> dropcaches
76.34sec 0.00usr 69.22sys (90.69% cpu)

Ishtar:law/bin> time crc32 1G		## no cache
5B64C2B0
6.67sec 3.85usr 0.94sys (71.82% cpu)

> time crc32 1G				## in cache
5B64C2B0
4.00sec 3.57usr 0.42sys (100.00% cpu)
(using Xeon X5660 @ 2.80GHz)


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

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

[Greg Freemyer]

On Thu, Mar 19, 2015 at 10:27 PM, Linda Walsh <xfs@tlinx.org> wrote:
> Dave Chinner wrote:
>>
>> From: Dave Chinner <dchinner@redhat.com>
>>
>> It's time to change the mkfs defaults to enable CRCs for all new
>> filesystems.
>
> ---
>         "Sounds good", but what exactly is CRC'ed?
>
> I.e. all data? Or how much data/second would I expect
> to 'need' CRC'ing?

metadata only.

====
>From the man page:

       -m global_metadata_options
              These options specify metadata format options that either
              apply to the entire filesystem or aren't easily characterised
              by a specific functionality group. The valid
              global_metadata_options are:

                   crc=value
                          This is used to create a filesystem which
                          maintains and checks CRC information in all
                          metadata objects on disk. The value is either 0 to
                          disable the feature, or 1 to enable the use of
                          CRCs.

                          CRCs enable enhanced error detection due to
                          hardware issues, whilst the format changes also
                          improves crash recovery algorithms and the ability
                          of various tools to validate and repair metadata
                          corruptions when they are found.  The CRC
                          algorithm used is CRC32c, so the overhead is
                          dependent on CPU architecture as some CPUs have
                          hardware acceleration of this algorithm.
                          Typically the overhead of calculating and checking
                          the CRCs is not noticable in normal operation.

                          By default, mkfs.xfs will not enable metadata
                          CRCs.
====

The purpose of the patch is to drop "not" from the last sentence.

Greg

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

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

[Eric Sandeen]

On 3/19/15 6:13 PM, Dave Chinner wrote:
> On Thu, Mar 19, 2015 at 09:55:25AM -0500, Eric Sandeen wrote:

...

>> 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...

Oops, I accidentally missed reply-all last time.

I just think that silently changing an explicitly-specified option seems
like a bad idea.

Perhaps if defaults are specified before getopt, the getopt handlers can
flag the incorrect combination, and bail without the extra flag.

I don't see how this requires xfstests rework, though?

-Eric

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

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

[Darrick J. Wong]

On Thu, Mar 19, 2015 at 07:27:25PM -0700, Linda Walsh wrote:
> Dave Chinner wrote:
> >From: Dave Chinner <dchinner@redhat.com>
> >
> >It's time to change the mkfs defaults to enable CRCs for all new
> >filesystems.
> ---
> 	"Sounds good", but what exactly is CRC'ed?
> 
> I.e. all data? Or how much data/second would I expect
> to 'need' CRC'ing?
> 
> Some quick timing stuff:
> 
> >time  dd if=/dev/zero of=1G bs=1M count=1024 oflag=direct
> 1024+0 records in
> 1024+0 records out
> 1073741824 bytes (1.1 GB) copied, 1.76947 s, 607 MB/s
> 1.77sec 0.00usr 0.50sys (28.38% cpu)
> 
> >dropcaches
> 76.34sec 0.00usr 69.22sys (90.69% cpu)
> 
> Ishtar:law/bin> time crc32 1G		## no cache
> 5B64C2B0
> 6.67sec 3.85usr 0.94sys (71.82% cpu)
> 
> >time crc32 1G				## in cache
> 5B64C2B0
> 4.00sec 3.57usr 0.42sys (100.00% cpu)
> (using Xeon X5660 @ 2.80GHz)

Usually the kernel will load one of the CRC32c accelerators to reduce the
impact of the metadata checksumming.  Not sure if 'crc32' knows how to use
the new instructions Intel built into that X5660, though I'm guessing not.
Also, XFS uses crc32c, not crc32.

--D

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

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

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

[Dave Chinner]

On Fri, Mar 20, 2015 at 10:26:49AM -0500, Eric Sandeen wrote:
> On 3/19/15 6:13 PM, Dave Chinner wrote:
> > On Thu, Mar 19, 2015 at 09:55:25AM -0500, Eric Sandeen wrote:
> 
> ...
> 
> >> 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...
> 
> Oops, I accidentally missed reply-all last time.
> 
> I just think that silently changing an explicitly-specified option seems
> like a bad idea.
> 
> Perhaps if defaults are specified before getopt, the getopt handlers can
> flag the incorrect combination, and bail without the extra flag.
> 
> I don't see how this requires xfstests rework, though?

About 50 tests fail with:

xfs/031 6s ... - output mismatch (see /home/dave/src/xfstests-dev/results//xfs_v4/xfs/031.out.bad)
    --- tests/xfs/031.out       2014-01-20 16:57:33.000000000 +1100
    +++ /home/dave/src/xfstests-dev/results//xfs_v4/xfs/031.out.bad     2015-03-18 18:41:36.000000000 +1100
    @@ -38,6 +38,7 @@
     done
     
     === twenty entries (block form)
    +warning: finobt not supported without CRC support, disabled.
     Repairing, iteration 1
     Phase 1 - find and verify superblock...
     Phase 2 - using <TYPEOF> log
    ...
    (Run 'diff -u tests/xfs/031.out /home/dave/src/xfstests-dev/results//xfs_v4/xfs/031.out.bad'  to see the entire diff)

When run with MKFS_OPTIONS="-m crc=0".

i.e. finobt is not specified, but mkfs issues warnings about it.

I've reworked the patch, anyway, so there's no need to continue the
discussion on this...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

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

[Eric Sandeen]

On 3/20/15 4:47 PM, Dave Chinner wrote:
> On Fri, Mar 20, 2015 at 10:26:49AM -0500, Eric Sandeen wrote:

...

>> I don't see how this requires xfstests rework, though?
> 
> About 50 tests fail with:

>     +warning: finobt not supported without CRC support, disabled.

> When run with MKFS_OPTIONS="-m crc=0".

Ok, I didn't mean it shouldn't be automatically disabled, I just meant
it should probably reject invalid option combinations on the cmdline.
 
> i.e. finobt is not specified, but mkfs issues warnings about it.
> 
> I've reworked the patch, anyway, so there's no need to continue the
> discussion on this...

Heh, ok.  <EOM>

-Eric

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