[PATCH 0/6] xfsprogs: blockdev dax detection and warnings

[PATCH 0/6] xfsprogs: blockdev dax detection and warnings

[Anthony Iliopoulos]

Hi all,

This short series adds blockdev dax capability detection via libblkid,
and subsequently uses this bit to warn on a couple of incompatible
configurations during mkfs.

The first patch adds the detection capability to libtopology, and the
following two patches add mkfs warnings that are issued when the fs
block size is not matching the page size, and when reflink is being
enabled in conjunction with dax.

The next patch adds a new cli option that can be used to override
warnings, and converts all warnings that can be forced to this option
instead the overloaded -f option. This avoids cases where forcing a
warning may also be implicitly forcing overwriting an existing
partition.

The last two patches are small cleanups that remove redundant code.

Anthony Iliopoulos (6):
  libfrog: add dax capability detection in topology probing
  mkfs: warn if blocksize doesn't match pagesize on dax devices
  mkfs: warn if reflink option is enabled on dax-capable devices
  mkfs: introduce -x option to force incompat config combinations
  mkfs: remove redundant assignment of cli sb options on failure
  mkfs: remove a couple of unused function parameters

 include/builddefs.in |  1 +
 libfrog/Makefile     |  4 ++++
 libfrog/topology.c   | 21 +++++++++++------
 libfrog/topology.h   |  1 +
 m4/package_blkid.m4  |  5 ++++
 man/man8/mkfs.xfs.8  |  6 +++++
 mkfs/xfs_mkfs.c      | 55 +++++++++++++++++++++++++++++---------------
 7 files changed, 68 insertions(+), 25 deletions(-)

--
2.28.0

[PATCH 1/6] libfrog: add dax capability detection in topology probing

[Anthony Iliopoulos]

Detect support for blkid_topology_get_dax in libblkid which was
introduced in util-linux v2.36, and use it to obtain if the underlying
block device is dax-capable. This can be used to issue warnings for
incompatible configurations during mkfs.

Signed-off-by: Anthony Iliopoulos <ailiop@suse.com>
---
 include/builddefs.in |  1 +
 libfrog/Makefile     |  4 ++++
 libfrog/topology.c   | 11 +++++++++--
 libfrog/topology.h   |  1 +
 m4/package_blkid.m4  |  5 +++++
 5 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/include/builddefs.in b/include/builddefs.in
index 30b2727a8db4..88ecf24a74a7 100644
--- a/include/builddefs.in
+++ b/include/builddefs.in
@@ -140,6 +140,7 @@ PCFLAGS+= -DHAVE_FSETXATTR
 endif
 ifeq ($(ENABLE_BLKID),yes)
 PCFLAGS+= -DENABLE_BLKID
+HAVE_BLKID_DAX = @have_blkid_dax@
 endif
 ifeq ($(NEED_INTERNAL_FSXATTR),yes)
 PCFLAGS+= -DOVERRIDE_SYSTEM_FSXATTR
diff --git a/libfrog/Makefile b/libfrog/Makefile
index 395ce30804b7..bb680b6822ed 100644
--- a/libfrog/Makefile
+++ b/libfrog/Makefile
@@ -56,6 +56,10 @@ ifeq ($(HAVE_GETMNTENT),yes)
 LCFLAGS += -DHAVE_GETMNTENT
 endif
 
+ifeq ($(HAVE_BLKID_DAX),yes)
+LCFLAGS += -DHAVE_BLKID_DAX
+endif
+
 LDIRT = gen_crc32table crc32table.h crc32selftest
 
 default: crc32selftest ltdepend $(LTLIBRARY)
diff --git a/libfrog/topology.c b/libfrog/topology.c
index b1b470c9b6d3..713358b01b4c 100644
--- a/libfrog/topology.c
+++ b/libfrog/topology.c
@@ -180,6 +180,7 @@ static void blkid_get_topology(
 	int		*swidth,
 	int		*lsectorsize,
 	int		*psectorsize,
+	int		*dax,
 	int		force_overwrite)
 {
 
@@ -212,6 +213,10 @@ static void blkid_get_topology(
 	*sunit = val;
 	val = blkid_topology_get_optimal_io_size(tp);
 	*swidth = val;
+#if defined(HAVE_BLKID_DAX)
+	val = blkid_topology_get_dax(tp);
+	*dax = val;
+#endif
 
 	/*
 	 * If the reported values are the same as the physical sector size
@@ -275,6 +280,7 @@ static void blkid_get_topology(
 	int		*swidth,
 	int		*lsectorsize,
 	int		*psectorsize,
+	int		*dax,
 	int		force_overwrite)
 {
 	/*
@@ -320,13 +326,14 @@ void get_topology(
 	} else {
 		blkid_get_topology(dfile, &ft->dsunit, &ft->dswidth,
 				   &ft->lsectorsize, &ft->psectorsize,
-				   force_overwrite);
+				   &ft->dax, force_overwrite);
 	}
 
 	if (xi->rtname && !xi->risfile) {
 		int sunit, lsectorsize, psectorsize;
 
 		blkid_get_topology(xi->rtname, &sunit, &ft->rtswidth,
-				   &lsectorsize, &psectorsize, force_overwrite);
+				   &lsectorsize, &psectorsize, &ft->dax,
+				   force_overwrite);
 	}
 }
diff --git a/libfrog/topology.h b/libfrog/topology.h
index 6fde868a5923..cde8ff282287 100644
--- a/libfrog/topology.h
+++ b/libfrog/topology.h
@@ -16,6 +16,7 @@ typedef struct fs_topology {
 	int	rtswidth;	/* stripe width - rt subvolume */
 	int	lsectorsize;	/* logical sector size &*/
 	int	psectorsize;	/* physical sector size */
+	int	dax;		/* dax support */
 } fs_topology_t;
 
 extern void
diff --git a/m4/package_blkid.m4 b/m4/package_blkid.m4
index 9510dced59c1..db7595237120 100644
--- a/m4/package_blkid.m4
+++ b/m4/package_blkid.m4
@@ -14,5 +14,10 @@ AC_DEFUN([AC_HAVE_BLKID_TOPO],
     echo 'Install the Block device ID development package.'
     exit 1
   fi
+  AC_CHECK_FUNCS(blkid_topology_get_dax)
+  if test $ac_cv_func_blkid_topology_get_dax = yes; then
+	have_blkid_dax=yes
+	AC_SUBST(have_blkid_dax)
+  fi
   AC_SUBST(libblkid)
 ])
-- 
2.28.0

[PATCH 2/6] mkfs: warn if blocksize doesn't match pagesize on dax devices

[Anthony Iliopoulos]

Block devices that are dax-capable require that the blocksize matches
the platform page size in order to enable dax. Warn if this is not the
case during mkfs, to make it clear that the filesystem will not be
mountable with the dax option enabled.

Make the option overridable so that incompatible filesystem configs can
still be created, e.g. for testing or for cases where the filesystem is
not intended to be mounted with the dax option switched on.

Signed-off-by: Anthony Iliopoulos <ailiop@suse.com>
---
 mkfs/xfs_mkfs.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 2e6cd280e388..4fe0bbdcc40c 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -1693,8 +1693,7 @@ validate_sectorsize(
 	struct mkfs_default_params *dft,
 	struct fs_topology	*ft,
 	char			*dfile,
-	int			dry_run,
-	int			force_overwrite)
+	int			dry_run)
 {
 	/*
 	 * Before anything else, verify that we are correctly operating on
@@ -1719,9 +1718,6 @@ validate_sectorsize(
 	if (cli->xi->disfile || cli->xi->lisfile || cli->xi->risfile)
 		cli->xi->isdirect = 0;
 
-	memset(ft, 0, sizeof(*ft));
-	get_topology(cli->xi, ft, force_overwrite);
-
 	/* set configured sector sizes in preparation for checks */
 	if (!cli->sectorsize) {
 		/*
@@ -1781,7 +1777,9 @@ static void
 validate_blocksize(
 	struct mkfs_params	*cfg,
 	struct cli_params	*cli,
-	struct mkfs_default_params *dft)
+	struct mkfs_default_params *dft,
+	struct fs_topology	*ft,
+	int			force)
 {
 	/*
 	 * Blocksize and sectorsize first, other things depend on them
@@ -1809,6 +1807,16 @@ _("Minimum block size for CRC enabled filesystems is %d bytes.\n"),
 		usage();
 	}
 
+	if (ft->dax && cfg->blocksize != getpagesize()) {
+		fprintf(stderr,
+_("warning: block size should match platform page size on dax devices.\n"));
+
+		if (!force) {
+			fprintf(stderr,
+				_("Use -f to force usage of block size\n"));
+			usage();
+		}
+	}
 }
 
 /*
@@ -3694,9 +3702,9 @@ main(
 	 * Extract as much of the valid config as we can from the CLI input
 	 * before opening the libxfs devices.
 	 */
-	validate_blocksize(&cfg, &cli, &dft);
-	validate_sectorsize(&cfg, &cli, &dft, &ft, dfile, dry_run,
-			    force_overwrite);
+	get_topology(cli.xi, &ft, force_overwrite);
+	validate_blocksize(&cfg, &cli, &dft, &ft, force_overwrite);
+	validate_sectorsize(&cfg, &cli, &dft, &ft, dfile, dry_run);
 
 	/*
 	 * XXX: we still need to set block size and sector size global variables
-- 
2.28.0

[PATCH 3/6] mkfs: warn if reflink option is enabled on dax-capable devices

[Anthony Iliopoulos]

Reflink is currently not supported in conjunction with dax. Warn if this
config is enabled during mkfs to make it clear that the filesystem will
not be mountable with reflinks and dax enabled.

Make the option overridable so that incompatible fs configurations can
still be created, e.g. for testing or for cases where the filesystem is
not intended to be mounted with the dax option switched on.

Signed-off-by: Anthony Iliopoulos <ailiop@suse.com>
---
 mkfs/xfs_mkfs.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 4fe0bbdcc40c..b8739c2b9093 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -1883,7 +1883,9 @@ _("log stripe unit specified, using v2 logs\n"));
 static void
 validate_sb_features(
 	struct mkfs_params	*cfg,
-	struct cli_params	*cli)
+	struct cli_params	*cli,
+	struct fs_topology	*ft,
+	int			force)
 {
 	/*
 	 * Now we have blocks and sector sizes set up, check parameters that are
@@ -2003,6 +2005,16 @@ _("rmapbt not supported with realtime devices\n"));
 		cli->sb_feat.rmapbt = false;
 	}
 
+	if (cli->sb_feat.reflink && ft->dax) {
+		fprintf(stderr, _("reflink not supported with dax devices\n"));
+
+		if (!force) {
+			fprintf(stderr,
+				_("Use -f to force enabling reflink\n"));
+			usage();
+		}
+	}
+
 	/*
 	 * Copy features across to config structure now.
 	 */
@@ -3714,7 +3726,7 @@ main(
 	sectorsize = cfg.sectorsize;
 
 	validate_log_sectorsize(&cfg, &cli, &dft);
-	validate_sb_features(&cfg, &cli);
+	validate_sb_features(&cfg, &cli, &ft, force_overwrite);
 
 	/*
 	 * we've now completed basic validation of the features, sector and
-- 
2.28.0

[PATCH 4/6] mkfs: introduce -y option to force incompat config combinations

[Anthony Iliopoulos]

Introduce a new command line option that can be used to override bailing
out on combinations of incompatible configurations and other warnings.

The -f option is currently overloaded to include these semantics, but it
conceals cases where there is an fs signature detected and confirmation
is required to overwrite the filesystem.

Signed-off-by: Anthony Iliopoulos <ailiop@suse.com>
---
 libfrog/topology.c  | 14 +++++++-------
 man/man8/mkfs.xfs.8 |  6 ++++++
 mkfs/xfs_mkfs.c     | 16 ++++++++++------
 3 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/libfrog/topology.c b/libfrog/topology.c
index 713358b01b4c..ad317fe4e287 100644
--- a/libfrog/topology.c
+++ b/libfrog/topology.c
@@ -181,7 +181,7 @@ static void blkid_get_topology(
 	int		*lsectorsize,
 	int		*psectorsize,
 	int		*dax,
-	int		force_overwrite)
+	int		force)
 {
 
 	blkid_topology tp;
@@ -240,9 +240,9 @@ static void blkid_get_topology(
 			_("warning: device is not properly aligned %s\n"),
 			device);
 
-		if (!force_overwrite) {
+		if (!force) {
 			fprintf(stderr,
-				_("Use -f to force usage of a misaligned device\n"));
+				_("Use -y to force usage of a misaligned device\n"));
 
 			exit(EXIT_FAILURE);
 		}
@@ -281,7 +281,7 @@ static void blkid_get_topology(
 	int		*lsectorsize,
 	int		*psectorsize,
 	int		*dax,
-	int		force_overwrite)
+	int		force)
 {
 	/*
 	 * Shouldn't make any difference (no blkid = no block device access),
@@ -296,7 +296,7 @@ static void blkid_get_topology(
 void get_topology(
 	libxfs_init_t		*xi,
 	struct fs_topology	*ft,
-	int			force_overwrite)
+	int			force)
 {
 	struct stat statbuf;
 	char *dfile = xi->volname ? xi->volname : xi->dname;
@@ -326,7 +326,7 @@ void get_topology(
 	} else {
 		blkid_get_topology(dfile, &ft->dsunit, &ft->dswidth,
 				   &ft->lsectorsize, &ft->psectorsize,
-				   &ft->dax, force_overwrite);
+				   &ft->dax, force);
 	}
 
 	if (xi->rtname && !xi->risfile) {
@@ -334,6 +334,6 @@ void get_topology(
 
 		blkid_get_topology(xi->rtname, &sunit, &ft->rtswidth,
 				   &lsectorsize, &psectorsize, &ft->dax,
-				   force_overwrite);
+				   force);
 	}
 }
diff --git a/man/man8/mkfs.xfs.8 b/man/man8/mkfs.xfs.8
index 9d762a43011a..d84db416db1b 100644
--- a/man/man8/mkfs.xfs.8
+++ b/man/man8/mkfs.xfs.8
@@ -41,6 +41,8 @@ mkfs.xfs \- construct an XFS filesystem
 .B \-N
 ] [
 .B \-K
+] [
+.B \-y
 ]
 .I device
 .br
@@ -919,6 +921,10 @@ Do not attempt to discard blocks at mkfs time.
 .TP
 .B \-V
 Prints the version number and exits.
+.TP
+.B \-y
+Override warnings and force usage of incompatible features.
+.TP
 .SH SEE ALSO
 .BR xfs (5),
 .BR mkfs (8),
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index b8739c2b9093..75e910dd4a30 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -1813,7 +1813,7 @@ _("warning: block size should match platform page size on dax devices.\n"));
 
 		if (!force) {
 			fprintf(stderr,
-				_("Use -f to force usage of block size\n"));
+				_("Use -y to force usage of block size\n"));
 			usage();
 		}
 	}
@@ -2010,7 +2010,7 @@ _("rmapbt not supported with realtime devices\n"));
 
 		if (!force) {
 			fprintf(stderr,
-				_("Use -f to force enabling reflink\n"));
+				_("Use -y to force enabling reflink\n"));
 			usage();
 		}
 	}
@@ -3586,6 +3586,7 @@ main(
 	int			dry_run = 0;
 	int			discard = 1;
 	int			force_overwrite = 0;
+	int			force = 0;
 	int			quiet = 0;
 	char			*protofile = NULL;
 	char			*protostring = NULL;
@@ -3658,7 +3659,7 @@ main(
 	memcpy(&cli.sb_feat, &dft.sb_feat, sizeof(cli.sb_feat));
 	memcpy(&cli.fsx, &dft.fsx, sizeof(cli.fsx));
 
-	while ((c = getopt(argc, argv, "b:d:i:l:L:m:n:KNp:qr:s:CfV")) != EOF) {
+	while ((c = getopt(argc, argv, "b:d:i:l:L:m:n:KNp:qr:s:CfVy")) != EOF) {
 		switch (c) {
 		case 'C':
 		case 'f':
@@ -3696,6 +3697,9 @@ main(
 		case 'V':
 			printf(_("%s version %s\n"), progname, VERSION);
 			exit(0);
+		case 'y':
+			force = 1;
+			break;
 		default:
 			unknown(optopt, "");
 		}
@@ -3714,8 +3718,8 @@ main(
 	 * Extract as much of the valid config as we can from the CLI input
 	 * before opening the libxfs devices.
 	 */
-	get_topology(cli.xi, &ft, force_overwrite);
-	validate_blocksize(&cfg, &cli, &dft, &ft, force_overwrite);
+	get_topology(cli.xi, &ft, force);
+	validate_blocksize(&cfg, &cli, &dft, &ft, force);
 	validate_sectorsize(&cfg, &cli, &dft, &ft, dfile, dry_run);
 
 	/*
@@ -3726,7 +3730,7 @@ main(
 	sectorsize = cfg.sectorsize;
 
 	validate_log_sectorsize(&cfg, &cli, &dft);
-	validate_sb_features(&cfg, &cli, &ft, force_overwrite);
+	validate_sb_features(&cfg, &cli, &ft, force);
 
 	/*
 	 * we've now completed basic validation of the features, sector and
-- 
2.28.0

[PATCH 5/6] mkfs: remove redundant assignment of cli sb options on failure

[Anthony Iliopoulos]

rmapbt and reflink are incompatible with realtime devices and mkfs bails
out on such configurations.  Switching them off in the cli params after
exit is dead code, remove it.

Signed-off-by: Anthony Iliopoulos <ailiop@suse.com>
---
 mkfs/xfs_mkfs.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 75e910dd4a30..1f142f78e677 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -1995,14 +1995,12 @@ _("cowextsize not supported without reflink support\n"));
 		fprintf(stderr,
 _("reflink not supported with realtime devices\n"));
 		usage();
-		cli->sb_feat.reflink = false;
 	}
 
 	if (cli->sb_feat.rmapbt && cli->xi->rtname) {
 		fprintf(stderr,
 _("rmapbt not supported with realtime devices\n"));
 		usage();
-		cli->sb_feat.rmapbt = false;
 	}
 
 	if (cli->sb_feat.reflink && ft->dax) {
-- 
2.28.0

[PATCH 6/6] mkfs: remove a couple of unused function parameters

[Anthony Iliopoulos]

initialise_mount does not use mkfs_params, and initialise_ag_headers
does not use the xfs_sb param, remove them.

Signed-off-by: Anthony Iliopoulos <ailiop@suse.com>
---
 mkfs/xfs_mkfs.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 1f142f78e677..03bbe3b4697d 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -3243,7 +3243,6 @@ start_superblock_setup(
 
 static void
 initialise_mount(
-	struct mkfs_params	*cfg,
 	struct xfs_mount	*mp,
 	struct xfs_sb		*sbp)
 {
@@ -3431,7 +3430,6 @@ static void
 initialise_ag_headers(
 	struct mkfs_params	*cfg,
 	struct xfs_mount	*mp,
-	struct xfs_sb		*sbp,
 	xfs_agnumber_t		agno,
 	int			*worst_freelist,
 	struct list_head	*buffer_list)
@@ -3776,7 +3774,7 @@ main(
 	 * provided functions to determine on-disk format information.
 	 */
 	start_superblock_setup(&cfg, mp, sbp);
-	initialise_mount(&cfg, mp, sbp);
+	initialise_mount(mp, sbp);
 
 	/*
 	 * With the mount set up, we can finally calculate the log size
@@ -3829,7 +3827,7 @@ main(
 	 */
 	INIT_LIST_HEAD(&buffer_list);
 	for (agno = 0; agno < cfg.agcount; agno++) {
-		initialise_ag_headers(&cfg, mp, sbp, agno, &worst_freelist,
+		initialise_ag_headers(&cfg, mp, agno, &worst_freelist,
 				&buffer_list);
 
 		if (agno % 16)
-- 
2.28.0

Re: [PATCH 0/6] xfsprogs: blockdev dax detection and warnings

[Dave Chinner]

On Mon, Aug 24, 2020 at 10:37:18PM +0200, Anthony Iliopoulos wrote:
> Hi all,
> 
> This short series adds blockdev dax capability detection via libblkid,
> and subsequently uses this bit to warn on a couple of incompatible
> configurations during mkfs.
> 
> The first patch adds the detection capability to libtopology, and the
> following two patches add mkfs warnings that are issued when the fs
> block size is not matching the page size, and when reflink is being
> enabled in conjunction with dax.

This makes the assumption that anyone running mkfs on a dax capable
device is going to use DAX, and prevents mkfs from running if the
config is not DAX compatible.

The issue here is that making a filesystem that is not DAX
compatible on a DAX capable device is not a fatal error. The
filesystem will work just fine using buffered and direct IO, and
there are definitely workloads where we want to use buffered IO on
pmem and not DAX. Why? Because existing pmem is terribly slow for
write intensive applications compared to page cache based mmap().
And even buffered writes are faster than DAX direct writes because
the slow writeback is done in the background via async writeback.

Also, what happens if you have a 64kB page size? mkfs defaults to
4kB block size, so with these changes mkfs will refuse to run on a
dax capable device unless the user specifically directs it to do
something different. That's not a good behaviour for the default
config to have....

Hence I don't think that preventing mkfs from running unless the config
is exactly waht DAX requires or the "force" option is set is the
right policy here.

I agree that mkfs needs to be aware of DAX capability of the block
device, but that capability existing should not cause mkfs to fail.
If we want users to be able to direct mkfs to to create a DAX
capable filesystem then adding a -d dax option would be a better
idea. This would direct mkfs to align/size all the data options to
use a DAX compatible topology if blkid supports reporting the DAX
topology. It would also do things like turn off reflink (until that
is supported w/ DAX), etc.

i.e. if the user knows they are going to use DAX (and they will)
then they can tell mkfs to make a DAX compatible filesystem.

> The next patch adds a new cli option that can be used to override
> warnings, and converts all warnings that can be forced to this option
> instead the overloaded -f option. This avoids cases where forcing a
> warning may also be implicitly forcing overwriting an existing
> partition.

I don't want both an "ignore warnings" and a "force" CLI option.
They both do the same thing - allow the user to override things that
are potentially destructive or result in an unusable config - so why
should we add the complexity of having a different "force" options
for every different possible thing that can be overridden?

Then we'll grow a "--force-all" option to override everything.
i.e.  we end up with this insanity:

$ dpkg --help |grep force
  --force-help                     Show help on forcing.
  --[no-]triggers            Skip or force consequential trigger processing.
  --force-<thing>[,...]      Override problems (see --force-help).
  --no-force-<thing>[,...]   Stop when problems encountered.

and "--force-help" gives you a list of about 30 different things you
can force the behaviour of, including "--force-all". This is simply
not a good UI model...

> The last two patches are small cleanups that remove redundant code.

They should be first.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 0/6] xfsprogs: blockdev dax detection and warnings

[Anthony Iliopoulos]

On Tue, Aug 25, 2020 at 08:55:33AM +1000, Dave Chinner wrote:
> On Mon, Aug 24, 2020 at 10:37:18PM +0200, Anthony Iliopoulos wrote:
> > Hi all,
> > 
> > This short series adds blockdev dax capability detection via libblkid,
> > and subsequently uses this bit to warn on a couple of incompatible
> > configurations during mkfs.
> > 
> > The first patch adds the detection capability to libtopology, and the
> > following two patches add mkfs warnings that are issued when the fs
> > block size is not matching the page size, and when reflink is being
> > enabled in conjunction with dax.
> 
> This makes the assumption that anyone running mkfs on a dax capable
> device is going to use DAX, and prevents mkfs from running if the
> config is not DAX compatible.
> 
> The issue here is that making a filesystem that is not DAX
> compatible on a DAX capable device is not a fatal error. The
> filesystem will work just fine using buffered and direct IO, and
> there are definitely workloads where we want to use buffered IO on
> pmem and not DAX. Why? Because existing pmem is terribly slow for
> write intensive applications compared to page cache based mmap().
> And even buffered writes are faster than DAX direct writes because
> the slow writeback is done in the background via async writeback.
> 
> Also, what happens if you have a 64kB page size? mkfs defaults to
> 4kB block size, so with these changes mkfs will refuse to run on a
> dax capable device unless the user specifically directs it to do
> something different. That's not a good behaviour for the default
> config to have....
> 
> Hence I don't think that preventing mkfs from running unless the config
> is exactly waht DAX requires or the "force" option is set is the
> right policy here.
> 
> I agree that mkfs needs to be aware of DAX capability of the block
> device, but that capability existing should not cause mkfs to fail.
> If we want users to be able to direct mkfs to to create a DAX
> capable filesystem then adding a -d dax option would be a better
> idea. This would direct mkfs to align/size all the data options to
> use a DAX compatible topology if blkid supports reporting the DAX
> topology. It would also do things like turn off reflink (until that
> is supported w/ DAX), etc.

I do like the idea of adding an explicit dax option, but I'm not sure
what the right policy would be:

1. -d dax option specified, set dax-compatible parameters irrespective
   of dax capability (blkid detection not strictly required)

2. -d dax option specified, set dax-compatible parameters only if
   blockdev is dax capable; fallback to default params otherwise

3. autodetect dax capability and automatically set dax-compatible
   params, irrespective of if the option is specified or not
   (potentially also needs an explicit override option e.g. -d dax=0).

I'd be inclined to go with the second option which should lead to the
least amount of surprises for users.

Still, this doesn't address the exact thing I was trying to do (see
below).

> i.e. if the user knows they are going to use DAX (and they will)
> then they can tell mkfs to make a DAX compatible filesystem.

So I've been trying to prevent cases where users create a filesystem
with the default params, go on to populate it, and at some later point
of time find themselves wanting to mount it with dax only to realize
that this is not possible without mkfs (most commonly due to the
mismatch of the 64kB page size on ppc64). We could potentially just
issue the warning and not force mkfs to bail out, but I'm afraid that
warnings aren't very discernible and are easily missed. I do agree
though that requiring an override may not be the best model here.

Would it make sense to simply emit the warnings and drop the bail-out
and override logic altogether?

Alternatively the third option above (autodetection), would take care of
those cases at the expense of overriding otherwise potentially desirable
options (e.g. switching off reflink), which will come as a surprise to
users that don't intent to use dax. I don't think this would be a good
default policy.

> > The next patch adds a new cli option that can be used to override
> > warnings, and converts all warnings that can be forced to this option
> > instead the overloaded -f option. This avoids cases where forcing a
> > warning may also be implicitly forcing overwriting an existing
> > partition.
>
> I don't want both an "ignore warnings" and a "force" CLI option.
> They both do the same thing - allow the user to override things that
> are potentially destructive or result in an unusable config - so why
> should we add the complexity of having a different "force" options
> for every different possible thing that can be overridden?

The rationale here was to only make a distinction between destructive
and (conditionally) unusable, otherwise we would indeed need an override
toggle per warning - which I totally agree is an overkill. If implicitly
suppressing the destructive operation confirmation isn't a concern, then
I'd definitely drop this patch.

Thanks for the feedback!

Anthony

Re: [PATCH 0/6] xfsprogs: blockdev dax detection and warnings

[Eric Sandeen]

On 8/24/20 5:55 PM, Dave Chinner wrote:
> I agree that mkfs needs to be aware of DAX capability of the block
> device, but that capability existing should not cause mkfs to fail.
> If we want users to be able to direct mkfs to to create a DAX
> capable filesystem then adding a -d dax option would be a better
> idea. This would direct mkfs to align/size all the data options to
> use a DAX compatible topology if blkid supports reporting the DAX
> topology. It would also do things like turn off reflink (until that
> is supported w/ DAX), etc.
> 
> i.e. if the user knows they are going to use DAX (and they will)
> then they can tell mkfs to make a DAX compatible filesystem.

FWIW, Darrick /just/ added a -d daxinherit option, though all it does
now is set the inheritable dax flag on the root dir, it doesn't enforce
things like page vs block size, etc.

That change is currently staged in my local tree.

I suppose we could condition that on other requirements, although we've
always had the ability to mkfs a filesystem that can't necessarily be
used on the current machine - i.e. you can make a 64k block size filesystem
on a 4k page machine, etc.  So I'm not sure we want to tie mkfs abilities
to the current mkfs environment....

Still, I wonder if I should hold off on "-d daxinherit" patch until we
have thought through things like reflink conflicts, for now.

(though again, mkfs is "perfectly capapable" of making a consistent
reflink+dax filesystem, it's just that no kernel can mount it today...)

-Eric

Re: [PATCH 0/6] xfsprogs: blockdev dax detection and warnings

[Darrick J. Wong]

On Tue, Aug 25, 2020 at 08:59:39AM -0500, Eric Sandeen wrote:
> On 8/24/20 5:55 PM, Dave Chinner wrote:
> > I agree that mkfs needs to be aware of DAX capability of the block
> > device, but that capability existing should not cause mkfs to fail.
> > If we want users to be able to direct mkfs to to create a DAX
> > capable filesystem then adding a -d dax option would be a better
> > idea. This would direct mkfs to align/size all the data options to
> > use a DAX compatible topology if blkid supports reporting the DAX
> > topology. It would also do things like turn off reflink (until that
> > is supported w/ DAX), etc.
> > 
> > i.e. if the user knows they are going to use DAX (and they will)
> > then they can tell mkfs to make a DAX compatible filesystem.
> 
> FWIW, Darrick /just/ added a -d daxinherit option, though all it does
> now is set the inheritable dax flag on the root dir, it doesn't enforce
> things like page vs block size, etc.
> 
> That change is currently staged in my local tree.
> 
> I suppose we could condition that on other requirements, although we've
> always had the ability to mkfs a filesystem that can't necessarily be
> used on the current machine - i.e. you can make a 64k block size filesystem
> on a 4k page machine, etc.  So I'm not sure we want to tie mkfs abilities
> to the current mkfs environment....
> 
> Still, I wonder if I should hold off on "-d daxinherit" patch until we
> have thought through things like reflink conflicts, for now.
> 
> (though again, mkfs is "perfectly capapable" of making a consistent
> reflink+dax filesystem, it's just that no kernel can mount it today...)

No, please don't layer additional meanings onto daxinherit=1.

I actually /do/ want to have a -d dax=1 option for "set up this
filesystem for DAX" that will configure the geometry for that device to
play nicely with the things that (some) DAX users want.

IOWs, you say "-d dax=1" and that means that mkfs sniffs out the
DAXiness of the underlying device and the PMD size.  Then it turns off
reflink by default, sets the daxinherit=1 hint, and configures the
extent size and su/sw hints to match the PMD size.

Or, you say "-r dax=1" for the realtime device, and now it sets the
allocation unit to the PMD size for people running huge databases and
want only huge pages to back their table data<cough>.

Zooming out a bit, maybe we should instead introduce a new "tuning"
parameter for -d and -r so that administrators could tune the filesystem
for specific purposes:

	-d tune=dax: Reject if device not dax, set daxinherit=1, set
	extsize/su/sw to match PMD

	-d tune=ssd: Set agcount to match the number of CPUs if
	possible, make the log larger to support a large number of
	threads and iops.

	-d tune=rotational: Probably does nothing. ;)

	-d tune=auto: Query blkid to guess which of the above three
	profiles we should use.

	-d tune=none: No tuning.

And then you'd do the same for the realtime device.

This would help us get rid of the seeeekret mkfs wrapper that we use to
make it easier for our internal customers to use DAX since mkfs.xfs
doesn't support config files.

--D

> -Eric

Re: [PATCH 0/6] xfsprogs: blockdev dax detection and warnings

[Anthony Iliopoulos]

On Tue, Aug 25, 2020 at 08:59:39AM -0500, Eric Sandeen wrote:
> On 8/24/20 5:55 PM, Dave Chinner wrote:
> > I agree that mkfs needs to be aware of DAX capability of the block
> > device, but that capability existing should not cause mkfs to fail.
> > If we want users to be able to direct mkfs to to create a DAX
> > capable filesystem then adding a -d dax option would be a better
> > idea. This would direct mkfs to align/size all the data options to
> > use a DAX compatible topology if blkid supports reporting the DAX
> > topology. It would also do things like turn off reflink (until that
> > is supported w/ DAX), etc.
> > 
> > i.e. if the user knows they are going to use DAX (and they will)
> > then they can tell mkfs to make a DAX compatible filesystem.
> 
> FWIW, Darrick /just/ added a -d daxinherit option, though all it does
> now is set the inheritable dax flag on the root dir, it doesn't enforce
> things like page vs block size, etc.

I am aware of that patch, but I considered the option to be somewhat
orthogonal, given that FS_XFLAG_DAX can be set (and inherited)
irrespective of dax support in the block device (and overridden via
mount opts if need be), so I didn't want to overload daxinherit.

> That change is currently staged in my local tree.
> 
> I suppose we could condition that on other requirements, although we've
> always had the ability to mkfs a filesystem that can't necessarily be
> used on the current machine - i.e. you can make a 64k block size filesystem
> on a 4k page machine, etc.  So I'm not sure we want to tie mkfs abilities
> to the current mkfs environment....

Agreed, so I suppose any dax option should be an opt-in, e.g. similar to
the -d dax=1 proposal. That won't prevent users from neglecting it and
creating a fs which will be later incompatible with -o dax, but that's a
different story I guess..

- Anthony

Re: [PATCH 0/6] xfsprogs: blockdev dax detection and warnings

[Eric Sandeen]

On 8/25/20 10:09 AM, Anthony Iliopoulos wrote:
> On Tue, Aug 25, 2020 at 08:59:39AM -0500, Eric Sandeen wrote:
>> On 8/24/20 5:55 PM, Dave Chinner wrote:
>>> I agree that mkfs needs to be aware of DAX capability of the block
>>> device, but that capability existing should not cause mkfs to fail.
>>> If we want users to be able to direct mkfs to to create a DAX
>>> capable filesystem then adding a -d dax option would be a better
>>> idea. This would direct mkfs to align/size all the data options to
>>> use a DAX compatible topology if blkid supports reporting the DAX
>>> topology. It would also do things like turn off reflink (until that
>>> is supported w/ DAX), etc.
>>>
>>> i.e. if the user knows they are going to use DAX (and they will)
>>> then they can tell mkfs to make a DAX compatible filesystem.
>>
>> FWIW, Darrick /just/ added a -d daxinherit option, though all it does
>> now is set the inheritable dax flag on the root dir, it doesn't enforce
>> things like page vs block size, etc.
> 
> I am aware of that patch, but I considered the option to be somewhat
> orthogonal, given that FS_XFLAG_DAX can be set (and inherited)
> irrespective of dax support in the block device (and overridden via
> mount opts if need be), so I didn't want to overload daxinherit.

OK fair enough.

>> That change is currently staged in my local tree.
>>
>> I suppose we could condition that on other requirements, although we've
>> always had the ability to mkfs a filesystem that can't necessarily be
>> used on the current machine - i.e. you can make a 64k block size filesystem
>> on a 4k page machine, etc.  So I'm not sure we want to tie mkfs abilities
>> to the current mkfs environment....
> 
> Agreed, so I suppose any dax option should be an opt-in, e.g. similar to
> the -d dax=1 proposal. That won't prevent users from neglecting it and
> creating a fs which will be later incompatible with -o dax, but that's a
> different story I guess..

I guess my overarching desire here is to not try to predict too much, or to
make predictions that won't stand the test of time. Inferring what the admin
wants can be tricky.  :)  

I also want to be consistent; i.e. today we can mkfs a 64k block filesystem
on a 4k block machine, and no warning is emitted. If we mkfs a V5/CRC filesystem
or a reflink filesystem on an old kernel that doesn't support it, no warning is
emitted.

If we're going to warn every time when "this filesystem can't be used under the
currently running kernel" then there are quite a lot of cases to handle... I
don't want to warn about some things and not others, so we need to decide if
this is actually something we want to do in general, not just for dax.  I'm 
somewhat disinclined, TBH, and would rather rely on a mount failure to alert
the admin to the problem.  (It's not like there are a lot of resources invested
between mkfs & mount).

I'll reread this thread & Dave's responses and give this some more thought.

Thanks,
-Eric

> - Anthony
> 

Re: [PATCH 0/6] xfsprogs: blockdev dax detection and warnings

[Dave Chinner]

On Tue, Aug 25, 2020 at 07:40:15AM -0700, Darrick J. Wong wrote:
> Zooming out a bit, maybe we should instead introduce a new "tuning"
> parameter for -d and -r so that administrators could tune the filesystem
> for specific purposes:
> 
> 	-d tune=dax: Reject if device not dax, set daxinherit=1, set
> 	extsize/su/sw to match PMD
> 
> 	-d tune=ssd: Set agcount to match the number of CPUs if
> 	possible, make the log larger to support a large number of
> 	threads and iops.
> 
> 	-d tune=rotational: Probably does nothing. ;)
> 
> 	-d tune=auto: Query blkid to guess which of the above three
> 	profiles we should use.
> 
> 	-d tune=none: No tuning.
> 
> And then you'd do the same for the realtime device.

Please, no.

The problem with this is that a specific "tune" will need to vary
over time (e.g. when reflink is supported with DAX) and so now we
back to the same situation where the definition of "tune=dax"
changes depending on what version of mkfs you use. Hence you can
still make a filesystem that the kernel won't mount because you have
a xfsprogs that supports DAX+reflink and a kernel that doesn't.

I just don't see this as a viable way to produce filesystems that
work for specific situations because it doesn't solve the kernel vs
xfsprogs version support issue that requires tweaking mkfs
parameters manually to avoid...

> This would help us get rid of the seeeekret mkfs wrapper that we use to
> make it easier for our internal customers to use DAX since mkfs.xfs
> doesn't support config files.

Let's fix that, then. I've written a bunch of stuff in the past
couple of years that uses basic ini config files via a simple
library and it just works. If people are happy with ini format
config files via a library, then I'll just go do that, eh?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 5/6] mkfs: remove redundant assignment of cli sb options on failure

[Eric Sandeen]

On 8/24/20 3:37 PM, Anthony Iliopoulos wrote:
> rmapbt and reflink are incompatible with realtime devices and mkfs bails
> out on such configurations.  Switching them off in the cli params after
> exit is dead code, remove it.
> 
> Signed-off-by: Anthony Iliopoulos <ailiop@suse.com>

I'll go ahead & pull patches 5 & 6 since they're standalone fixes.

Reviewed-by: Eric Sandeen <sandeen@redhat.com>


> ---
>  mkfs/xfs_mkfs.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 75e910dd4a30..1f142f78e677 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -1995,14 +1995,12 @@ _("cowextsize not supported without reflink support\n"));
>  		fprintf(stderr,
>  _("reflink not supported with realtime devices\n"));
>  		usage();
> -		cli->sb_feat.reflink = false;
>  	}
>  
>  	if (cli->sb_feat.rmapbt && cli->xi->rtname) {
>  		fprintf(stderr,
>  _("rmapbt not supported with realtime devices\n"));
>  		usage();
> -		cli->sb_feat.rmapbt = false;
>  	}
>  
>  	if (cli->sb_feat.reflink && ft->dax) {
> 

Re: [PATCH 6/6] mkfs: remove a couple of unused function parameters

[Eric Sandeen]

On 8/24/20 3:37 PM, Anthony Iliopoulos wrote:
> initialise_mount does not use mkfs_params, and initialise_ag_headers
> does not use the xfs_sb param, remove them.
> 
> Signed-off-by: Anthony Iliopoulos <ailiop@suse.com>

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  mkfs/xfs_mkfs.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 1f142f78e677..03bbe3b4697d 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -3243,7 +3243,6 @@ start_superblock_setup(
>  
>  static void
>  initialise_mount(
> -	struct mkfs_params	*cfg,
>  	struct xfs_mount	*mp,
>  	struct xfs_sb		*sbp)
>  {
> @@ -3431,7 +3430,6 @@ static void
>  initialise_ag_headers(
>  	struct mkfs_params	*cfg,
>  	struct xfs_mount	*mp,
> -	struct xfs_sb		*sbp,
>  	xfs_agnumber_t		agno,
>  	int			*worst_freelist,
>  	struct list_head	*buffer_list)
> @@ -3776,7 +3774,7 @@ main(
>  	 * provided functions to determine on-disk format information.
>  	 */
>  	start_superblock_setup(&cfg, mp, sbp);
> -	initialise_mount(&cfg, mp, sbp);
> +	initialise_mount(mp, sbp);
>  
>  	/*
>  	 * With the mount set up, we can finally calculate the log size
> @@ -3829,7 +3827,7 @@ main(
>  	 */
>  	INIT_LIST_HEAD(&buffer_list);
>  	for (agno = 0; agno < cfg.agcount; agno++) {
> -		initialise_ag_headers(&cfg, mp, sbp, agno, &worst_freelist,
> +		initialise_ag_headers(&cfg, mp, agno, &worst_freelist,
>  				&buffer_list);
>  
>  		if (agno % 16)
> 

Re: [PATCH 5/6] mkfs: remove redundant assignment of cli sb options on failure

[Eric Sandeen]

On 9/28/20 4:49 PM, Eric Sandeen wrote:
> On 8/24/20 3:37 PM, Anthony Iliopoulos wrote:
>> rmapbt and reflink are incompatible with realtime devices and mkfs bails
>> out on such configurations.  Switching them off in the cli params after
>> exit is dead code, remove it.
>>
>> Signed-off-by: Anthony Iliopoulos <ailiop@suse.com>
> 
> I'll go ahead & pull patches 5 & 6 since they're standalone fixes.
> 
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>

Whoops, Darrick's 

mkfs: fix reflink/rmap logic w.r.t. realtime devices and crc=0 support

already fixed this and a bit more, sorry for the noise.