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

[Dave Chinner <david@fromorbit.com>]

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