Re: [PATCH 12/12 V2] mkfs: enable reflink by default

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Tue, May 21, 2019 at 02:30:57PM -0500, Eric Sandeen wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Data block sharing (a.k.a. reflink) has been stable for a while, so turn
> it on by default.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> [sandeen: update comments & man page]
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Looks good to me.
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

Hmm I'm not allowed to RVB my own patch, right?

--D

> ---
> 
> diff --git a/man/man8/mkfs.xfs.8 b/man/man8/mkfs.xfs.8
> index 4b8c78c..78b1501 100644
> --- a/man/man8/mkfs.xfs.8
> +++ b/man/man8/mkfs.xfs.8
> @@ -231,7 +231,7 @@ available for the data forks of regular files.
>  .IP
>  By default,
>  .B mkfs.xfs
> -will not create reference count btrees and therefore will not enable the
> +will create reference count btrees and therefore will enable the
>  reflink feature.  This feature is only available for filesystems created with
>  the (default)
>  .B \-m crc=1
> @@ -239,6 +239,13 @@ option set. When the option
>  .B \-m crc=0
>  is used, the reference count btree feature is not supported and reflink is
>  disabled.
> +.IP
> +Note: the filesystem DAX mount option (
> +.B \-o dax
> +) is incompatible with
> +reflink-enabled XFS filesystems.  To use filesystem DAX with XFS, specify the
> +.B \-m reflink=0
> +option to mkfs.xfs to disable the reflink feature.
>  .RE
>  .TP
>  .BI \-d " data_section_options"
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 0910664..ddb25ec 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -1973,15 +1973,15 @@ _("Directory ftype field always enabled on CRC enabled filesystems\n"));
>  			usage();
>  		}
>  
> -	} else {
> +	} else {	/* !crcs_enabled */
>  		/*
> -		 * The kernel doesn't currently support crc=0,finobt=1
> -		 * filesystems. If crcs are not enabled and the user has not
> -		 * explicitly turned finobt on, then silently turn it off to
> -		 * avoid an unnecessary warning.
> +		 * The kernel doesn't support crc=0,finobt=1 filesystems.
> +		 * If crcs are not enabled and the user has not explicitly
> +		 * turned finobt on, then silently turn it off to avoid an
> +		 * unnecessary warning.
>  		 * If the user explicitly tried to use crc=0,finobt=1,
>  		 * then issue an error.
> -		 * The same is also for sparse inodes.
> +		 * The same is also true for sparse inodes and reflink.
>  		 */
>  		if (cli->sb_feat.finobt && cli_opt_set(&mopts, M_FINOBT)) {
>  			fprintf(stderr,
> @@ -2004,7 +2004,7 @@ _("rmapbt not supported without CRC support\n"));
>  		}
>  		cli->sb_feat.rmapbt = false;
>  
> -		if (cli->sb_feat.reflink) {
> +		if (cli->sb_feat.reflink && cli_opt_set(&mopts, M_REFLINK)) {
>  			fprintf(stderr,
>  _("reflink not supported without CRC support\n"));
>  			usage();
> @@ -3876,7 +3876,7 @@ main(
>  			.finobt = true,
>  			.spinodes = true,
>  			.rmapbt = false,
> -			.reflink = false,
> +			.reflink = true,
>  			.parent_pointers = false,
>  			.nodalign = false,
>  			.nortalign = false,
> 
>