Re: [PATCH V5 00/12] Enable per-file/per-directory DAX operations V5

From: Ira Weiny <ira.weiny@intel.com>
To: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: Christoph Hellwig <hch@lst.de>,
	Dave Chinner <david@fromorbit.com>,
	"Theodore Y. Ts'o" <tytso@mit.edu>,
	Dan Williams <dan.j.williams@intel.com>, Jan Kara <jack@suse.cz>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	linux-ext4 <linux-ext4@vger.kernel.org>,
	linux-xfs <linux-xfs@vger.kernel.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: [PATCH V5 00/12] Enable per-file/per-directory DAX operations V5
Date: Thu, 2 Apr 2020 21:39:01 -0700	[thread overview]
Message-ID: <20200403043901.GH3952565@iweiny-DESK2.sc.intel.com> (raw)
In-Reply-To: <20200401040021.GC56958@magnolia>

On Wed, Apr 01, 2020 at 04:00:21AM +0000, Darrick J. Wong wrote:
> On Mon, Mar 16, 2020 at 10:55:09AM +0100, Christoph Hellwig wrote:
> > On Mon, Mar 16, 2020 at 10:52:24AM +0100, Jan Kara wrote:
> > > > This sounds reasonable to me.
> > > > 
> > > > As for deprecating the mount option, I think at a minimum it needs to
> > > > continue be accepted as an option even if it is ignored to not break
> > > > existing setups.
> > > 
> > > Agreed. But that's how we usually deprecate mount options. Also I'd say
> > > that statx() support for reporting DAX state and some education of
> > > programmers using DAX is required before we deprecate the mount option
> > > since currently applications check 'dax' mount option to determine how much
> > > memory they need to set aside for page cache before they consume everything
> > > else on the machine...
> > 
> > I don't even think we should deprecate it.  It isn't painful to maintain
> > and actually useful for testing.  Instead we should expand it into a
> > tristate:
> > 
> >   dax=off
> >   dax=flag
> >   dax=always
> > 
> > where the existing "dax" option maps to "dax=always" and nodax maps
> > to "dax=off". and dax=flag becomes the default for DAX capable devices.
> 
> That works for me.  In summary:
> 
>  - Applications must call statx to discover the current S_DAX state.
> 
>  - There exists an advisory file inode flag FS_XFLAG_DAX that can be
>    changed on files that have no blocks allocated to them.  Changing
>    this flag does not necessarily change the S_DAX state immediately
>    but programs can query the S_DAX state via statx.
> 
>    If FS_XFLAG_DAX is set and the fs is on pmem then it will always
>    enable S_DAX at inode load time; if FS_XFLAG_DAX is not set, it will
>    never enable S_DAX.  Unless overridden...
> 
>  - There exists a dax= mount option.  dax=off means "never set S_DAX,
>    ignore FS_XFLAG_DAX"; dax=always means "always set S_DAX (at least on
>    pmem), ignore FS_XFLAG_DAX"; and dax=iflag means "follow FS_XFLAG_DAX"
>    and is the default.  "dax" by itself means "dax=always".  "nodax"
>    means "dax=off".
> 
>  - There exists an advisory directory inode flag FS_XFLAG_DAX that can
>    be changed at any time.  The flag state is copied into any files or
>    subdirectories created within that directory.  If programs require
>    that file access runs in S_DAX mode, they'll have to create those
>    files themselves inside a directory with FS_XFLAG_DAX set, or mount
>    the fs with dax=always.

One other thing to add here.  They _can_ set the FS_XFLAG_DAX on a file with
data and force an eviction to get S_DAX to change.

I think that is a nice reason to have a different error code returned.

> 
> Ok?  Let's please get this part finished for 5.8, then we can get back
> to arguing about fs-rmap and reflink and dax and whatnot.

I'm happy to see you motivated to get this in.

I'm starting with a new xfstest to make sure we agree on the semantics prior to
more patches.  I hope to have the xfstest patch sent tomorrow sometime.

Ira

> 
> --D