[PATCH v4 0/3] fuse: add a new fuse init flag to relax restrictions in no cache mode

[PATCH v4 0/3] fuse: add a new fuse init flag to relax restrictions in no cache mode

[Hao Xu]

From: Hao Xu <howeyxu@tencent.com>

Patch 1 is a fix for private mmap in FOPEN_DIRECT_IO mode
  This is added here together since the later two depends on it.
Patch 2 is the main dish
Patch 3 is to maintain direct io logic for shared mmap in FOPEN_DIRECT_IO mode

v3 -> v4
    fix race condition for buffered write and direct read by flushing
    pages before direct read to avoid to get stale data

v2 -> v3
    add patch 1 fix here, and adjust it follow Bernd's comment
    add patch 3 which does right thing for shared mmap in FOPEN_DIRECT_IO mode

v1 -> v2:
     make the new flag a fuse init one rather than a open flag since it's
     not common that different files in a filesystem has different
     strategy of shared mmap.


Hao Xu (3):
  fuse: invalidate page cache pages before direct write
  fuse: add a new fuse init flag to relax restrictions in no cache mode
  fuse: write back dirty pages before direct write in direct_io_relax
    mode

 fs/fuse/file.c            | 26 +++++++++++++++++++++++---
 fs/fuse/fuse_i.h          |  3 +++
 fs/fuse/inode.c           |  5 ++++-
 include/uapi/linux/fuse.h |  4 ++++
 4 files changed, 34 insertions(+), 4 deletions(-)


base-commit: 5d0c230f1de8c7515b6567d9afba1f196fb4e2f4
-- 
2.25.1

[PATCH 1/3] fuse: invalidate page cache pages before direct write

[Hao Xu]

From: Hao Xu <howeyxu@tencent.com>

In FOPEN_DIRECT_IO, page cache may still be there for a file since
private mmap is allowed. Direct write should respect that and invalidate
the corresponding pages so that page cache readers don't get stale data.

Signed-off-by: Hao Xu <howeyxu@tencent.com>
---
 fs/fuse/file.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index bc4115288eec..3d320fc99859 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1465,7 +1465,8 @@ ssize_t fuse_direct_io(struct fuse_io_priv *io, struct iov_iter *iter,
 	int write = flags & FUSE_DIO_WRITE;
 	int cuse = flags & FUSE_DIO_CUSE;
 	struct file *file = io->iocb->ki_filp;
-	struct inode *inode = file->f_mapping->host;
+	struct address_space *mapping = file->f_mapping;
+	struct inode *inode = mapping->host;
 	struct fuse_file *ff = file->private_data;
 	struct fuse_conn *fc = ff->fm->fc;
 	size_t nmax = write ? fc->max_write : fc->max_read;
@@ -1477,6 +1478,7 @@ ssize_t fuse_direct_io(struct fuse_io_priv *io, struct iov_iter *iter,
 	int err = 0;
 	struct fuse_io_args *ia;
 	unsigned int max_pages;
+	bool fopen_direct_io = ff->open_flags & FOPEN_DIRECT_IO;
 
 	max_pages = iov_iter_npages(iter, fc->max_pages);
 	ia = fuse_io_alloc(io, max_pages);
@@ -1491,6 +1493,14 @@ ssize_t fuse_direct_io(struct fuse_io_priv *io, struct iov_iter *iter,
 			inode_unlock(inode);
 	}
 
+	if (fopen_direct_io && write) {
+		res = invalidate_inode_pages2_range(mapping, idx_from, idx_to);
+		if (res) {
+			fuse_io_free(ia);
+			return res;
+		}
+	}
+
 	io->should_dirty = !write && user_backed_iter(iter);
 	while (count) {
 		ssize_t nres;
-- 
2.25.1

Re: [PATCH 1/3] fuse: invalidate page cache pages before direct write

[Alan Huang]

> 2023年8月1日 16:06，Hao Xu <hao.xu@linux.dev> 写道：
> 
> From: Hao Xu <howeyxu@tencent.com>
> 
> In FOPEN_DIRECT_IO, page cache may still be there for a file since
> private mmap is allowed. Direct write should respect that and invalidate
> the corresponding pages so that page cache readers don't get stale data.

Do other filesystems also invalidate page cache in this case?

> 
> Signed-off-by: Hao Xu <howeyxu@tencent.com>
> ---
> fs/fuse/file.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index bc4115288eec..3d320fc99859 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -1465,7 +1465,8 @@ ssize_t fuse_direct_io(struct fuse_io_priv *io, struct iov_iter *iter,
> int write = flags & FUSE_DIO_WRITE;
> int cuse = flags & FUSE_DIO_CUSE;
> struct file *file = io->iocb->ki_filp;
> - struct inode *inode = file->f_mapping->host;
> + struct address_space *mapping = file->f_mapping;
> + struct inode *inode = mapping->host;
> struct fuse_file *ff = file->private_data;
> struct fuse_conn *fc = ff->fm->fc;
> size_t nmax = write ? fc->max_write : fc->max_read;
> @@ -1477,6 +1478,7 @@ ssize_t fuse_direct_io(struct fuse_io_priv *io, struct iov_iter *iter,
> int err = 0;
> struct fuse_io_args *ia;
> unsigned int max_pages;
> + bool fopen_direct_io = ff->open_flags & FOPEN_DIRECT_IO;
> 
> max_pages = iov_iter_npages(iter, fc->max_pages);
> ia = fuse_io_alloc(io, max_pages);
> @@ -1491,6 +1493,14 @@ ssize_t fuse_direct_io(struct fuse_io_priv *io, struct iov_iter *iter,
> inode_unlock(inode);
> }
> 
> + if (fopen_direct_io && write) {
> + res = invalidate_inode_pages2_range(mapping, idx_from, idx_to);
> + if (res) {
> + fuse_io_free(ia);
> + return res;
> + }
> + }
> +
> io->should_dirty = !write && user_backed_iter(iter);
> while (count) {
> ssize_t nres;
> -- 
> 2.25.1
> 

Re: [PATCH 1/3] fuse: invalidate page cache pages before direct write

[Hao Xu]

Hi Alan,


On 8/1/23 18:14, Alan Huang wrote:
>> 2023年8月1日 16:06，Hao Xu <hao.xu@linux.dev> 写道：
>>
>> From: Hao Xu <howeyxu@tencent.com>
>>
>> In FOPEN_DIRECT_IO, page cache may still be there for a file since
>> private mmap is allowed. Direct write should respect that and invalidate
>> the corresponding pages so that page cache readers don't get stale data.
> Do other filesystems also invalidate page cache in this case?


For now all filesystems that use iomap do this invalidation, see: 
__iomap_dio_rw()

e.g. ext4, xfs


Regards,

Hao

Re: [PATCH 1/3] fuse: invalidate page cache pages before direct write

[Alan Huang]

> 2023年8月1日 18:40，Hao Xu <hao.xu@linux.dev> 写道：
> 
> Hi Alan,
> 
> 
> On 8/1/23 18:14, Alan Huang wrote:
>>> 2023年8月1日 16:06，Hao Xu <hao.xu@linux.dev> 写道：
>>> 
>>> From: Hao Xu <howeyxu@tencent.com>
>>> 
>>> In FOPEN_DIRECT_IO, page cache may still be there for a file since
>>> private mmap is allowed. Direct write should respect that and invalidate
>>> the corresponding pages so that page cache readers don't get stale data.
>> Do other filesystems also invalidate page cache in this case?
> 
> 
> For now all filesystems that use iomap do this invalidation, see: __iomap_dio_rw()

Thanks!

> 
> e.g. ext4, xfs
> 
> 
> Regards,
> 
> Hao
> 

Re: [PATCH 1/3] fuse: invalidate page cache pages before direct write

[Jiachen Zhang]

On 2023/8/1 16:06, Hao Xu wrote:
> From: Hao Xu <howeyxu@tencent.com>
> 
> In FOPEN_DIRECT_IO, page cache may still be there for a file since
> private mmap is allowed. Direct write should respect that and invalidate
> the corresponding pages so that page cache readers don't get stale data.
> 
> Signed-off-by: Hao Xu <howeyxu@tencent.com>
> ---
>   fs/fuse/file.c | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index bc4115288eec..3d320fc99859 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -1465,7 +1465,8 @@ ssize_t fuse_direct_io(struct fuse_io_priv *io, struct iov_iter *iter,
>   	int write = flags & FUSE_DIO_WRITE;
>   	int cuse = flags & FUSE_DIO_CUSE;
>   	struct file *file = io->iocb->ki_filp;
> -	struct inode *inode = file->f_mapping->host;
> +	struct address_space *mapping = file->f_mapping;
> +	struct inode *inode = mapping->host;
>   	struct fuse_file *ff = file->private_data;
>   	struct fuse_conn *fc = ff->fm->fc;
>   	size_t nmax = write ? fc->max_write : fc->max_read;
> @@ -1477,6 +1478,7 @@ ssize_t fuse_direct_io(struct fuse_io_priv *io, struct iov_iter *iter,
>   	int err = 0;
>   	struct fuse_io_args *ia;
>   	unsigned int max_pages;
> +	bool fopen_direct_io = ff->open_flags & FOPEN_DIRECT_IO;
>   
>   	max_pages = iov_iter_npages(iter, fc->max_pages);
>   	ia = fuse_io_alloc(io, max_pages);
> @@ -1491,6 +1493,14 @@ ssize_t fuse_direct_io(struct fuse_io_priv *io, struct iov_iter *iter,
>   			inode_unlock(inode);
>   	}
>   
> +	if (fopen_direct_io && write) {
> +		res = invalidate_inode_pages2_range(mapping, idx_from, idx_to);
> +		if (res) {
> +			fuse_io_free(ia);
> +			return res;
> +		}
> +	}
> +
>   	io->should_dirty = !write && user_backed_iter(iter);
>   	while (count) {
>   		ssize_t nres;

Tested-by: Jiachen Zhang <zhangjiachen.jaycee@bytedance.com>

Thanks,
Jiachen

[PATCH 2/3] fuse: add a new fuse init flag to relax restrictions in no cache 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 fuse init flag FUSE_DIRECT_IO_RELAX to
relax restrictions in that mode, currently, it allows shared mmap.
One thing to note is to make sure it doesn't break coherency in your
use case.

Signed-off-by: Hao Xu <howeyxu@tencent.com>
---
 fs/fuse/file.c            | 7 +++++--
 fs/fuse/fuse_i.h          | 3 +++
 fs/fuse/inode.c           | 5 ++++-
 include/uapi/linux/fuse.h | 4 ++++
 4 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 3d320fc99859..60f64eafb231 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -2488,14 +2488,17 @@ static const struct vm_operations_struct fuse_file_vm_ops = {
 static int fuse_file_mmap(struct file *file, struct vm_area_struct *vma)
 {
 	struct fuse_file *ff = file->private_data;
+	struct fuse_conn *fc = ff->fm->fc;
 
 	/* 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)
+		/* Can't provide the coherency needed for MAP_SHARED
+		 * if FUSE_DIRECT_IO_RELAX isn't set.
+		 */
+		if ((vma->vm_flags & VM_MAYSHARE) && !fc->direct_io_relax)
 			return -ENODEV;
 
 		invalidate_inode_pages2(file->f_mapping);
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 9b7fc7d3c7f1..d830c2360aef 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -792,6 +792,9 @@ struct fuse_conn {
 	/* Is tmpfile not implemented by fs? */
 	unsigned int no_tmpfile:1;
 
+	/* relax restrictions in FOPEN_DIRECT_IO mode */
+	unsigned int direct_io_relax:1;
+
 	/** The number of requests waiting for completion */
 	atomic_t num_waiting;
 
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index f19d748890f0..53bc9b9a2a75 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -1212,6 +1212,9 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
 				fc->init_security = 1;
 			if (flags & FUSE_CREATE_SUPP_GROUP)
 				fc->create_supp_group = 1;
+
+			if (flags & FUSE_DIRECT_IO_RELAX)
+				fc->direct_io_relax = 1;
 		} else {
 			ra_pages = fc->max_read / PAGE_SIZE;
 			fc->no_lock = 1;
@@ -1258,7 +1261,7 @@ void fuse_send_init(struct fuse_mount *fm)
 		FUSE_NO_OPENDIR_SUPPORT | FUSE_EXPLICIT_INVAL_DATA |
 		FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT | FUSE_INIT_EXT |
 		FUSE_SECURITY_CTX | FUSE_CREATE_SUPP_GROUP |
-		FUSE_HAS_EXPIRE_ONLY;
+		FUSE_HAS_EXPIRE_ONLY | FUSE_DIRECT_IO_RELAX;
 #ifdef CONFIG_FUSE_DAX
 	if (fm->fc->dax)
 		flags |= FUSE_MAP_ALIGNMENT;
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index b3fcab13fcd3..284e6a56c6cd 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -207,6 +207,7 @@
  *  - add FUSE_EXT_GROUPS
  *  - add FUSE_CREATE_SUPP_GROUP
  *  - add FUSE_HAS_EXPIRE_ONLY
+ *  - add FUSE_DIRECT_IO_RELAX
  */
 
 #ifndef _LINUX_FUSE_H
@@ -371,6 +372,8 @@ struct fuse_file_lock {
  * FUSE_CREATE_SUPP_GROUP: add supplementary group info to create, mkdir,
  *			symlink and mknod (single group that matches parent)
  * FUSE_HAS_EXPIRE_ONLY: kernel supports expiry-only entry invalidation
+ * FUSE_DIRECT_IO_RELAX: relax restrictions in FOPEN_DIRECT_IO mode, for now
+ *                       allow shared mmap
  */
 #define FUSE_ASYNC_READ		(1 << 0)
 #define FUSE_POSIX_LOCKS	(1 << 1)
@@ -409,6 +412,7 @@ struct fuse_file_lock {
 #define FUSE_HAS_INODE_DAX	(1ULL << 33)
 #define FUSE_CREATE_SUPP_GROUP	(1ULL << 34)
 #define FUSE_HAS_EXPIRE_ONLY	(1ULL << 35)
+#define FUSE_DIRECT_IO_RELAX	(1ULL << 36)
 
 /**
  * CUSE INIT request/reply flags
-- 
2.25.1

[PATCH 3/3] fuse: write back dirty pages before direct write in direct_io_relax mode

[Hao Xu]

From: Hao Xu <howeyxu@tencent.com>

In direct_io_relax mode, there can be shared mmaped files and thus dirty
pages in its page cache. Therefore those dirty pages should be written
back to backend before direct io to avoid data loss.

Signed-off-by: Hao Xu <howeyxu@tencent.com>
---
 fs/fuse/file.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 60f64eafb231..0bcdf0aafeb7 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1485,6 +1485,13 @@ ssize_t fuse_direct_io(struct fuse_io_priv *io, struct iov_iter *iter,
 	if (!ia)
 		return -ENOMEM;
 
+	if (fopen_direct_io && fc->direct_io_relax) {
+		res = filemap_write_and_wait_range(mapping, pos, pos + count - 1);
+		if (res) {
+			fuse_io_free(ia);
+			return res;
+		}
+	}
 	if (!cuse && fuse_range_is_writeback(inode, idx_from, idx_to)) {
 		if (!write)
 			inode_lock(inode);
-- 
2.25.1

Re: [PATCH 3/3] fuse: write back dirty pages before direct write in direct_io_relax mode

[Jiachen Zhang]

On 2023/8/1 16:06, Hao Xu wrote:
> From: Hao Xu <howeyxu@tencent.com>
> 
> In direct_io_relax mode, there can be shared mmaped files and thus dirty
> pages in its page cache. Therefore those dirty pages should be written
> back to backend before direct io to avoid data loss.
> 
> Signed-off-by: Hao Xu <howeyxu@tencent.com>
> ---
>   fs/fuse/file.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 60f64eafb231..0bcdf0aafeb7 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -1485,6 +1485,13 @@ ssize_t fuse_direct_io(struct fuse_io_priv *io, struct iov_iter *iter,
>   	if (!ia)
>   		return -ENOMEM;
>   
> +	if (fopen_direct_io && fc->direct_io_relax) {
> +		res = filemap_write_and_wait_range(mapping, pos, pos + count - 1);
> +		if (res) {
> +			fuse_io_free(ia);
> +			return res;
> +		}
> +	}
>   	if (!cuse && fuse_range_is_writeback(inode, idx_from, idx_to)) {
>   		if (!write)
>   			inode_lock(inode);


Reviewed-by: Jiachen Zhang <zhangjiachen.jaycee@bytedance.com>

Thanks,
Jiachen

Re: [PATCH v4 0/3] fuse: add a new fuse init flag to relax restrictions in no cache mode

[Miklos Szeredi]

On Tue, 1 Aug 2023 at 10:07, Hao Xu <hao.xu@linux.dev> wrote:
>
> From: Hao Xu <howeyxu@tencent.com>
>
> Patch 1 is a fix for private mmap in FOPEN_DIRECT_IO mode
>   This is added here together since the later two depends on it.
> Patch 2 is the main dish
> Patch 3 is to maintain direct io logic for shared mmap in FOPEN_DIRECT_IO mode

Applied, thanks.

Miklos