Re: [PATCH v5 2/5] fuse: make DAX mount option a tri-state

[Vivek Goyal <vgoyal@redhat.com>]

On Tue, Sep 28, 2021 at 01:17:27PM +0800, JeffleXu wrote:
> 
> 
> On 9/28/21 11:44 AM, Dave Chinner wrote:
> > On Mon, Sep 27, 2021 at 10:28:34AM +0800, JeffleXu wrote:
> >> On 9/27/21 8:21 AM, Dave Chinner wrote:
> >>> On Thu, Sep 23, 2021 at 09:02:26PM -0400, Vivek Goyal wrote:
> >>>> On Fri, Sep 24, 2021 at 08:26:18AM +1000, Dave Chinner wrote:
> >>>>> On Thu, Sep 23, 2021 at 03:02:41PM -0400, Vivek Goyal wrote:
> >>> In the case that the user changes FS_XFLAG_DAX, the FUSE client
> >>> needs to communicate that attribute change to the server, where the
> >>> server then changes the persistent state of the on-disk inode so
> >>> that the next time the client requests that inode, it gets the state
> >>> it previously set. Unless, of course, there are server side policy
> >>> overrides (never/always).
> >>
> >> One thing I'm concerned with is that, is the following behavior
> >> consistent with the semantics of per-file DAX in ext4/xfs?
> >>
> >> Client changes FS_XFLAG_DAX, this change is communicated to server and
> >> then server also returns **success**. Then client finds that this file
> >> is not DAX enabled, since server doesn't honor the previously set state.
> > 
> > FS_XFLAG_DAX is advisory in nature - it does not have to be honored
> > at inode instantiation.
> 
> Fine.
> 
> > 
> >> IOWs, shall server always honor the persistent per-inode attribute of
> >> host file (if the change of FS_XFLAG_DAX inside guest is stored as
> >> persistent per-inode attribute on host file)?
> > 
> > If the user set the flag, then queries it, the server should be
> > returning the state that the user set, regardless of whether it is
> > being honored at inode instantiation time.
> > 
> > Remember, FS_XFLAG_DAX does not imply S_DAX and vice versa
> Got it.
> 
> > 
> >>>> Not sure what do you mean by server turns of DAX flag based on client
> >>>> turning off DAX. Server does not have to do anything. If client decides
> >>>> to not use DAX (in guest), it will not send FUSE_SETUPMAPPING requests
> >>>> to server and that's it.
> >>>
> >>> Where does the client get it's per-inode DAX policy from if
> >>> dax=inode is, like other DAX filesystems, the default behaviour?
> >>>
> >>> Where is the persistent storage of that per-inode attribute kept?
> >>
> >> In the latest patch set, it is not supported yet to change FS_XFLAG_DAX
> >> (and thus setting/clearing persistent per-inode attribute) inside guest,
> >> since this scenario is not urgently needed as the real using case.
> > 
> > AFAICT the FS_IOC_FS{GS}ETXATTR ioctl is already supported by the
> > fuse client and it sends the ioctl to the server. Hence the client
> > should already support persistent FS_XFLAG_DAX manipulations
> > regardless of where/how the attribute is stored by the server. Did
> > you actually add code to the client in this patchset to stop
> > FS_XFLAG_DAX from being propagated to the server?
> 
> Yes fuse client supports FS_IOC_FS{GS}ETXATTR ioctl already, but AFAIK
> "passthrough" type virtiofsd doesn't support FUSE_IOCTL yet. My previous
> patch had ever added support for FUSE_IOCTL to virtiofsd.
> 
> > 
> >> Currently the per-inode dax attribute is completely fed from server
> >> thourgh FUSE protocol, e.g. server could set/clear the per-inode dax
> >> attribute depending on the file size.
> > 
> > Yup, that's a policy dax=inode on the client side would allow.
> > Indeed, this same policy could also be implemented as a client side
> > policy, allowing user control instead of admin control of such
> > conditional DAX behaviour... :)
> > 
> >> The previous path set indeed had ever supported changing FS_XFLAG_DAX
> >> and accordingly setting/clearing persistent per-inode attribute inside
> >> guest. For "passthrough" type virtiofsd, the persistent per-inode
> >> attribute is stored as XFS_DIFLAG2_DAX/EXT4_DAX_FL on host file
> >> directly, since this is what "passthrough" means.
> > 
> > Right, but that's server side storage implementation details, not a
> > protocol or client side detail. What I can't find in the current
> > client is where this per-inode flag is actually used in the FUSE dax
> > inode init path - it just checks whether the connection has DAX
> > state set up. Hence I don't see how FS_XFLAG_DAX control from the
> > client currently has any influence on the client side DAX usage.
> 
> Fuse client fetches the per-inode DAX attribute from
> fuse_entry_out.attr.flags of FUSE_LOOKUP reply. It's implemented in
> patch 4 of this patch set.
> 
> The background info is that, fuse client will send a FUSE_LOOKUP request
> to server during inode instantiation, while FS_XFLAG_DAX flag is not
> included in the FUSE_LOOKUP reply, and thus fuse client need to send
> another FUSE_IOCTL if it wants to query the persistent inode flags. To
> remove this extra fuse request during inode instantiation, this flag is
> merged into FUSE_LOOKUP reply (fuse_entry_out.attr.flags) as
> FUSE_ATTR_DAX. Then if FUSE_ATTR_DAX flag is set in
> fuse_entry_out.attr.flags, then fuse client knows that this file shall
> be DAX enabled.
> 
> IOWs, under this mechanism it relies on fuse server to check persistent
> inode flags on host, and then set FUSE_ATTR_DAX flag accordingly.
> 
> > 
> > Seems somewhat crazy to me explicitly want to remove that client
> > side control of per-inode behaviour whilst adding the missing client
> > side bits that allow for the per-inode policy control from server
> > side.  Can we please just start with the common, compatible
> > dax=inode behaviour on the client side, then layer the server side
> > policy control options over the top of that?
> 
> 
> Hi Vivek,
> 
> It seems that we shall also support setting/clearing FS_XFLAG_DAX inside
> guest? If that's the case, then how to design virtiofsd behavior? I
> mean, is it mandatory or optional for virtiofsd to query FS_XFLAG_DAX of
> host files when guest is mounted with "dax=inode"? If it's optional,
> then the performance may be better since it doesn't need to do one extra
> FS_IOC_FSGETXATTR ioctl when handling FUSE_LOOKUP, but admin needs to
> specify "-o policy=flag" to virtiofsd explicitly if it's really needed.

Hi Jeffle,

How about first doing a patch series to just enable ioctl in virtiofsd.
I know David Gilbert and others had security concenrs w.r.t. These
are coming from untrusted guest and they had concerns that we should
only allow selective operations as needed (opted-in by admin). So may
be a daemon option which specifies which operations to allow.

Once that's done, we probably will have to do a patch series, to
make sure FS_XFLAG_DAX inherit behavior is working properly. Especially
that behavior about inheriting FS_XFLAG_DAX flag when a new file
is created and parent dir has FS_XFLAG_DAX set. May be we can just
rely on host filesystem doing it? Limitation will be that it will
only work if host fs is ext4/xfs.

w.r.t virtiofsd, I think we can provide an option say "-o
dax_policy=<option>", which controls what policy is in effect. So if
a daemon specific policy is in effect, we can skip checking FS_XFLAG_DAX
state. In fact, checking FS_XFLAG_DAX state also should probably be
a policy option and not enabled by default.

Say, "-o dax_policy=FS_XFLAG_DAX" will enable cheking FS_XFLAG_DAX on
inode during FUSE_LOOKUP time. Otherwise daemon can fallback to
its own policy and set DAX flag in lookup reply accordingly.

Vivek