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

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

[Hao Xu]

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)
 
 /**
  * INIT request/reply flags
-- 
2.25.1

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

[Bernd Schubert]

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

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

[Hao Xu]

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
> 

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

[Hao Xu]

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?


Thanks,

Hao

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

[Bernd Schubert]

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

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

[Vivek Goyal]

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.

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.

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?

Thanks
Vivek

>  
>  		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)
>  
>  /**
>   * INIT request/reply flags
> -- 
> 2.25.1
> 

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

[Hao Xu]

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.

Thanks,
Hao

> 
> Thanks
> Vivek

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

[Hao Xu]

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
> 

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

[Jingbo Xu]

On 5/6/23 1:01 PM, 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.

IIUC, I guess the current read/write routine will still initiate DIRECT
IO to server in FOPEN_DIRECT_IO mode, even there's page cache initiated
by shared mmap?

Private mmap doesn't need to care about the coherency issue, as private
mmap is private and doesn't need to be flushed to server.  Thus IMHO the
weakened DIRECT_IO, or dio_shared_mmap mode only applies to scenarios
where strong coherency is not needed.

-- 
Thanks,
Jingbo

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

[Hao Xu]

Hi Jingbo,

On 6/13/23 10:56, Jingbo Xu wrote:
> 
> 
> On 5/6/23 1:01 PM, 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.
> 
> IIUC, I guess the current read/write routine will still initiate DIRECT
> IO to server in FOPEN_DIRECT_IO mode, even there's page cache initiated
> by shared mmap?

Yes, currently no matter we private or shared mmap a file in 
FOPEN_DIRECT_IO, when we call syscall write to that file, it goes to 
backend directly, what's worse, it doesn't invalidate the page cache, 
I've filed a patch for it: 
https://lore.kernel.org/linux-fsdevel/0625d0cb-2a65-ffae-b072-e14a3f6c7571@linux.dev/
In this patch, I flush pages regardless the mmap is private or shared,
that will be tweaked in v2. glad if you have time to reviewing.

> 
> Private mmap doesn't need to care about the coherency issue, as private
> mmap is private and doesn't need to be flushed to server.  Thus IMHO the

Yea, but just like what I said, we should invalidate the page cache page 
for private mmaped file.

> weakened DIRECT_IO, or dio_shared_mmap mode only applies to scenarios
> where strong coherency is not needed.
> 

hmmm, not exactly, we should flush the page cache page in that case to 
enforce strong coherency.

Thanks,
Hao

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

[Jingbo Xu]

On 6/13/23 11:20 AM, Hao Xu wrote:
> Hi Jingbo,
> 
> On 6/13/23 10:56, Jingbo Xu wrote:
>>
>>
>> On 5/6/23 1:01 PM, 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.
>>
>> IIUC, I guess the current read/write routine will still initiate DIRECT
>> IO to server in FOPEN_DIRECT_IO mode, even there's page cache initiated
>> by shared mmap?
> 
> Yes, currently no matter we private or shared mmap a file in
> FOPEN_DIRECT_IO, when we call syscall write to that file, it goes to
> backend directly, what's worse, it doesn't invalidate the page cache,
> I've filed a patch for it:
> https://lore.kernel.org/linux-fsdevel/0625d0cb-2a65-ffae-b072-e14a3f6c7571@linux.dev/
> In this patch, I flush pages regardless the mmap is private or shared,
> that will be tweaked in v2. glad if you have time to reviewing.
> 
>>
>> Private mmap doesn't need to care about the coherency issue, as private
>> mmap is private and doesn't need to be flushed to server.  Thus IMHO the
> 
> Yea, but just like what I said, we should invalidate the page cache page
> for private mmaped file.

It's not needed for private mmap.  The private mmaped page is not even
inside inode's address space, and it's not part of the file anymore.
Rather it's more likely the process's anonymous page, though the content
of the page is initially from the file.  It's not inside the address
space and thus it doesn't make any sense to flush or invalidate the
inode's page cache.

Your patch [1] indeed makes sense if the file is writable in shared mmap
mode.  However it seems that the dio_shared_mmap mode only applies to
the read-only scenarios, otherwise I don't know how to resolve the
strong coherency issues as the cache exists in both the guest and server
side.

Please correct me if I'm wrong.


[1]
https://lore.kernel.org/linux-fsdevel/0625d0cb-2a65-ffae-b072-e14a3f6c7571@linux.dev/

-- 
Thanks,
Jingbo

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

[Hao Xu]

On 5/5/23 16: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>


Hi Miklos, any comments on this one?


Thanks,

Hao

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

[Hao Xu]

ping...

On 5/5/23 16: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)
>   
>   /**
>    * INIT request/reply flags

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

[Bernd Schubert]

On 6/8/23 09:17, Hao Xu wrote:
> ping...
> 
> On 5/5/23 16: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)
>>   /**
>>    * INIT request/reply flags
> 
> 
> 

Sorry, currently get distracted by non-fuse work :/

I think see the reply from Miklos on the initial question, which is on 
fuse-devel. Ah, I see you replied to it.

https://sourceforge.net/p/fuse/mailman/message/37849170/


I think what Miklos asks for, is to add a new FUSE_INIT reply flag and 
then to set something like fc->dio_shared_mmap. That way it doesn't need 
to be set for each open, but only once in the server init handler.
@Miklos, please correct me if I'm wrong.


Thanks,
Bernd

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

[Miklos Szeredi]

On Thu, 8 Jun 2023 at 23:29, Bernd Schubert <bernd.schubert@fastmail.fm> wrote:
>
>
>
> On 6/8/23 09:17, Hao Xu wrote:
> > ping...
> >
> > On 5/5/23 16: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)
> >>   /**
> >>    * INIT request/reply flags
> >
> >
> >
>
> Sorry, currently get distracted by non-fuse work :/
>
> I think see the reply from Miklos on the initial question, which is on
> fuse-devel. Ah, I see you replied to it.
>
> https://sourceforge.net/p/fuse/mailman/message/37849170/
>
>
> I think what Miklos asks for, is to add a new FUSE_INIT reply flag and
> then to set something like fc->dio_shared_mmap. That way it doesn't need
> to be set for each open, but only once in the server init handler.
> @Miklos, please correct me if I'm wrong.

Yes, that's exactly what I suggested.

Thanks,
Miklos