Re: [PATCH v2 3/4] fuse: add per-file DAX flag
On Wed, Jul 21, 2021 at 10:14:44PM +0800, JeffleXu wrote: [..] > > Also, please copy virtiofs list (virtio...@redhat.com) when you post > > patches next time. > > > > Got it. By the way, what's the git repository of virtiofsd? AFAIK, > virtiofsd included in qemu (g...@github.com:qemu/qemu.git) doesn't > support DAX yet? Yes virtiofsd got merged in qemu upstream. And it does not support dax yet. David is still sorting out couple of issues based on feedback. I think following is the branch where he had pushed his latest patches. https://gitlab.com/virtio-fs/qemu/-/tree/virtio-fs-dev David, please correct me if that's not the case. Vivek ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 3/4] fuse: add per-file DAX flag
On 7/21/21 3:27 AM, Vivek Goyal wrote:
> On Tue, Jul 20, 2021 at 02:51:34PM +0800, JeffleXu wrote:
>>
>>
>> On 7/20/21 3:44 AM, Vivek Goyal wrote:
>>> On Fri, Jul 16, 2021 at 06:47:52PM +0800, Jeffle Xu wrote:
Add one flag for fuse_attr.flags indicating if DAX shall be enabled for
this file.
When the per-file DAX flag changes for an *opened* file, the state of
the file won't be updated until this file is closed and reopened later.
Signed-off-by: Jeffle Xu
---
fs/fuse/dax.c | 21 +
fs/fuse/file.c| 4 ++--
fs/fuse/fuse_i.h | 5 +++--
fs/fuse/inode.c | 5 -
include/uapi/linux/fuse.h | 5 +
5 files changed, 31 insertions(+), 9 deletions(-)
diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
index a478e824c2d0..0e862119757a 100644
--- a/fs/fuse/dax.c
+++ b/fs/fuse/dax.c
@@ -1341,7 +1341,7 @@ static const struct address_space_operations
fuse_dax_file_aops = {
.invalidatepage = noop_invalidatepage,
};
-static bool fuse_should_enable_dax(struct inode *inode)
+static bool fuse_should_enable_dax(struct inode *inode, unsigned int
flags)
{
struct fuse_conn *fc = get_fuse_conn(inode);
unsigned int mode;
@@ -1354,18 +1354,31 @@ static bool fuse_should_enable_dax(struct inode
*inode)
if (mode == FUSE_DAX_MOUNT_NEVER)
return false;
- return true;
+ if (mode == FUSE_DAX_MOUNT_ALWAYS)
+ return true;
+
+ WARN_ON(mode != FUSE_DAX_MOUNT_INODE);
+ return flags & FUSE_ATTR_DAX;
}
-void fuse_dax_inode_init(struct inode *inode)
+void fuse_dax_inode_init(struct inode *inode, unsigned int flags)
{
- if (!fuse_should_enable_dax(inode))
+ if (!fuse_should_enable_dax(inode, flags))
return;
inode->i_flags |= S_DAX;
inode->i_data.a_ops = &fuse_dax_file_aops;
}
+void fuse_dax_dontcache(struct inode *inode, bool newdax)
+{
+ struct fuse_conn *fc = get_fuse_conn(inode);
+
+ if (fc->dax && fc->dax->mode == FUSE_DAX_MOUNT_INODE &&
+ IS_DAX(inode) != newdax)
+ d_mark_dontcache(inode);
+}
+
>>>
>>> This capability to mark an inode dontcache should probably be in a
>>> separate patch. These seem to logically two functionalities. One is
>>> enabling DAX on an inode. And second is making sure how soon you
>>> see the effect of that change and hence marking inode dontcache.
>>
>> OK, sounds reasonable.
>>
>>>
>>> Not sure how useful this is. In cache=none mode we should get rid of
>>> inode ASAP. In cache=auto mode we will get rid of after 1 second (or
>>> after a user specified timeout). So only place this seems to be
>>> useful is cache=always.
>>
>> Actually dontcache here is used to avoid dynamic switching between DAX
>> and non-DAX state while file is opened. The complexity of dynamic
>> switching is that, you have to clear the address_space, since page cache
>> and DAX entry can not coexist in the address space. Besides,
>> inode->a_ops also needs to be changed dynamically.
>>
>> With dontcache, dynamic switching is no longer needed and the DAX state
>> will be decided only when inode (in memory) is initialized. The downside
>> is that the new DAX state won't be updated until the file is closed and
>> reopened later.
>>
>> 'cache=none' only invalidates dentry, while the inode (in memory) is
>> still there (with address_space uncleared and a_ops unchanged).
>
> Aha.., that's a good point.
>>
>> The dynamic switching may be done, though it's not such straightforward.
>> Currently, ext4/xfs are all implemented in this dontcache way, i.e., the
>> new DAX state won't be seen until the file is closed and reopened later.
>
> Got it. Agreed that dontcache seems reasonable if file's DAX state
> has changed. Keep it in separate patch though with proper commit
> logs.
>
> Also, please copy virtiofs list (virtio...@redhat.com) when you post
> patches next time.
>
Got it. By the way, what's the git repository of virtiofsd? AFAIK,
virtiofsd included in qemu (g...@github.com:qemu/qemu.git) doesn't
support DAX yet?
--
Thanks,
Jeffle
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 3/4] fuse: add per-file DAX flag
On 7/21/21 3:40 AM, Vivek Goyal wrote:
> On Tue, Jul 20, 2021 at 03:19:50PM +0800, JeffleXu wrote:
>>
>>
>> On 7/20/21 2:41 AM, Vivek Goyal wrote:
>>> On Fri, Jul 16, 2021 at 06:47:52PM +0800, Jeffle Xu wrote:
Add one flag for fuse_attr.flags indicating if DAX shall be enabled for
this file.
When the per-file DAX flag changes for an *opened* file, the state of
the file won't be updated until this file is closed and reopened later.
Signed-off-by: Jeffle Xu
>>>
>>> [..]
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index 36ed092227fa..90c9df10d37a 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -184,6 +184,9 @@
*
* 7.34
* - add FUSE_SYNCFS
+ *
+ * 7.35
+ * - add FUSE_ATTR_DAX
*/
#ifndef _LINUX_FUSE_H
@@ -449,8 +452,10 @@ struct fuse_file_lock {
* fuse_attr flags
*
* FUSE_ATTR_SUBMOUNT: Object is a submount root
+ * FUSE_ATTR_DAX: Enable DAX for this file in per-file DAX mode
*/
#define FUSE_ATTR_SUBMOUNT (1 << 0)
+#define FUSE_ATTR_DAX (1 << 1)
>>>
>>> Generic fuse changes (addition of FUSE_ATTR_DAX) should probably in
>>> a separate patch.
>>
>> Got it.
>>
>>>
>>> I am not clear on one thing. If we are planning to rely on persistent
>>> inode attr (FS_XFLAG_DAX as per Documentation/filesystems/dax.rst), then
>>> why fuse server needs to communicate the state of that attr using a
>>> flag? Can client directly query it? I am not sure where at these
>>> attrs stored and if fuse protocol currently supports it.
>>
>> There are two issues.
>>
>> 1. FUSE server side: Algorithm of deciding whether DAX is enabled for a
>> file.
>>
>> As I previously replied in [1], FUSE server must enable DAX if the
>> backend file is flagged with FS_XFLAG_DAX, to make the FS_XFLAG_DAX
>> previously set by FUSE client effective.
>>
>> But I will argue that FUSE server also has the flexibility of the
>> algorithm implementation. Even if guest queries FS_XFLAG_DAX by
>> GETFLAGS/FSGETXATTR ioctl, FUSE server can still enable DAX when the
>> backend file is not FS_XFLAG_DAX flagged.
>>
>>
>> 2. The protocol between server and client.
>>
>> extending LOOKUP vs. LOOKUP + GETFLAGS/FSGETXATTR ioctl
>>
>> As I said in [1], client can directly query the FS_XFLAG_DAX flag, but
>> there will be one more round trip.
>>
>>
>> [1]
>> https://lore.kernel.org/linux-fsdevel/031efb1d-7c0d-35fb-c147-dcc3b6cac...@linux.alibaba.com/T/#m3f3407158b2c028694c85d82d0d6bd0387f4e24e
>>
>>>
>>> What about flag STATX_ATTR_DAX. We probably should report that too
>>> in stat if we are using dax on the inode?
>>>
>>
>> VFS will automatically report STATX_ATTR_DAX if inode is in DAX mode,
>> e.g., in vfs_getattr_nosec().
>
> Good to know. Given user will know which files are using dax and
> which ones are not, it is even more important to define semantics
> properly. In what cases DAX will be driven by FS_XFLAGS_DAX attr
> and in what cases DAX will completely be driven by server.
>
> May be we should divide it in two patch series. First patch series
> implements "-o dax=inode" and server follows FS_XFLAGS_DAX attr
> and reports during lookup/getattr/.
Agreed, '-o dax=inode' and policy upon FS_XFLAG_DAX xattr could be
implemented as the first step.
>
> And once that is merged this can be ehanced with "-o dax=server" where
> server is free to choose what files dax should be used on. Only if
> this is still needed.
I also need to discuss with my colleagues about our using case, and if
FS_XFLAG_DAX poly is enough.
--
Thanks,
Jeffle
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 3/4] fuse: add per-file DAX flag
On Tue, Jul 20, 2021 at 03:19:50PM +0800, JeffleXu wrote:
>
>
> On 7/20/21 2:41 AM, Vivek Goyal wrote:
> > On Fri, Jul 16, 2021 at 06:47:52PM +0800, Jeffle Xu wrote:
> >> Add one flag for fuse_attr.flags indicating if DAX shall be enabled for
> >> this file.
> >>
> >> When the per-file DAX flag changes for an *opened* file, the state of
> >> the file won't be updated until this file is closed and reopened later.
> >>
> >> Signed-off-by: Jeffle Xu
> >
> > [..]
> >> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> >> index 36ed092227fa..90c9df10d37a 100644
> >> --- a/include/uapi/linux/fuse.h
> >> +++ b/include/uapi/linux/fuse.h
> >> @@ -184,6 +184,9 @@
> >> *
> >> * 7.34
> >> * - add FUSE_SYNCFS
> >> + *
> >> + * 7.35
> >> + * - add FUSE_ATTR_DAX
> >> */
> >>
> >> #ifndef _LINUX_FUSE_H
> >> @@ -449,8 +452,10 @@ struct fuse_file_lock {
> >> * fuse_attr flags
> >> *
> >> * FUSE_ATTR_SUBMOUNT: Object is a submount root
> >> + * FUSE_ATTR_DAX: Enable DAX for this file in per-file DAX mode
> >> */
> >> #define FUSE_ATTR_SUBMOUNT (1 << 0)
> >> +#define FUSE_ATTR_DAX (1 << 1)
> >
> > Generic fuse changes (addition of FUSE_ATTR_DAX) should probably in
> > a separate patch.
>
> Got it.
>
> >
> > I am not clear on one thing. If we are planning to rely on persistent
> > inode attr (FS_XFLAG_DAX as per Documentation/filesystems/dax.rst), then
> > why fuse server needs to communicate the state of that attr using a
> > flag? Can client directly query it? I am not sure where at these
> > attrs stored and if fuse protocol currently supports it.
>
> There are two issues.
>
> 1. FUSE server side: Algorithm of deciding whether DAX is enabled for a
> file.
>
> As I previously replied in [1], FUSE server must enable DAX if the
> backend file is flagged with FS_XFLAG_DAX, to make the FS_XFLAG_DAX
> previously set by FUSE client effective.
>
> But I will argue that FUSE server also has the flexibility of the
> algorithm implementation. Even if guest queries FS_XFLAG_DAX by
> GETFLAGS/FSGETXATTR ioctl, FUSE server can still enable DAX when the
> backend file is not FS_XFLAG_DAX flagged.
>
>
> 2. The protocol between server and client.
>
> extending LOOKUP vs. LOOKUP + GETFLAGS/FSGETXATTR ioctl
>
> As I said in [1], client can directly query the FS_XFLAG_DAX flag, but
> there will be one more round trip.
>
>
> [1]
> https://lore.kernel.org/linux-fsdevel/031efb1d-7c0d-35fb-c147-dcc3b6cac...@linux.alibaba.com/T/#m3f3407158b2c028694c85d82d0d6bd0387f4e24e
>
> >
> > What about flag STATX_ATTR_DAX. We probably should report that too
> > in stat if we are using dax on the inode?
> >
>
> VFS will automatically report STATX_ATTR_DAX if inode is in DAX mode,
> e.g., in vfs_getattr_nosec().
Good to know. Given user will know which files are using dax and
which ones are not, it is even more important to define semantics
properly. In what cases DAX will be driven by FS_XFLAGS_DAX attr
and in what cases DAX will completely be driven by server.
May be we should divide it in two patch series. First patch series
implements "-o dax=inode" and server follows FS_XFLAGS_DAX attr
and reports during lookup/getattr/.
And once that is merged this can be ehanced with "-o dax=server" where
server is free to choose what files dax should be used on. Only if
this is still needed.
Vivek
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 3/4] fuse: add per-file DAX flag
On Tue, Jul 20, 2021 at 02:51:34PM +0800, JeffleXu wrote:
>
>
> On 7/20/21 3:44 AM, Vivek Goyal wrote:
> > On Fri, Jul 16, 2021 at 06:47:52PM +0800, Jeffle Xu wrote:
> >> Add one flag for fuse_attr.flags indicating if DAX shall be enabled for
> >> this file.
> >>
> >> When the per-file DAX flag changes for an *opened* file, the state of
> >> the file won't be updated until this file is closed and reopened later.
> >>
> >> Signed-off-by: Jeffle Xu
> >> ---
> >> fs/fuse/dax.c | 21 +
> >> fs/fuse/file.c| 4 ++--
> >> fs/fuse/fuse_i.h | 5 +++--
> >> fs/fuse/inode.c | 5 -
> >> include/uapi/linux/fuse.h | 5 +
> >> 5 files changed, 31 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
> >> index a478e824c2d0..0e862119757a 100644
> >> --- a/fs/fuse/dax.c
> >> +++ b/fs/fuse/dax.c
> >> @@ -1341,7 +1341,7 @@ static const struct address_space_operations
> >> fuse_dax_file_aops = {
> >>.invalidatepage = noop_invalidatepage,
> >> };
> >>
> >> -static bool fuse_should_enable_dax(struct inode *inode)
> >> +static bool fuse_should_enable_dax(struct inode *inode, unsigned int
> >> flags)
> >> {
> >>struct fuse_conn *fc = get_fuse_conn(inode);
> >>unsigned int mode;
> >> @@ -1354,18 +1354,31 @@ static bool fuse_should_enable_dax(struct inode
> >> *inode)
> >>if (mode == FUSE_DAX_MOUNT_NEVER)
> >>return false;
> >>
> >> - return true;
> >> + if (mode == FUSE_DAX_MOUNT_ALWAYS)
> >> + return true;
> >> +
> >> + WARN_ON(mode != FUSE_DAX_MOUNT_INODE);
> >> + return flags & FUSE_ATTR_DAX;
> >> }
> >>
> >> -void fuse_dax_inode_init(struct inode *inode)
> >> +void fuse_dax_inode_init(struct inode *inode, unsigned int flags)
> >> {
> >> - if (!fuse_should_enable_dax(inode))
> >> + if (!fuse_should_enable_dax(inode, flags))
> >>return;
> >>
> >>inode->i_flags |= S_DAX;
> >>inode->i_data.a_ops = &fuse_dax_file_aops;
> >> }
> >>
> >> +void fuse_dax_dontcache(struct inode *inode, bool newdax)
> >> +{
> >> + struct fuse_conn *fc = get_fuse_conn(inode);
> >> +
> >> + if (fc->dax && fc->dax->mode == FUSE_DAX_MOUNT_INODE &&
> >> + IS_DAX(inode) != newdax)
> >> + d_mark_dontcache(inode);
> >> +}
> >> +
> >
> > This capability to mark an inode dontcache should probably be in a
> > separate patch. These seem to logically two functionalities. One is
> > enabling DAX on an inode. And second is making sure how soon you
> > see the effect of that change and hence marking inode dontcache.
>
> OK, sounds reasonable.
>
> >
> > Not sure how useful this is. In cache=none mode we should get rid of
> > inode ASAP. In cache=auto mode we will get rid of after 1 second (or
> > after a user specified timeout). So only place this seems to be
> > useful is cache=always.
>
> Actually dontcache here is used to avoid dynamic switching between DAX
> and non-DAX state while file is opened. The complexity of dynamic
> switching is that, you have to clear the address_space, since page cache
> and DAX entry can not coexist in the address space. Besides,
> inode->a_ops also needs to be changed dynamically.
>
> With dontcache, dynamic switching is no longer needed and the DAX state
> will be decided only when inode (in memory) is initialized. The downside
> is that the new DAX state won't be updated until the file is closed and
> reopened later.
>
> 'cache=none' only invalidates dentry, while the inode (in memory) is
> still there (with address_space uncleared and a_ops unchanged).
Aha.., that's a good point.
>
> The dynamic switching may be done, though it's not such straightforward.
> Currently, ext4/xfs are all implemented in this dontcache way, i.e., the
> new DAX state won't be seen until the file is closed and reopened later.
Got it. Agreed that dontcache seems reasonable if file's DAX state
has changed. Keep it in separate patch though with proper commit
logs.
Also, please copy virtiofs list (virtio...@redhat.com) when you post
patches next time.
Vivek
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 3/4] fuse: add per-file DAX flag
On 7/20/21 2:51 PM, JeffleXu wrote:
>
>
> On 7/20/21 3:44 AM, Vivek Goyal wrote:
>> On Fri, Jul 16, 2021 at 06:47:52PM +0800, Jeffle Xu wrote:
>>> Add one flag for fuse_attr.flags indicating if DAX shall be enabled for
>>> this file.
>>>
>>> When the per-file DAX flag changes for an *opened* file, the state of
>>> the file won't be updated until this file is closed and reopened later.
>>>
>>> Signed-off-by: Jeffle Xu
>>> ---
>>> fs/fuse/dax.c | 21 +
>>> fs/fuse/file.c| 4 ++--
>>> fs/fuse/fuse_i.h | 5 +++--
>>> fs/fuse/inode.c | 5 -
>>> include/uapi/linux/fuse.h | 5 +
>>> 5 files changed, 31 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
>>> index a478e824c2d0..0e862119757a 100644
>>> --- a/fs/fuse/dax.c
>>> +++ b/fs/fuse/dax.c
>>> @@ -1341,7 +1341,7 @@ static const struct address_space_operations
>>> fuse_dax_file_aops = {
>>> .invalidatepage = noop_invalidatepage,
>>> };
>>>
>>> -static bool fuse_should_enable_dax(struct inode *inode)
>>> +static bool fuse_should_enable_dax(struct inode *inode, unsigned int flags)
>>> {
>>> struct fuse_conn *fc = get_fuse_conn(inode);
>>> unsigned int mode;
>>> @@ -1354,18 +1354,31 @@ static bool fuse_should_enable_dax(struct inode
>>> *inode)
>>> if (mode == FUSE_DAX_MOUNT_NEVER)
>>> return false;
>>>
>>> - return true;
>>> + if (mode == FUSE_DAX_MOUNT_ALWAYS)
>>> + return true;
>>> +
>>> + WARN_ON(mode != FUSE_DAX_MOUNT_INODE);
>>> + return flags & FUSE_ATTR_DAX;
>>> }
>>>
>>> -void fuse_dax_inode_init(struct inode *inode)
>>> +void fuse_dax_inode_init(struct inode *inode, unsigned int flags)
>>> {
>>> - if (!fuse_should_enable_dax(inode))
>>> + if (!fuse_should_enable_dax(inode, flags))
>>> return;
>>>
>>> inode->i_flags |= S_DAX;
>>> inode->i_data.a_ops = &fuse_dax_file_aops;
>>> }
>>>
>>> +void fuse_dax_dontcache(struct inode *inode, bool newdax)
>>> +{
>>> + struct fuse_conn *fc = get_fuse_conn(inode);
>>> +
>>> + if (fc->dax && fc->dax->mode == FUSE_DAX_MOUNT_INODE &&
>>> + IS_DAX(inode) != newdax)
>>> + d_mark_dontcache(inode);
>>> +}
>>> +
>>
>> This capability to mark an inode dontcache should probably be in a
>> separate patch. These seem to logically two functionalities. One is
>> enabling DAX on an inode. And second is making sure how soon you
>> see the effect of that change and hence marking inode dontcache.
>
> OK, sounds reasonable.
>
>>
>> Not sure how useful this is. In cache=none mode we should get rid of
>> inode ASAP. In cache=auto mode we will get rid of after 1 second (or
>> after a user specified timeout). So only place this seems to be
>> useful is cache=always.
>
> Actually dontcache here is used to avoid dynamic switching between DAX
> and non-DAX state while file is opened. The complexity of dynamic
> switching is that, you have to clear the address_space, since page cache
> and DAX entry can not coexist in the address space. Besides,
> inode->a_ops also needs to be changed dynamically.
FYI some context on the complication of switching DAX state dynamically,
when Ira Weiny was working on per-file DAX of ext4/xfs.
> At LSF/MM we discussed the difficulties of switching the DAX state of
a file with active mappings / page cache. It was thought the races
could be avoided by limiting DAX state flips to 0-length files.
>
> However, this turns out to not be true.[3][5] This is because address
space operations (a_ops) may be in use at any time the inode is referenced.
> from Ira Weiny
[1]
https://patchwork.kernel.org/project/xfs/cover/20200407182958.568475-1-ira.we...@intel.com/
[2] https://lore.kernel.org/lkml/20200305155144.ga5...@lst.de/
[3] https://lore.kernel.org/lkml/20200401040021.GC56958@magnolia/
[4] https://lore.kernel.org/lkml/20200403182904.GP80283@magnolia/
>
> With dontcache, dynamic switching is no longer needed and the DAX state
> will be decided only when inode (in memory) is initialized. The downside
> is that the new DAX state won't be updated until the file is closed and
> reopened later.
>
> 'cache=none' only invalidates dentry, while the inode (in memory) is
> still there (with address_space uncleared and a_ops unchanged).
>
> The dynamic switching may be done, though it's not such straightforward.
> Currently, ext4/xfs are all implemented in this dontcache way, i.e., the
> new DAX state won't be seen until the file is closed and reopened later.
>
--
Thanks,
Jeffle
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 3/4] fuse: add per-file DAX flag
On 7/20/21 2:41 AM, Vivek Goyal wrote:
> On Fri, Jul 16, 2021 at 06:47:52PM +0800, Jeffle Xu wrote:
>> Add one flag for fuse_attr.flags indicating if DAX shall be enabled for
>> this file.
>>
>> When the per-file DAX flag changes for an *opened* file, the state of
>> the file won't be updated until this file is closed and reopened later.
>>
>> Signed-off-by: Jeffle Xu
>
> [..]
>> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
>> index 36ed092227fa..90c9df10d37a 100644
>> --- a/include/uapi/linux/fuse.h
>> +++ b/include/uapi/linux/fuse.h
>> @@ -184,6 +184,9 @@
>> *
>> * 7.34
>> * - add FUSE_SYNCFS
>> + *
>> + * 7.35
>> + * - add FUSE_ATTR_DAX
>> */
>>
>> #ifndef _LINUX_FUSE_H
>> @@ -449,8 +452,10 @@ struct fuse_file_lock {
>> * fuse_attr flags
>> *
>> * FUSE_ATTR_SUBMOUNT: Object is a submount root
>> + * FUSE_ATTR_DAX: Enable DAX for this file in per-file DAX mode
>> */
>> #define FUSE_ATTR_SUBMOUNT (1 << 0)
>> +#define FUSE_ATTR_DAX (1 << 1)
>
> Generic fuse changes (addition of FUSE_ATTR_DAX) should probably in
> a separate patch.
Got it.
>
> I am not clear on one thing. If we are planning to rely on persistent
> inode attr (FS_XFLAG_DAX as per Documentation/filesystems/dax.rst), then
> why fuse server needs to communicate the state of that attr using a
> flag? Can client directly query it? I am not sure where at these
> attrs stored and if fuse protocol currently supports it.
There are two issues.
1. FUSE server side: Algorithm of deciding whether DAX is enabled for a
file.
As I previously replied in [1], FUSE server must enable DAX if the
backend file is flagged with FS_XFLAG_DAX, to make the FS_XFLAG_DAX
previously set by FUSE client effective.
But I will argue that FUSE server also has the flexibility of the
algorithm implementation. Even if guest queries FS_XFLAG_DAX by
GETFLAGS/FSGETXATTR ioctl, FUSE server can still enable DAX when the
backend file is not FS_XFLAG_DAX flagged.
2. The protocol between server and client.
extending LOOKUP vs. LOOKUP + GETFLAGS/FSGETXATTR ioctl
As I said in [1], client can directly query the FS_XFLAG_DAX flag, but
there will be one more round trip.
[1]
https://lore.kernel.org/linux-fsdevel/031efb1d-7c0d-35fb-c147-dcc3b6cac...@linux.alibaba.com/T/#m3f3407158b2c028694c85d82d0d6bd0387f4e24e
>
> What about flag STATX_ATTR_DAX. We probably should report that too
> in stat if we are using dax on the inode?
>
VFS will automatically report STATX_ATTR_DAX if inode is in DAX mode,
e.g., in vfs_getattr_nosec().
--
Thanks,
Jeffle
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 3/4] fuse: add per-file DAX flag
On 7/20/21 3:44 AM, Vivek Goyal wrote:
> On Fri, Jul 16, 2021 at 06:47:52PM +0800, Jeffle Xu wrote:
>> Add one flag for fuse_attr.flags indicating if DAX shall be enabled for
>> this file.
>>
>> When the per-file DAX flag changes for an *opened* file, the state of
>> the file won't be updated until this file is closed and reopened later.
>>
>> Signed-off-by: Jeffle Xu
>> ---
>> fs/fuse/dax.c | 21 +
>> fs/fuse/file.c| 4 ++--
>> fs/fuse/fuse_i.h | 5 +++--
>> fs/fuse/inode.c | 5 -
>> include/uapi/linux/fuse.h | 5 +
>> 5 files changed, 31 insertions(+), 9 deletions(-)
>>
>> diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
>> index a478e824c2d0..0e862119757a 100644
>> --- a/fs/fuse/dax.c
>> +++ b/fs/fuse/dax.c
>> @@ -1341,7 +1341,7 @@ static const struct address_space_operations
>> fuse_dax_file_aops = {
>> .invalidatepage = noop_invalidatepage,
>> };
>>
>> -static bool fuse_should_enable_dax(struct inode *inode)
>> +static bool fuse_should_enable_dax(struct inode *inode, unsigned int flags)
>> {
>> struct fuse_conn *fc = get_fuse_conn(inode);
>> unsigned int mode;
>> @@ -1354,18 +1354,31 @@ static bool fuse_should_enable_dax(struct inode
>> *inode)
>> if (mode == FUSE_DAX_MOUNT_NEVER)
>> return false;
>>
>> -return true;
>> +if (mode == FUSE_DAX_MOUNT_ALWAYS)
>> +return true;
>> +
>> +WARN_ON(mode != FUSE_DAX_MOUNT_INODE);
>> +return flags & FUSE_ATTR_DAX;
>> }
>>
>> -void fuse_dax_inode_init(struct inode *inode)
>> +void fuse_dax_inode_init(struct inode *inode, unsigned int flags)
>> {
>> -if (!fuse_should_enable_dax(inode))
>> +if (!fuse_should_enable_dax(inode, flags))
>> return;
>>
>> inode->i_flags |= S_DAX;
>> inode->i_data.a_ops = &fuse_dax_file_aops;
>> }
>>
>> +void fuse_dax_dontcache(struct inode *inode, bool newdax)
>> +{
>> +struct fuse_conn *fc = get_fuse_conn(inode);
>> +
>> +if (fc->dax && fc->dax->mode == FUSE_DAX_MOUNT_INODE &&
>> +IS_DAX(inode) != newdax)
>> +d_mark_dontcache(inode);
>> +}
>> +
>
> This capability to mark an inode dontcache should probably be in a
> separate patch. These seem to logically two functionalities. One is
> enabling DAX on an inode. And second is making sure how soon you
> see the effect of that change and hence marking inode dontcache.
OK, sounds reasonable.
>
> Not sure how useful this is. In cache=none mode we should get rid of
> inode ASAP. In cache=auto mode we will get rid of after 1 second (or
> after a user specified timeout). So only place this seems to be
> useful is cache=always.
Actually dontcache here is used to avoid dynamic switching between DAX
and non-DAX state while file is opened. The complexity of dynamic
switching is that, you have to clear the address_space, since page cache
and DAX entry can not coexist in the address space. Besides,
inode->a_ops also needs to be changed dynamically.
With dontcache, dynamic switching is no longer needed and the DAX state
will be decided only when inode (in memory) is initialized. The downside
is that the new DAX state won't be updated until the file is closed and
reopened later.
'cache=none' only invalidates dentry, while the inode (in memory) is
still there (with address_space uncleared and a_ops unchanged).
The dynamic switching may be done, though it's not such straightforward.
Currently, ext4/xfs are all implemented in this dontcache way, i.e., the
new DAX state won't be seen until the file is closed and reopened later.
--
Thanks,
Jeffle
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v2 3/4] fuse: add per-file DAX flag
On Fri, Jul 16, 2021 at 06:47:52PM +0800, Jeffle Xu wrote:
> Add one flag for fuse_attr.flags indicating if DAX shall be enabled for
> this file.
>
> When the per-file DAX flag changes for an *opened* file, the state of
> the file won't be updated until this file is closed and reopened later.
>
> Signed-off-by: Jeffle Xu
> ---
> fs/fuse/dax.c | 21 +
> fs/fuse/file.c| 4 ++--
> fs/fuse/fuse_i.h | 5 +++--
> fs/fuse/inode.c | 5 -
> include/uapi/linux/fuse.h | 5 +
> 5 files changed, 31 insertions(+), 9 deletions(-)
>
> diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
> index a478e824c2d0..0e862119757a 100644
> --- a/fs/fuse/dax.c
> +++ b/fs/fuse/dax.c
> @@ -1341,7 +1341,7 @@ static const struct address_space_operations
> fuse_dax_file_aops = {
> .invalidatepage = noop_invalidatepage,
> };
>
> -static bool fuse_should_enable_dax(struct inode *inode)
> +static bool fuse_should_enable_dax(struct inode *inode, unsigned int flags)
> {
> struct fuse_conn *fc = get_fuse_conn(inode);
> unsigned int mode;
> @@ -1354,18 +1354,31 @@ static bool fuse_should_enable_dax(struct inode
> *inode)
> if (mode == FUSE_DAX_MOUNT_NEVER)
> return false;
>
> - return true;
> + if (mode == FUSE_DAX_MOUNT_ALWAYS)
> + return true;
> +
> + WARN_ON(mode != FUSE_DAX_MOUNT_INODE);
> + return flags & FUSE_ATTR_DAX;
> }
>
> -void fuse_dax_inode_init(struct inode *inode)
> +void fuse_dax_inode_init(struct inode *inode, unsigned int flags)
> {
> - if (!fuse_should_enable_dax(inode))
> + if (!fuse_should_enable_dax(inode, flags))
> return;
>
> inode->i_flags |= S_DAX;
> inode->i_data.a_ops = &fuse_dax_file_aops;
> }
>
> +void fuse_dax_dontcache(struct inode *inode, bool newdax)
> +{
> + struct fuse_conn *fc = get_fuse_conn(inode);
> +
> + if (fc->dax && fc->dax->mode == FUSE_DAX_MOUNT_INODE &&
> + IS_DAX(inode) != newdax)
> + d_mark_dontcache(inode);
> +}
> +
This capability to mark an inode dontcache should probably be in a
separate patch. These seem to logically two functionalities. One is
enabling DAX on an inode. And second is making sure how soon you
see the effect of that change and hence marking inode dontcache.
Not sure how useful this is. In cache=none mode we should get rid of
inode ASAP. In cache=auto mode we will get rid of after 1 second (or
after a user specified timeout). So only place this seems to be
useful is cache=always.
Vivek
> bool fuse_dax_check_alignment(struct fuse_conn *fc, unsigned int
> map_alignment)
> {
> if (fc->dax && (map_alignment > FUSE_DAX_SHIFT)) {
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 97f860cfc195..cf42af492146 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -3142,7 +3142,7 @@ static const struct address_space_operations
> fuse_file_aops = {
> .write_end = fuse_write_end,
> };
>
> -void fuse_init_file_inode(struct inode *inode)
> +void fuse_init_file_inode(struct inode *inode, struct fuse_attr *attr)
> {
> struct fuse_inode *fi = get_fuse_inode(inode);
>
> @@ -3156,5 +3156,5 @@ void fuse_init_file_inode(struct inode *inode)
> fi->writepages = RB_ROOT;
>
> if (IS_ENABLED(CONFIG_FUSE_DAX))
> - fuse_dax_inode_init(inode);
> + fuse_dax_inode_init(inode, attr->flags);
> }
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index f29018323845..0793b93d680a 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -1000,7 +1000,7 @@ int fuse_notify_poll_wakeup(struct fuse_conn *fc,
> /**
> * Initialize file operations on a regular file
> */
> -void fuse_init_file_inode(struct inode *inode);
> +void fuse_init_file_inode(struct inode *inode, struct fuse_attr *attr);
>
> /**
> * Initialize inode operations on regular files and special files
> @@ -1252,8 +1252,9 @@ int fuse_dax_conn_alloc(struct fuse_conn *fc, unsigned
> int mode,
> struct dax_device *dax_dev);
> void fuse_dax_conn_free(struct fuse_conn *fc);
> bool fuse_dax_inode_alloc(struct super_block *sb, struct fuse_inode *fi);
> -void fuse_dax_inode_init(struct inode *inode);
> +void fuse_dax_inode_init(struct inode *inode, unsigned int flags);
> void fuse_dax_inode_cleanup(struct inode *inode);
> +void fuse_dax_dontcache(struct inode *inode, bool newdax);
> bool fuse_dax_check_alignment(struct fuse_conn *fc, unsigned int
> map_alignment);
> void fuse_dax_cancel_work(struct fuse_conn *fc);
>
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index f6b46395edb2..2ae92798126e 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -269,6 +269,9 @@ void fuse_change_attributes(struct inode *inode, struct
> fuse_attr *attr,
> if (inval)
> invalidate_inode_pages2(inode->i_mapping);
> }
> +
> + if (IS_ENABLED(CONFIG_FUSE_DAX))
> + fuse_dax_
Re: [PATCH v2 3/4] fuse: add per-file DAX flag
On Fri, Jul 16, 2021 at 06:47:52PM +0800, Jeffle Xu wrote:
> Add one flag for fuse_attr.flags indicating if DAX shall be enabled for
> this file.
>
> When the per-file DAX flag changes for an *opened* file, the state of
> the file won't be updated until this file is closed and reopened later.
>
> Signed-off-by: Jeffle Xu
[..]
> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> index 36ed092227fa..90c9df10d37a 100644
> --- a/include/uapi/linux/fuse.h
> +++ b/include/uapi/linux/fuse.h
> @@ -184,6 +184,9 @@
> *
> * 7.34
> * - add FUSE_SYNCFS
> + *
> + * 7.35
> + * - add FUSE_ATTR_DAX
> */
>
> #ifndef _LINUX_FUSE_H
> @@ -449,8 +452,10 @@ struct fuse_file_lock {
> * fuse_attr flags
> *
> * FUSE_ATTR_SUBMOUNT: Object is a submount root
> + * FUSE_ATTR_DAX: Enable DAX for this file in per-file DAX mode
> */
> #define FUSE_ATTR_SUBMOUNT (1 << 0)
> +#define FUSE_ATTR_DAX(1 << 1)
Generic fuse changes (addition of FUSE_ATTR_DAX) should probably in
a separate patch.
I am not clear on one thing. If we are planning to rely on persistent
inode attr (FS_XFLAG_DAX as per Documentation/filesystems/dax.rst), then
why fuse server needs to communicate the state of that attr using a
flag? Can client directly query it? I am not sure where at these
attrs stored and if fuse protocol currently supports it.
What about flag STATX_ATTR_DAX. We probably should report that too
in stat if we are using dax on the inode?
Vivek
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH v2 3/4] fuse: add per-file DAX flag
Add one flag for fuse_attr.flags indicating if DAX shall be enabled for
this file.
When the per-file DAX flag changes for an *opened* file, the state of
the file won't be updated until this file is closed and reopened later.
Signed-off-by: Jeffle Xu
---
fs/fuse/dax.c | 21 +
fs/fuse/file.c| 4 ++--
fs/fuse/fuse_i.h | 5 +++--
fs/fuse/inode.c | 5 -
include/uapi/linux/fuse.h | 5 +
5 files changed, 31 insertions(+), 9 deletions(-)
diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
index a478e824c2d0..0e862119757a 100644
--- a/fs/fuse/dax.c
+++ b/fs/fuse/dax.c
@@ -1341,7 +1341,7 @@ static const struct address_space_operations
fuse_dax_file_aops = {
.invalidatepage = noop_invalidatepage,
};
-static bool fuse_should_enable_dax(struct inode *inode)
+static bool fuse_should_enable_dax(struct inode *inode, unsigned int flags)
{
struct fuse_conn *fc = get_fuse_conn(inode);
unsigned int mode;
@@ -1354,18 +1354,31 @@ static bool fuse_should_enable_dax(struct inode *inode)
if (mode == FUSE_DAX_MOUNT_NEVER)
return false;
- return true;
+ if (mode == FUSE_DAX_MOUNT_ALWAYS)
+ return true;
+
+ WARN_ON(mode != FUSE_DAX_MOUNT_INODE);
+ return flags & FUSE_ATTR_DAX;
}
-void fuse_dax_inode_init(struct inode *inode)
+void fuse_dax_inode_init(struct inode *inode, unsigned int flags)
{
- if (!fuse_should_enable_dax(inode))
+ if (!fuse_should_enable_dax(inode, flags))
return;
inode->i_flags |= S_DAX;
inode->i_data.a_ops = &fuse_dax_file_aops;
}
+void fuse_dax_dontcache(struct inode *inode, bool newdax)
+{
+ struct fuse_conn *fc = get_fuse_conn(inode);
+
+ if (fc->dax && fc->dax->mode == FUSE_DAX_MOUNT_INODE &&
+ IS_DAX(inode) != newdax)
+ d_mark_dontcache(inode);
+}
+
bool fuse_dax_check_alignment(struct fuse_conn *fc, unsigned int map_alignment)
{
if (fc->dax && (map_alignment > FUSE_DAX_SHIFT)) {
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 97f860cfc195..cf42af492146 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -3142,7 +3142,7 @@ static const struct address_space_operations
fuse_file_aops = {
.write_end = fuse_write_end,
};
-void fuse_init_file_inode(struct inode *inode)
+void fuse_init_file_inode(struct inode *inode, struct fuse_attr *attr)
{
struct fuse_inode *fi = get_fuse_inode(inode);
@@ -3156,5 +3156,5 @@ void fuse_init_file_inode(struct inode *inode)
fi->writepages = RB_ROOT;
if (IS_ENABLED(CONFIG_FUSE_DAX))
- fuse_dax_inode_init(inode);
+ fuse_dax_inode_init(inode, attr->flags);
}
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index f29018323845..0793b93d680a 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -1000,7 +1000,7 @@ int fuse_notify_poll_wakeup(struct fuse_conn *fc,
/**
* Initialize file operations on a regular file
*/
-void fuse_init_file_inode(struct inode *inode);
+void fuse_init_file_inode(struct inode *inode, struct fuse_attr *attr);
/**
* Initialize inode operations on regular files and special files
@@ -1252,8 +1252,9 @@ int fuse_dax_conn_alloc(struct fuse_conn *fc, unsigned
int mode,
struct dax_device *dax_dev);
void fuse_dax_conn_free(struct fuse_conn *fc);
bool fuse_dax_inode_alloc(struct super_block *sb, struct fuse_inode *fi);
-void fuse_dax_inode_init(struct inode *inode);
+void fuse_dax_inode_init(struct inode *inode, unsigned int flags);
void fuse_dax_inode_cleanup(struct inode *inode);
+void fuse_dax_dontcache(struct inode *inode, bool newdax);
bool fuse_dax_check_alignment(struct fuse_conn *fc, unsigned int
map_alignment);
void fuse_dax_cancel_work(struct fuse_conn *fc);
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index f6b46395edb2..2ae92798126e 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -269,6 +269,9 @@ void fuse_change_attributes(struct inode *inode, struct
fuse_attr *attr,
if (inval)
invalidate_inode_pages2(inode->i_mapping);
}
+
+ if (IS_ENABLED(CONFIG_FUSE_DAX))
+ fuse_dax_dontcache(inode, attr->flags & FUSE_ATTR_DAX);
}
static void fuse_init_inode(struct inode *inode, struct fuse_attr *attr)
@@ -281,7 +284,7 @@ static void fuse_init_inode(struct inode *inode, struct
fuse_attr *attr)
inode->i_ctime.tv_nsec = attr->ctimensec;
if (S_ISREG(inode->i_mode)) {
fuse_init_common(inode);
- fuse_init_file_inode(inode);
+ fuse_init_file_inode(inode, attr);
} else if (S_ISDIR(inode->i_mode))
fuse_init_dir(inode);
else if (S_ISLNK(inode->i_mode))
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index 36ed092227fa..90c9df10d37a 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -184,6 +
