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

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

On 5/6/23 13:01, Hao Xu wrote:
> Hi Vivek,
> 
> On 5/6/23 04:37, Vivek Goyal wrote:
>> On Fri, May 05, 2023 at 04:16:52PM +0800, 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;
>>
>> Can you give an example how this is useful and how do you plan to
>> use it?
>>
>> If goal is not using guest cache (either for saving memory or for cache
>> coherency with other clients) and hence you used FOPEN_DIRECT_IO,
>> then by allowing page cache for mmap(), we are contracting that goal.
>> We are neither saving memory and at the same time we are not
>> cache coherent.
> 
> We use it to reduce guest memory "as possible as we can", which means we 
> first have to ensure the functionality so shared mmap should work when 
> users call it, then second reduce memory when users use read/write 
> (from/to other files).
> 
> In cases where users do read/write in most time and calls shared mmap 
> sometimes, disabling shared mmap makes this case out of service, but
> with this flag we still reduce memory and the application works.
> 
>>
>> IIUC, for virtiofs, you want to use cache=none but at the same time
>> allow guest applications to do shared mmap() hence you are introducing
>> this change. There have been folks who have complained about it
>> and I think 9pfs offers a mode which does this. So solving this
>> problem will be nice.
>>
>> BTW, if "-o dax" is used, it solves this problem. But unfortunately qemu
>> does not have DAX support yet and we also have issues with page 
>> truncation
>> on host and MCE not travelling into the guest. So DAX is not a perfect
>> option yet.
> 
> Yea, just like I relied in another mail, users' IO pattern may be a 
> bunch of small IO to a bunch of small files, dax may help but not so 
> much in that case.
> 
>>
>> I agree that solving this problem will be nice. Just trying to
>> understand the behavior better. How these cache pages will
>> interact with read/write?
> 
> I went through the code, it looks like there are issues when users mmap
> a file and then write to it, this may cause coherency problem between 
> the backend file and the frontend page cache.
> I think this problem exists before this patchset: when we private mmap
> a file and then write to it in FOPEN_DIRECT_IO mode, the change doesn't
> update to the page cache because we falsely assume there is no page 
> cache under FOPEN_DIRECT_IO mode. I need to go over the code and do some
> test to see if it is really there and to solve it.

Bug confirmed, in FOPEN_DIRECT_IO mode, if we first private mmap a file,
then do write() syscall to the same file, the page cache page is stale. 
fuse forgets to invalidate page cache page before write().

> 
> Thanks,
> Hao
> 
>>
>> Thanks
>> Vivek
>