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

[Hao Xu <hao.xu@linux.dev>]

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"
> 

make sense for me, thanks, I'll update it in v2 sent after Miklos' review.

> Thanks,
> Bernd
>