Re: [PATCH 2/2] mkfs: allow setting dax flag on root directory

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

On Tue, Aug 11, 2020 at 03:02:07PM -0500, Eric Sandeen wrote:
> On 8/11/20 12:54 PM, Darrick J. Wong wrote:
> > On Tue, Aug 11, 2020 at 02:39:01PM -0500, Eric Sandeen wrote:
> >> On 8/11/20 9:42 AM, Darrick J. Wong wrote:
> >>> From: Darrick J. Wong <darrick.wong@oracle.com>
> >>>
> >>> Teach mkfs to set the DAX flag on the root directory so that all new
> >>> files can be created in dax mode.  This is a complement to removing the
> >>> mount option.
> >>
> >> So, a new -d option, "-d dax"
> >>
> >> This is ~analogous to cowextsize, rtinherit, projinherit, and extszinherit
> >> so there is certainly precedence for this.  (where only rtinherit is a boolean
> >> like this, but they are all inheritable behaviors)
> >>
> >> (I wonder if "daxinherit" would be more consistent, but won't bikeshed
> >> that (much))
> > 
> > /me is indifferent either way.  But I guess some day we might want to
> > have a dax= flag to indicate something like "set the data device
> > geometry to optimize for DAX?
> > 
> > Nah, I think if we were ever going to do that, we'd have something more
> > like:
> > 
> > 	-d usage=dax
> > 	-d usage=ssd
> > 	-d usage=floopy
> > 
> > Meh.  I'll change it to daxinherit, since that /is/ what it does.
> 
> Ok.  I'm really pretty indifferent as well.
> 
> >>> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> >>> ---
> >>>  man/man8/mkfs.xfs.8 |   11 +++++++++++
> >>>  mkfs/xfs_mkfs.c     |   14 ++++++++++++++
> >>>  2 files changed, 25 insertions(+)
> >>>
> >>>
> >>> diff --git a/man/man8/mkfs.xfs.8 b/man/man8/mkfs.xfs.8
> >>> index 9d762a43011a..4b4fdd86b2f4 100644
> >>> --- a/man/man8/mkfs.xfs.8
> >>> +++ b/man/man8/mkfs.xfs.8
> >>> @@ -394,6 +394,17 @@ All inodes created by
> >>>  will have this extent size hint applied.
> >>>  The value must be provided in units of filesystem blocks.
> >>>  Directories will pass on this hint to newly created children.
> >>> +.TP
> >>> +.BI dax= value
> >>> +All inodes created by
> >>> +.B mkfs.xfs
> >>> +will have the DAX flag set.
> >>> +This means that directories will pass the flag on to newly created files
> >>
> >> let's call this "children" to match the other similar options?
> >>
> >> (because technically it is passed on not only to regular files, right?)
> > 
> > Directories and regular files, though not to other special files.
> > Maybe we should fix that.
> 
> Ok so not all children.  But also not just files.  :P
> 
> "... pass the flag on to newly created files and directories, so that new
> files will use the DAX IO paths when possible." ?
> 
> >>> +and files will use the DAX IO paths when possible.
> >>> +This value is either 1 to enable the use or 0 to disable.
> 
> ...
> 
> >>> @@ -369,6 +371,12 @@ static struct opt_params dopts = {
> >>>  		  .maxval = UINT_MAX,
> >>>  		  .defaultval = SUBOPT_NEEDS_VAL,
> >>>  		},
> >>> +		{ .index = D_DAX,
> >>> +		  .conflicts = { { NULL, LAST_CONFLICT } },
> >>
> >> er....  should we conflict with reflink ....  ?
> 
> Thoughts? :)

That only prevents the user from specifying a reflink cli option; it
doesn't prevent them from turning on daxinherit when reflink already
defaults to enabled.

If you want to prevent people from formatting with the two options, you
have to do it in validate_sb_features.  Hm, maybe we should at least
warn about that.

> >>> +		  .minval = 0,
> >>> +		  .maxval = 1,
> >>> +		  .defaultval = 1,
> >>
> >> Hm, interesting that this is a little different from rtinherit:
> >>
> >>                 { .index = D_RTINHERIT,
> >>                   .conflicts = { { NULL, LAST_CONFLICT } },
> >>                   .minval = 1,
> >>                   .maxval = 1,
> >>                   .defaultval = 1,
> >>                 },
> >>
> >> I think this means that:
> >>
> >> -d rtinherit
> >> -d rtinherit=1
> >>
> >> are valid, but
> >>
> >> -d rtinherit=0 is not, but
> >>
> >> -d dax
> >> -d dax=1
> >> -d dax=0
> >>
> >> are all valid?
> > 
> > TBH, I find it a little odd that you *can't* say "-d rtinherit=0" from a
> > completeness perspective, but...
> 
> We could probably loosen it up and start allowing zero here too.
> It wouldn't break any old scripts, right.

Right.

> >> While the latter makes a bit more sense, I wonder if we should stay
> >> consistent w/ the rtinherit semantics.  Or do you envision some sort
> >> of automatic enabling of this based on device typethat we'd need to
> >> override in the future?
> > 
> > ...the goal is to set this automatically once distros start shipping a
> > libblkid that has blkid_topology_get_dax().  At that point we'll
> > probably want a way to force it off.
> 
> *nod*
> 
> > Unless we want the ability to specify -ddax=0 the magic seekrit hook to
> > discover if (future) mkfs actually supports dax autodetection?  Hmm,
> > that alone sounds like sufficient justification.  Ok.
> 
> Not sure I followed that... :)

Never mind, I talked myself out of it since script authors aren't going
to be happy with an option that only sometimes works.

--D

> -Eric
>