* [PATCH] mkfs: default to CRC enabled filesystems @ 2015-03-18 23:22 Dave Chinner 2015-03-19 14:55 ` Eric Sandeen 2015-03-20 2:27 ` Linda Walsh 0 siblings, 2 replies; 10+ messages in thread From: Dave Chinner @ 2015-03-18 23:22 UTC (permalink / raw) To: xfs 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] mkfs: default to CRC enabled filesystems 2015-03-18 23:22 [PATCH] mkfs: default to CRC enabled filesystems Dave Chinner @ 2015-03-19 14:55 ` Eric Sandeen 2015-03-19 14:58 ` Eric Sandeen 2015-03-19 23:13 ` Dave Chinner 2015-03-20 2:27 ` Linda Walsh 1 sibling, 2 replies; 10+ messages in thread From: Eric Sandeen @ 2015-03-19 14:55 UTC (permalink / raw) To: Dave Chinner, xfs 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mkfs: default to CRC enabled filesystems 2015-03-19 14:55 ` Eric Sandeen @ 2015-03-19 14:58 ` Eric Sandeen 2015-03-19 23:13 ` Dave Chinner 1 sibling, 0 replies; 10+ messages in thread From: Eric Sandeen @ 2015-03-19 14:58 UTC (permalink / raw) To: Dave Chinner, xfs 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mkfs: default to CRC enabled filesystems 2015-03-19 14:55 ` Eric Sandeen 2015-03-19 14:58 ` Eric Sandeen @ 2015-03-19 23:13 ` Dave Chinner 2015-03-20 15:26 ` Eric Sandeen 1 sibling, 1 reply; 10+ messages in thread From: Dave Chinner @ 2015-03-19 23:13 UTC (permalink / raw) To: Eric Sandeen; +Cc: xfs 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mkfs: default to CRC enabled filesystems 2015-03-19 23:13 ` Dave Chinner @ 2015-03-20 15:26 ` Eric Sandeen 2015-03-20 21:47 ` Dave Chinner 0 siblings, 1 reply; 10+ messages in thread From: Eric Sandeen @ 2015-03-20 15:26 UTC (permalink / raw) To: Dave Chinner; +Cc: xfs 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mkfs: default to CRC enabled filesystems 2015-03-20 15:26 ` Eric Sandeen @ 2015-03-20 21:47 ` Dave Chinner 2015-03-20 21:56 ` Eric Sandeen 0 siblings, 1 reply; 10+ messages in thread From: Dave Chinner @ 2015-03-20 21:47 UTC (permalink / raw) To: Eric Sandeen; +Cc: xfs 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mkfs: default to CRC enabled filesystems 2015-03-20 21:47 ` Dave Chinner @ 2015-03-20 21:56 ` Eric Sandeen 0 siblings, 0 replies; 10+ messages in thread From: Eric Sandeen @ 2015-03-20 21:56 UTC (permalink / raw) To: Dave Chinner; +Cc: xfs 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mkfs: default to CRC enabled filesystems 2015-03-18 23:22 [PATCH] mkfs: default to CRC enabled filesystems Dave Chinner 2015-03-19 14:55 ` Eric Sandeen @ 2015-03-20 2:27 ` Linda Walsh 2015-03-20 2:44 ` Greg Freemyer 2015-03-20 18:38 ` Darrick J. Wong 1 sibling, 2 replies; 10+ messages in thread From: Linda Walsh @ 2015-03-20 2:27 UTC (permalink / raw) To: Dave Chinner; +Cc: xfs 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mkfs: default to CRC enabled filesystems 2015-03-20 2:27 ` Linda Walsh @ 2015-03-20 2:44 ` Greg Freemyer 2015-03-20 18:38 ` Darrick J. Wong 1 sibling, 0 replies; 10+ messages in thread From: Greg Freemyer @ 2015-03-20 2:44 UTC (permalink / raw) To: Linda Walsh; +Cc: xfs-oss 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mkfs: default to CRC enabled filesystems 2015-03-20 2:27 ` Linda Walsh 2015-03-20 2:44 ` Greg Freemyer @ 2015-03-20 18:38 ` Darrick J. Wong 1 sibling, 0 replies; 10+ messages in thread From: Darrick J. Wong @ 2015-03-20 18:38 UTC (permalink / raw) To: Linda Walsh; +Cc: xfs 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-03-20 21:56 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-03-18 23:22 [PATCH] mkfs: default to CRC enabled filesystems Dave Chinner 2015-03-19 14:55 ` Eric Sandeen 2015-03-19 14:58 ` Eric Sandeen 2015-03-19 23:13 ` Dave Chinner 2015-03-20 15:26 ` Eric Sandeen 2015-03-20 21:47 ` Dave Chinner 2015-03-20 21:56 ` Eric Sandeen 2015-03-20 2:27 ` Linda Walsh 2015-03-20 2:44 ` Greg Freemyer 2015-03-20 18:38 ` Darrick J. Wong
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.