mkfs: default to CRC enabled filesystems
PATCH 1/1 mkfs: default to CRC enabled filesystems
From: Dave Chinner <[email protected]>
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.
--- mkfs/xfs_mkfs.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)
mkfs/xfs_mkfs.c +10 -11
@@ -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 ; }
Signature · 6 lines
2.0.0 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 5 lines · Dave Chinner
From: Dave Chinner <[email protected]>
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" ;)
Quoted 37 lines
it into distro kernels, too.
Signed-off-by Dave Chinner <[email protected]>--- mkfs/xfs_mkfs.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)mkfs/xfs_mkfs.c +2
-11@@ -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" ));
mkfs/xfs_mkfs.c _("V2 attribute format always enabled on CRC enabled filesytems\n")); · line 1878
Quoted 10 lines
mkfs/xfs_mkfs.c +8
-0+ }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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 13 lines · Eric Sandeen
Quoted 5 lines · Dave Chinner
From: Dave Chinner <[email protected]>
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" ;)
Quoted 4 lines
it into distro kernels, too.
Signed-off-by Dave Chinner <[email protected]>
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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 51 lines · Eric Sandeen
Quoted 5 lines · Dave Chinner
From: Dave Chinner <[email protected]>
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" ;)
Quoted 42 lines
it into distro kernels, too.
Signed-off-by Dave Chinner <[email protected]>--- mkfs/xfs_mkfs.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)mkfs/xfs_mkfs.c +7
-11@@ -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
Quoted 8 lines
mkfs/xfs_mkfs.c _("V2 attribute format always enabled on CRC enabled filesytems\n")); · line 1878
Quoted 5 lines
mkfs/xfs_mkfs.c +3
-0+* 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".
Quoted 8 lines
# 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.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 1 line · Dave Chinner
On Thu, Mar 19, 2015 at 09:55:25AM -0500, Eric Sandeen wrote:
...
Quoted 23 lines
Quoted 2 lines
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".
Quoted 8 lines
# 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.
Quoted 3 lines
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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 34 lines · Eric Sandeen
Quoted 1 line · Dave Chinner
On Thu, Mar 19, 2015 at 09:55:25AM -0500, Eric Sandeen wrote:
...
Quoted 23 lines
Quoted 2 lines
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".
Quoted 8 lines
# 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.
Quoted 3 lines
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.
Quoted 7 lines
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.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 1 line · Dave Chinner
On Fri, Mar 20, 2015 at 10:26:49AM -0500, Eric Sandeen wrote:
...
Quoted 7 lines
Quoted 1 line
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.
Quoted 4 lines
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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 4 lines · Dave Chinner
From: Dave Chinner <[email protected]>
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:
Quoted 1 line
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)
Quoted 1 line
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)
Quoted 1 line
time crc32 1G ## in cache
5B64C2B0
4.00sec 3.57usr 0.42sys (100.00% cpu)
(using Xeon X5660 @ 2.80GHz)
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 4 lines · Linda Walsh
Quoted 3 lines · Dave Chinner
From: Dave Chinner <[email protected]>
Quoted 8 lines
Quoted 2 lines
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.
====
Quoted 1 line
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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 22 lines · Linda Walsh
Quoted 4 lines · Dave Chinner
From: Dave Chinner <[email protected]>
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:
Quoted 1 line
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)Quoted 1 line
dropcaches
76.34sec 0.00usr 69.22sys (90.69% cpu)
Quoted 8 lines
Ishtar:law/bin> time crc32 1G ## no cache
5B64C2B0
6.67sec 3.85usr 0.94sys (71.82% cpu)Quoted 1 line
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
Quoted 6 lines
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs