[V2] mkfs: default to CRC enabled filesystems

[V2] 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 functionality has also had long enough to make
it into distro kernels, too. Also update the man page to reflect the
change in defaults.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 man/man8/mkfs.xfs.8 |  9 +++++----
 mkfs/xfs_mkfs.c     | 27 +++++++++++++++++----------
 2 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/man/man8/mkfs.xfs.8 b/man/man8/mkfs.xfs.8
index ad9ff3d..e18f6d1 100644
--- a/man/man8/mkfs.xfs.8
+++ b/man/man8/mkfs.xfs.8
@@ -150,7 +150,7 @@ of calculating and checking the CRCs is not noticable in normal operation.
 .IP
 By default,
 .B mkfs.xfs
-will not enable metadata CRCs.
+will enable metadata CRCs.
 .TP
 .BI finobt= value
 This option enables the use of a separate free inode btree index in each
@@ -164,10 +164,11 @@ filesystems age.
 .IP
 By default,
 .B mkfs.xfs
-will not create free inode btrees. This feature is also currently only available
-for filesystems created with the
+will create free inode btrees for filesystems created with the (default)
 .B \-m crc=1
-option set.
+option set. When the option
+.B \-m crc=0
+is used, the free inode btree feature is not supported and is disabled.
 .RE
 .TP
 .BI \-d " data_section_options"
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 5084d75..0218491 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -1004,6 +1004,7 @@ main(
 	int			lazy_sb_counters;
 	int			crcs_enabled;
 	int			finobt;
+	bool			finobtflag;
 
 	progname = basename(argv[0]);
 	setlocale(LC_ALL, "");
@@ -1036,8 +1037,9 @@ main(
 	force_overwrite = 0;
 	worst_freelist = 0;
 	lazy_sb_counters = 1;
-	crcs_enabled = 0;
-	finobt = 0;
+	crcs_enabled = 1;
+	finobt = 1;
+	finobtflag = false;
 	memset(&fsx, 0, sizeof(fsx));
 
 	memset(&xi, 0, sizeof(xi));
@@ -1538,6 +1540,7 @@ _("cannot specify both crc and ftype\n"));
 					if (c < 0 || c > 1)
 						illegal(value, "m finobt");
 					finobt = c;
+					finobtflag = true;
 					break;
 				default:
 					unknown('m', value);
@@ -1878,15 +1881,19 @@ _("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,
+	} else {
+		/*
+		 * The kernel doesn't currently support crc=0,finobt=1
+		 * filesystems. If crcs are not enabled and the user has
+		 * explicitly turned them off then silently turn them off
+		 * to avoid an unnecessary warning. If the user explicitly
+		 * tried to use crc=0,finobt=1, then issue a warning before
+		 * turning them off.
+		 */
+		if (finobt && finobtflag) {
+			fprintf(stderr,
 _("warning: finobt not supported without CRC support, disabled.\n"));
+		}
 		finobt = 0;
 	}
 
-- 
2.0.0

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

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

[Brian Foster]

On Mon, May 04, 2015 at 01:52:28PM +1000, 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 functionality has also had long enough to make
> it into distro kernels, too. Also update the man page to reflect the
> change in defaults.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---

Fine by me:

Reviewed-by: Brian Foster <bfoster@redhat.com>

I noticed we do have a bit of poor usability with regard to setting
options that are invalid with crc=1 now. For example:

# ./mkfs/mkfs.xfs -f /dev/test/scratch -b size=512
illegal inode size 512
allowable inode size with 512 byte blocks is 256
# ./mkfs/mkfs.xfs -f /dev/test/scratch -b size=512 -i size=256
Minimum inode size for CRCs is 512 bytes
Usage: mkfs.xfs
...

*shakes fist* ;)

# ./mkfs/mkfs.xfs -f /dev/test/scratch -b size=512 -i size=256 -m crc=0
meta-data=/dev/test/scratch      isize=256    agcount=4, agsize=10485760
blks
...

It might be nice to clean that up one way or another, depending on how
ugly it might be to fix...

Brian

>  man/man8/mkfs.xfs.8 |  9 +++++----
>  mkfs/xfs_mkfs.c     | 27 +++++++++++++++++----------
>  2 files changed, 22 insertions(+), 14 deletions(-)
> 
> diff --git a/man/man8/mkfs.xfs.8 b/man/man8/mkfs.xfs.8
> index ad9ff3d..e18f6d1 100644
> --- a/man/man8/mkfs.xfs.8
> +++ b/man/man8/mkfs.xfs.8
> @@ -150,7 +150,7 @@ of calculating and checking the CRCs is not noticable in normal operation.
>  .IP
>  By default,
>  .B mkfs.xfs
> -will not enable metadata CRCs.
> +will enable metadata CRCs.
>  .TP
>  .BI finobt= value
>  This option enables the use of a separate free inode btree index in each
> @@ -164,10 +164,11 @@ filesystems age.
>  .IP
>  By default,
>  .B mkfs.xfs
> -will not create free inode btrees. This feature is also currently only available
> -for filesystems created with the
> +will create free inode btrees for filesystems created with the (default)
>  .B \-m crc=1
> -option set.
> +option set. When the option
> +.B \-m crc=0
> +is used, the free inode btree feature is not supported and is disabled.
>  .RE
>  .TP
>  .BI \-d " data_section_options"
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 5084d75..0218491 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -1004,6 +1004,7 @@ main(
>  	int			lazy_sb_counters;
>  	int			crcs_enabled;
>  	int			finobt;
> +	bool			finobtflag;
>  
>  	progname = basename(argv[0]);
>  	setlocale(LC_ALL, "");
> @@ -1036,8 +1037,9 @@ main(
>  	force_overwrite = 0;
>  	worst_freelist = 0;
>  	lazy_sb_counters = 1;
> -	crcs_enabled = 0;
> -	finobt = 0;
> +	crcs_enabled = 1;
> +	finobt = 1;
> +	finobtflag = false;
>  	memset(&fsx, 0, sizeof(fsx));
>  
>  	memset(&xi, 0, sizeof(xi));
> @@ -1538,6 +1540,7 @@ _("cannot specify both crc and ftype\n"));
>  					if (c < 0 || c > 1)
>  						illegal(value, "m finobt");
>  					finobt = c;
> +					finobtflag = true;
>  					break;
>  				default:
>  					unknown('m', value);
> @@ -1878,15 +1881,19 @@ _("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,
> +	} else {
> +		/*
> +		 * The kernel doesn't currently support crc=0,finobt=1
> +		 * filesystems. If crcs are not enabled and the user has
> +		 * explicitly turned them off then silently turn them off
> +		 * to avoid an unnecessary warning. If the user explicitly
> +		 * tried to use crc=0,finobt=1, then issue a warning before
> +		 * turning them off.
> +		 */
> +		if (finobt && finobtflag) {
> +			fprintf(stderr,
>  _("warning: finobt not supported without CRC support, disabled.\n"));
> +		}
>  		finobt = 0;
>  	}
>  
> -- 
> 2.0.0
> 
> _______________________________________________
> 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

[PATCH] mkfs: inode/block size error messages confusing

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

As reported by Brain:

# ./mkfs/mkfs.xfs -f /dev/test/scratch -b size=512
illegal inode size 512
allowable inode size with 512 byte blocks is 256
# ./mkfs/mkfs.xfs -f /dev/test/scratch -b size=512 -i size=256
Minimum inode size for CRCs is 512 bytes
Usage: mkfs.xfs
...

Fix mkfs to catch the block size as being too small, rather than
leaving it for inode size detection to enforce.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 include/xfs_types.h | 3 +++
 mkfs/xfs_mkfs.c     | 6 ++++++
 2 files changed, 9 insertions(+)

diff --git a/include/xfs_types.h b/include/xfs_types.h
index 65c6e66..accb95d 100644
--- a/include/xfs_types.h
+++ b/include/xfs_types.h
@@ -100,11 +100,14 @@ typedef __uint64_t	xfs_filblks_t;	/* number of blocks in a file */
  * Minimum and maximum blocksize and sectorsize.
  * The blocksize upper limit is pretty much arbitrary.
  * The sectorsize upper limit is due to sizeof(sb_sectsize).
+ * CRC enable filesystems use 512 byte inodes, meaning 512 byte block sizes
+ * cannot be used.
  */
 #define XFS_MIN_BLOCKSIZE_LOG	9	/* i.e. 512 bytes */
 #define XFS_MAX_BLOCKSIZE_LOG	16	/* i.e. 65536 bytes */
 #define XFS_MIN_BLOCKSIZE	(1 << XFS_MIN_BLOCKSIZE_LOG)
 #define XFS_MAX_BLOCKSIZE	(1 << XFS_MAX_BLOCKSIZE_LOG)
+#define XFS_MIN_CRC_BLOCKSIZE	(1 << (XFS_MIN_BLOCKSIZE_LOG + 1))
 #define XFS_MIN_SECTORSIZE_LOG	9	/* i.e. 512 bytes */
 #define XFS_MAX_SECTORSIZE_LOG	15	/* i.e. 32768 bytes */
 #define XFS_MIN_SECTORSIZE	(1 << XFS_MIN_SECTORSIZE_LOG)
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 0218491..e2a052d 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -1758,6 +1758,12 @@ _("cannot specify both crc and ftype\n"));
 		fprintf(stderr, _("illegal block size %d\n"), blocksize);
 		usage();
 	}
+	if (crcs_enabled && blocksize < XFS_MIN_CRC_BLOCKSIZE) {
+		fprintf(stderr,
+_("Minimum block size for CRC enabled filesystems is %d bytes.\n"),
+			XFS_MIN_CRC_BLOCKSIZE);
+		usage();
+	}
 
 	memset(&ft, 0, sizeof(ft));
 	get_topology(&xi, &ft, force_overwrite);

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

Re: [PATCH] mkfs: inode/block size error messages confusing

[Brian Foster]

On Wed, May 06, 2015 at 08:48:46AM +1000, Dave Chinner wrote:
> 
> From: Dave Chinner <dchinner@redhat.com>
> 
> As reported by Brain:
> 
> # ./mkfs/mkfs.xfs -f /dev/test/scratch -b size=512
> illegal inode size 512
> allowable inode size with 512 byte blocks is 256
> # ./mkfs/mkfs.xfs -f /dev/test/scratch -b size=512 -i size=256
> Minimum inode size for CRCs is 512 bytes
> Usage: mkfs.xfs
> ...
> 
> Fix mkfs to catch the block size as being too small, rather than
> leaving it for inode size detection to enforce.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  include/xfs_types.h | 3 +++
>  mkfs/xfs_mkfs.c     | 6 ++++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/include/xfs_types.h b/include/xfs_types.h
> index 65c6e66..accb95d 100644
> --- a/include/xfs_types.h
> +++ b/include/xfs_types.h
> @@ -100,11 +100,14 @@ typedef __uint64_t	xfs_filblks_t;	/* number of blocks in a file */
>   * Minimum and maximum blocksize and sectorsize.
>   * The blocksize upper limit is pretty much arbitrary.
>   * The sectorsize upper limit is due to sizeof(sb_sectsize).
> + * CRC enable filesystems use 512 byte inodes, meaning 512 byte block sizes
> + * cannot be used.
>   */
>  #define XFS_MIN_BLOCKSIZE_LOG	9	/* i.e. 512 bytes */
>  #define XFS_MAX_BLOCKSIZE_LOG	16	/* i.e. 65536 bytes */
>  #define XFS_MIN_BLOCKSIZE	(1 << XFS_MIN_BLOCKSIZE_LOG)
>  #define XFS_MAX_BLOCKSIZE	(1 << XFS_MAX_BLOCKSIZE_LOG)
> +#define XFS_MIN_CRC_BLOCKSIZE	(1 << (XFS_MIN_BLOCKSIZE_LOG + 1))
>  #define XFS_MIN_SECTORSIZE_LOG	9	/* i.e. 512 bytes */
>  #define XFS_MAX_SECTORSIZE_LOG	15	/* i.e. 32768 bytes */
>  #define XFS_MIN_SECTORSIZE	(1 << XFS_MIN_SECTORSIZE_LOG)
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 0218491..e2a052d 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -1758,6 +1758,12 @@ _("cannot specify both crc and ftype\n"));
>  		fprintf(stderr, _("illegal block size %d\n"), blocksize);
>  		usage();
>  	}
> +	if (crcs_enabled && blocksize < XFS_MIN_CRC_BLOCKSIZE) {
> +		fprintf(stderr,
> +_("Minimum block size for CRC enabled filesystems is %d bytes.\n"),
> +			XFS_MIN_CRC_BLOCKSIZE);
> +		usage();
> +	}

I'd move this down by the crc inode size check a page or so down since
we already have a crcs_enabled block there (the error messages could
also be made more uniform I suppose). Otherwise looks good, thanks!

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  
>  	memset(&ft, 0, sizeof(ft));
>  	get_topology(&xi, &ft, force_overwrite);
> 
> _______________________________________________
> 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: inode/block size error messages confusing

[Dave Chinner]

On Wed, May 06, 2015 at 07:36:15AM -0400, Brian Foster wrote:
> On Wed, May 06, 2015 at 08:48:46AM +1000, Dave Chinner wrote:
> > 
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > As reported by Brain:
> > 
> > # ./mkfs/mkfs.xfs -f /dev/test/scratch -b size=512
> > illegal inode size 512
> > allowable inode size with 512 byte blocks is 256
> > # ./mkfs/mkfs.xfs -f /dev/test/scratch -b size=512 -i size=256
> > Minimum inode size for CRCs is 512 bytes
> > Usage: mkfs.xfs
> > ...
> > 
> > Fix mkfs to catch the block size as being too small, rather than
> > leaving it for inode size detection to enforce.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
....
> > +	if (crcs_enabled && blocksize < XFS_MIN_CRC_BLOCKSIZE) {
> > +		fprintf(stderr,
> > +_("Minimum block size for CRC enabled filesystems is %d bytes.\n"),
> > +			XFS_MIN_CRC_BLOCKSIZE);
> > +		usage();
> > +	}
> 
> I'd move this down by the crc inode size check a page or so down since
> we already have a crcs_enabled block there (the error messages could
> also be made more uniform I suppose). Otherwise looks good, thanks!

Heh, I put it there to be with the other block size validity checks,
and I made the error message consistent with the inode size error
message:

$ mkfs.xfs -N -i size=256 /dev/ram1
Minimum inode size for CRCs is 512 bytes
Usage: mkfs.xfs
....

I can move it about and change it, but swings and roundabouts,
really ;)

> Reviewed-by: Brian Foster <bfoster@redhat.com>

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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