Re: [fuse-devel] [PATCH] fuse: add a new flag to allow shared mmap in FOPEN_DIRECT_IO mode

[Bernd Schubert <bernd.schubert@fastmail.fm>]

On 5/6/23 09:03, Hao Xu wrote:
> Hi Bernd,
> 
> 
> On 5/5/23 22:39, Bernd Schubert wrote:
>>
>>
>> On 5/5/23 10:16, Hao Xu wrote:
>>> From: Hao Xu <howeyxu@tencent.com>
>>>
>>> FOPEN_DIRECT_IO is usually set by fuse daemon to indicate need of strong
>>> coherency, e.g. network filesystems. Thus shared mmap is disabled since
>>> it leverages page cache and may write to it, which may cause
>>> inconsistence. But FOPEN_DIRECT_IO can be used not for coherency but to
>>> reduce memory footprint as well, e.g. reduce guest memory usage with
>>> virtiofs. Therefore, add a new flag FOPEN_DIRECT_IO_SHARED_MMAP to allow
>>> shared mmap for these cases.
>>>
>>> Signed-off-by: Hao Xu <howeyxu@tencent.com>
>>> ---
>>>   fs/fuse/file.c            | 11 ++++++++---
>>>   include/uapi/linux/fuse.h |  2 ++
>>>   2 files changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
>>> index 89d97f6188e0..655896bdb0d5 100644
>>> --- a/fs/fuse/file.c
>>> +++ b/fs/fuse/file.c
>>> @@ -161,7 +161,8 @@ struct fuse_file *fuse_file_open(struct 
>>> fuse_mount *fm, u64 nodeid,
>>>       }
>>>         if (isdir)
>>> -        ff->open_flags &= ~FOPEN_DIRECT_IO;
>>> +        ff->open_flags &=
>>> +            ~(FOPEN_DIRECT_IO | FOPEN_DIRECT_IO_SHARED_MMAP);
>>>         ff->nodeid = nodeid;
>>>   @@ -2509,8 +2510,12 @@ static int fuse_file_mmap(struct file *file, 
>>> struct vm_area_struct *vma)
>>>           return fuse_dax_mmap(file, vma);
>>>         if (ff->open_flags & FOPEN_DIRECT_IO) {
>>> -        /* Can't provide the coherency needed for MAP_SHARED */
>>> -        if (vma->vm_flags & VM_MAYSHARE)
>>> +        /* Can't provide the coherency needed for MAP_SHARED.
>>> +         * So disable it if FOPEN_DIRECT_IO_SHARED_MMAP is not
>>> +         * set, which means we do need strong coherency.
>>> +         */
>>> +        if (!(ff->open_flags & FOPEN_DIRECT_IO_SHARED_MMAP) &&
>>> +            vma->vm_flags & VM_MAYSHARE)
>>>               return -ENODEV;
>>>             invalidate_inode_pages2(file->f_mapping);
>>> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
>>> index 1b9d0dfae72d..003dcf42e8c2 100644
>>> --- a/include/uapi/linux/fuse.h
>>> +++ b/include/uapi/linux/fuse.h
>>> @@ -314,6 +314,7 @@ struct fuse_file_lock {
>>>    * FOPEN_STREAM: the file is stream-like (no file position at all)
>>>    * FOPEN_NOFLUSH: don't flush data cache on close (unless 
>>> FUSE_WRITEBACK_CACHE)
>>>    * FOPEN_PARALLEL_DIRECT_WRITES: Allow concurrent direct writes on 
>>> the same inode
>>> + * FOPEN_DIRECT_IO_SHARED_MMAP: allow shared mmap when 
>>> FOPEN_DIRECT_IO is set
>>>    */
>>>   #define FOPEN_DIRECT_IO        (1 << 0)
>>>   #define FOPEN_KEEP_CACHE    (1 << 1)
>>> @@ -322,6 +323,7 @@ struct fuse_file_lock {
>>>   #define FOPEN_STREAM        (1 << 4)
>>>   #define FOPEN_NOFLUSH        (1 << 5)
>>>   #define FOPEN_PARALLEL_DIRECT_WRITES    (1 << 6)
>>> +#define FOPEN_DIRECT_IO_SHARED_MMAP    (1 << 7)
>>
>> Thanks, that is what I had in my mind as well.
>>
>> I don't have a strong opinion on it (so don't change it before Miklos 
>> commented), but maybe FOPEN_DIRECT_IO_WEAK? Just in case there would 
>> be later on other conditions that need to be weakened? The comment 
>> would say then something like
>> "Weakens FOPEN_DIRECT_IO enforcement, allows MAP_SHARED mmap"
>>
>> Thanks,
>> Bernd
>>
> 
> Hi Bernd,
> 
> BTW, I have another question:
> 
> ```
> 
>    static int fuse_file_mmap(struct file *file, struct vm_area_struct *vma)
> {
>            struct fuse_file *ff = file->private_data;
> 
>            /* DAX mmap is superior to direct_io mmap */
>            if (FUSE_IS_DAX(file_inode(file)))
>                    return fuse_dax_mmap(file, vma);
> 
>            if (ff->open_flags & FOPEN_DIRECT_IO) {
>                    /* Can't provide the coherency needed for MAP_SHARED */
>                    if (vma->vm_flags & VM_MAYSHARE)
>                            return -ENODEV;
> 
> invalidate_inode_pages2(file->f_mapping);
> 
>                    return generic_file_mmap(file, vma);
> }
> 
>            if ((vma->vm_flags & VM_SHARED) && (vma->vm_flags & 
> VM_MAYWRITE))
> fuse_link_write_file(file);
> 
> file_accessed(file);
>            vma->vm_ops = &fuse_file_vm_ops;
>            return 0;
> }
> 
> ```
> 
> For FOPEN_DIRECT_IO and !FOPEN_DIRECT_IO case, the former set vm_ops to 
> generic_file_vm_ops
> 
> while the latter set it to fuse_file_vm_ops, and also it does the 
> fuse_link_write_file() stuff. Why is so?
> 
> What causes the difference here?

Sorry, this slipped through and I had been busy with other work.

Looks rather similar, I actually wonder if fuse_page_mkwrite() shouldn't 
be replaced with filemap_page_mkwrite. Going back in history, fuse got 
mmap in 2.6.26 and had page_mkwrite method, but 2.6.26 didn't have the 
filemap_page_mkwrite method - when it was added fuse was just not updated?
So that leaves the additional fuse_vma_close, I guess the direct-io code 
path could also use fuse_file_vm_ops.


Bernd