[PATCH 0/5 v3] fuse direct write consolidation and parallel IO

[PATCH 0/5 v3] fuse direct write consolidation and parallel IO

[Bernd Schubert]

This series consolidates DIO writes into a single code path via
fuse_cache_write_iter/generic_file_direct_write. Before
it was only used for O_DIRECT and when writeback cache was not enabled.
For server/daemon dio enforcement (FOPEN_DIRECT_IO) another code
path was used before, but I _think_ that is not needed and
just IOCB_DIRECT needs to be set/enforced.
When writeback-cache was enabled another code path was used, with
a fallback to write-through - for direct IO that should not be
needed either.

So far O_DIRECT through fuse_cache_write_iter also took an exclusive
lock, this should not be needed either, at least when server side
sets FOPEN_PARALLEL_DIRECT_WRITES.

v3:
Addresses review comments
  - Rename fuse_direct_write_extending_i_size to io_past_eof
  - Change to single line conditions in fuse_dio_wr_exclusive_lock
    (also fixes accidental parenthesis).
  - Add another patch to rename fuse_direct_io to fuse_send_dio
  - Add detailed information into the commit message of the patch 
    that consolidates IO paths (5/6, previously 4/5) and also 
    update the subject.

v2: 
The entire v1 approach to route DIO writes through fuse_direct_write_iter
was turned around and fuse_direct_write_iter is removed instead and all
DIO writes are now routed through fuse_cache_write_iter

Cc: Hao Xu <howeyxu@tencent.com>
Cc: Christoph Hellwig <hch@infradead.org>
Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: Dharmendra Singh <dsingh@ddn.com>
Cc: linux-fsdevel@vger.kernel.org

Bernd Schubert (5):
  fuse: direct IO can use the write-through code path
  fuse: Create helper function if DIO write needs exclusive lock
  fuse: Allow parallel direct writes for O_DIRECT
  [RFC] fuse: Set and use IOCB_DIRECT when FOPEN_DIRECT_IO is set
  fuse: Remove page flush/invaliation in fuse_direct_io

 fs/fuse/file.c  | 122 ++++++++++++++++--------------------------------
 fs/fuse/xattr.c |   8 ++--
 2 files changed, 43 insertions(+), 87 deletions(-)

-- 
2.39.2

[PATCH 1/6] fuse: direct IO can use the write-through code path

[Bernd Schubert]

Direct IO does not benefit from write back cache and it
also avoides another direct IO write code path.

Cc: Hao Xu <howeyxu@tencent.com>
Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: Dharmendra Singh <dsingh@ddn.com>
Signed-off-by: Bernd Schubert <bschubert@ddn.com>
Acked-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/fuse/file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 1cdb6327511e..b1b9f2b9a37d 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1307,7 +1307,7 @@ static ssize_t fuse_cache_write_iter(struct kiocb *iocb, struct iov_iter *from)
 	ssize_t err;
 	struct fuse_conn *fc = get_fuse_conn(inode);
 
-	if (fc->writeback_cache) {
+	if (fc->writeback_cache && !(iocb->ki_flags & IOCB_DIRECT)) {
 		/* Update size (EOF optimization) and mode (SUID clearing) */
 		err = fuse_update_attributes(mapping->host, file,
 					     STATX_SIZE | STATX_MODE);
-- 
2.39.2

[PATCH 2/6] fuse: Create helper function if DIO write needs exclusive lock

[Bernd Schubert]

This is just a preparation to avoid code duplication in the next
commit.

Cc: Hao Xu <howeyxu@tencent.com>
Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: Dharmendra Singh <dsingh@ddn.com>
Signed-off-by: Bernd Schubert <bschubert@ddn.com>
---
 fs/fuse/file.c | 48 +++++++++++++++++++++++++++++++++---------------
 1 file changed, 33 insertions(+), 15 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index b1b9f2b9a37d..6b8b9512c336 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1298,6 +1298,37 @@ static ssize_t fuse_perform_write(struct kiocb *iocb, struct iov_iter *ii)
 	return res;
 }
 
+static bool fuse_io_past_eof(struct kiocb *iocb,
+					       struct iov_iter *iter)
+{
+	struct inode *inode = file_inode(iocb->ki_filp);
+
+	return iocb->ki_pos + iov_iter_count(iter) > i_size_read(inode);
+}
+
+/*
+ * @return true if an exclusive lock direct IO writes is needed
+ */
+static bool fuse_dio_wr_exclusive_lock(struct kiocb *iocb, struct iov_iter *from)
+{
+	struct file *file = iocb->ki_filp;
+	struct fuse_file *ff = file->private_data;
+
+	/* server side has to advise that it supports parallel dio writes */
+	if (!(ff->open_flags & FOPEN_PARALLEL_DIRECT_WRITES))
+		return false;
+
+	/* append will need to know the eventual eof - always needs a lock */
+	if (iocb->ki_flags & IOCB_APPEND)
+		return false;
+
+	/* parallel dio beyond eof is at least for now not supported */
+	if (fuse_io_past_eof(iocb, from))
+		return false;
+
+	return true;
+}
+
 static ssize_t fuse_cache_write_iter(struct kiocb *iocb, struct iov_iter *from)
 {
 	struct file *file = iocb->ki_filp;
@@ -1557,25 +1588,12 @@ static ssize_t fuse_direct_read_iter(struct kiocb *iocb, struct iov_iter *to)
 	return res;
 }
 
-static bool fuse_direct_write_extending_i_size(struct kiocb *iocb,
-					       struct iov_iter *iter)
-{
-	struct inode *inode = file_inode(iocb->ki_filp);
-
-	return iocb->ki_pos + iov_iter_count(iter) > i_size_read(inode);
-}
-
 static ssize_t fuse_direct_write_iter(struct kiocb *iocb, struct iov_iter *from)
 {
 	struct inode *inode = file_inode(iocb->ki_filp);
-	struct file *file = iocb->ki_filp;
-	struct fuse_file *ff = file->private_data;
 	struct fuse_io_priv io = FUSE_IO_PRIV_SYNC(iocb);
 	ssize_t res;
-	bool exclusive_lock =
-		!(ff->open_flags & FOPEN_PARALLEL_DIRECT_WRITES) ||
-		iocb->ki_flags & IOCB_APPEND ||
-		fuse_direct_write_extending_i_size(iocb, from);
+	bool exclusive_lock = fuse_dio_wr_exclusive_lock(iocb, from);
 
 	/*
 	 * Take exclusive lock if
@@ -1591,7 +1609,7 @@ static ssize_t fuse_direct_write_iter(struct kiocb *iocb, struct iov_iter *from)
 		/* A race with truncate might have come up as the decision for
 		 * the lock type was done without holding the lock, check again.
 		 */
-		if (fuse_direct_write_extending_i_size(iocb, from)) {
+		if (fuse_io_past_eof(iocb, from)) {
 			inode_unlock_shared(inode);
 			inode_lock(inode);
 			exclusive_lock = true;
-- 
2.39.2

[PATCH 3/6] fuse: Allow parallel direct writes for O_DIRECT

[Bernd Schubert]

Take a shared lock in fuse_cache_write_iter. This was already
done for FOPEN_DIRECT_IO in

commit 153524053bbb ("fuse: allow non-extending parallel direct
writes on the same file")

but so far missing for plain O_DIRECT. Server side needs
to set FOPEN_PARALLEL_DIRECT_WRITES in order to signal that
it supports parallel dio writes.

Cc: Hao Xu <howeyxu@tencent.com>
Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: Dharmendra Singh <dsingh@ddn.com>
Cc: linux-fsdevel@vger.kernel.org
Signed-off-by: Bernd Schubert <bschubert@ddn.com>
---
 fs/fuse/file.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 6b8b9512c336..a6b99bc80fe7 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1314,6 +1314,10 @@ static bool fuse_dio_wr_exclusive_lock(struct kiocb *iocb, struct iov_iter *from
 	struct file *file = iocb->ki_filp;
 	struct fuse_file *ff = file->private_data;
 
+	/* this function is about direct IO only */
+	if (!(iocb->ki_flags & IOCB_DIRECT))
+		return false;
+
 	/* server side has to advise that it supports parallel dio writes */
 	if (!(ff->open_flags & FOPEN_PARALLEL_DIRECT_WRITES))
 		return false;
@@ -1337,6 +1341,7 @@ static ssize_t fuse_cache_write_iter(struct kiocb *iocb, struct iov_iter *from)
 	struct inode *inode = mapping->host;
 	ssize_t err;
 	struct fuse_conn *fc = get_fuse_conn(inode);
+	bool excl_lock = fuse_dio_wr_exclusive_lock(iocb, from);
 
 	if (fc->writeback_cache && !(iocb->ki_flags & IOCB_DIRECT)) {
 		/* Update size (EOF optimization) and mode (SUID clearing) */
@@ -1355,7 +1360,10 @@ static ssize_t fuse_cache_write_iter(struct kiocb *iocb, struct iov_iter *from)
 	}
 
 writethrough:
-	inode_lock(inode);
+	if (excl_lock)
+		inode_lock(inode);
+	else
+		inode_lock_shared(inode);
 
 	err = generic_write_checks(iocb, from);
 	if (err <= 0)
@@ -1370,6 +1378,9 @@ static ssize_t fuse_cache_write_iter(struct kiocb *iocb, struct iov_iter *from)
 		goto out;
 
 	if (iocb->ki_flags & IOCB_DIRECT) {
+		/* file extending writes will trigger i_size_write - exclusive
+		 * lock is needed
+		 */
 		written = generic_file_direct_write(iocb, from);
 		if (written < 0 || !iov_iter_count(from))
 			goto out;
@@ -1379,7 +1390,10 @@ static ssize_t fuse_cache_write_iter(struct kiocb *iocb, struct iov_iter *from)
 		written = fuse_perform_write(iocb, from);
 	}
 out:
-	inode_unlock(inode);
+	if (excl_lock)
+		inode_unlock(inode);
+	else
+		inode_unlock_shared(inode);
 	if (written > 0)
 		written = generic_write_sync(iocb, written);
 
-- 
2.39.2

[PATCH 4/6] fuse: Rename fuse_direct_io

[Bernd Schubert]

Just to avoid confusion with fuse_direct_IO.

Cc: Hao Xu <howeyxu@tencent.com>
Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: Dharmendra Singh <dsingh@ddn.com>
Cc: linux-fsdevel@vger.kernel.org
Signed-off-by: Bernd Schubert <bschubert@ddn.com>
---
 fs/fuse/cuse.c   |  5 ++---
 fs/fuse/dax.c    |  2 +-
 fs/fuse/file.c   | 14 +++++++-------
 fs/fuse/fuse_i.h |  8 ++++----
 4 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/fs/fuse/cuse.c b/fs/fuse/cuse.c
index 91e89e68177e..c267ae9dcba6 100644
--- a/fs/fuse/cuse.c
+++ b/fs/fuse/cuse.c
@@ -96,7 +96,7 @@ static ssize_t cuse_read_iter(struct kiocb *kiocb, struct iov_iter *to)
 	struct fuse_io_priv io = FUSE_IO_PRIV_SYNC(kiocb);
 	loff_t pos = 0;
 
-	return fuse_direct_io(&io, to, &pos, FUSE_DIO_CUSE);
+	return fuse_send_dio(&io, to, &pos, FUSE_DIO_CUSE);
 }
 
 static ssize_t cuse_write_iter(struct kiocb *kiocb, struct iov_iter *from)
@@ -107,8 +107,7 @@ static ssize_t cuse_write_iter(struct kiocb *kiocb, struct iov_iter *from)
 	 * No locking or generic_write_checks(), the server is
 	 * responsible for locking and sanity checks.
 	 */
-	return fuse_direct_io(&io, from, &pos,
-			      FUSE_DIO_WRITE | FUSE_DIO_CUSE);
+	return fuse_send_dio(&io, from, &pos, FUSE_DIO_WRITE | FUSE_DIO_CUSE);
 }
 
 static int cuse_open(struct inode *inode, struct file *file)
diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
index 8e74f278a3f6..423e40ab3e31 100644
--- a/fs/fuse/dax.c
+++ b/fs/fuse/dax.c
@@ -731,7 +731,7 @@ static ssize_t fuse_dax_direct_write(struct kiocb *iocb, struct iov_iter *from)
 	struct fuse_io_priv io = FUSE_IO_PRIV_SYNC(iocb);
 	ssize_t ret;
 
-	ret = fuse_direct_io(&io, from, &iocb->ki_pos, FUSE_DIO_WRITE);
+	ret = fuse_send_dio(&io, from, &iocb->ki_pos, FUSE_DIO_WRITE);
 
 	fuse_write_update_attr(inode, iocb->ki_pos, ret);
 	return ret;
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index a6b99bc80fe7..f9d21804d313 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1467,8 +1467,8 @@ static int fuse_get_user_pages(struct fuse_args_pages *ap, struct iov_iter *ii,
 	return ret < 0 ? ret : 0;
 }
 
-ssize_t fuse_direct_io(struct fuse_io_priv *io, struct iov_iter *iter,
-		       loff_t *ppos, int flags)
+ssize_t fuse_send_dio(struct fuse_io_priv *io, struct iov_iter *iter,
+		      loff_t *ppos, int flags)
 {
 	int write = flags & FUSE_DIO_WRITE;
 	int cuse = flags & FUSE_DIO_CUSE;
@@ -1569,7 +1569,7 @@ ssize_t fuse_direct_io(struct fuse_io_priv *io, struct iov_iter *iter,
 
 	return res > 0 ? res : err;
 }
-EXPORT_SYMBOL_GPL(fuse_direct_io);
+EXPORT_SYMBOL_GPL(fuse_send_dio);
 
 static ssize_t __fuse_direct_read(struct fuse_io_priv *io,
 				  struct iov_iter *iter,
@@ -1578,7 +1578,7 @@ static ssize_t __fuse_direct_read(struct fuse_io_priv *io,
 	ssize_t res;
 	struct inode *inode = file_inode(io->iocb->ki_filp);
 
-	res = fuse_direct_io(io, iter, ppos, 0);
+	res = fuse_send_dio(io, iter, ppos, 0);
 
 	fuse_invalidate_atime(inode);
 
@@ -1635,8 +1635,8 @@ static ssize_t fuse_direct_write_iter(struct kiocb *iocb, struct iov_iter *from)
 		if (!is_sync_kiocb(iocb) && iocb->ki_flags & IOCB_DIRECT) {
 			res = fuse_direct_IO(iocb, from);
 		} else {
-			res = fuse_direct_io(&io, from, &iocb->ki_pos,
-					     FUSE_DIO_WRITE);
+			res = fuse_send_dio(&io, from, &iocb->ki_pos,
+					    FUSE_DIO_WRITE);
 			fuse_write_update_attr(inode, iocb->ki_pos, res);
 		}
 	}
@@ -2972,7 +2972,7 @@ fuse_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
 	}
 
 	if (iov_iter_rw(iter) == WRITE) {
-		ret = fuse_direct_io(io, iter, &pos, FUSE_DIO_WRITE);
+		ret = fuse_send_dio(io, iter, &pos, FUSE_DIO_WRITE);
 		fuse_invalidate_attr_mask(inode, FUSE_STATX_MODSIZE);
 	} else {
 		ret = __fuse_direct_read(io, iter, &pos);
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index bf0b85d0b95c..05c5cae59bad 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -1247,17 +1247,17 @@ int fuse_do_open(struct fuse_mount *fm, u64 nodeid, struct file *file,
 		 bool isdir);
 
 /**
- * fuse_direct_io() flags
+ * fuse_send_dio() flags
  */
 
 /** If set, it is WRITE; otherwise - READ */
 #define FUSE_DIO_WRITE (1 << 0)
 
-/** CUSE pass fuse_direct_io() a file which f_mapping->host is not from FUSE */
+/** CUSE pass fuse_send_dio() a file which f_mapping->host is not from FUSE */
 #define FUSE_DIO_CUSE  (1 << 1)
 
-ssize_t fuse_direct_io(struct fuse_io_priv *io, struct iov_iter *iter,
-		       loff_t *ppos, int flags);
+ssize_t fuse_send_dio(struct fuse_io_priv *io, struct iov_iter *iter,
+		      loff_t *ppos, int flags);
 long fuse_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg,
 		   unsigned int flags);
 long fuse_ioctl_common(struct file *file, unsigned int cmd,
-- 
2.39.2

[PATCH 5/6] fuse: Remove fuse_direct_write_iter code path / use IOCB_DIRECT

[Bernd Schubert]

fuse_direct_write_iter is basically duplicating what is already
in fuse_cache_write_iter/generic_file_direct_write. That can be
avoided by setting IOCB_DIRECT in fuse_file_write_iter, after that
fuse_cache_write_iter can be used for the FOPEN_DIRECT_IO code path
and fuse_direct_write_iter can be removed.

Before it was using for FOPEN_DIRECT_IO

1) async (!is_sync_kiocb(iocb)) && IOCB_DIRECT

fuse_file_write_iter
    fuse_direct_write_iter
        fuse_direct_IO
            fuse_send_dio

2) sync (is_sync_kiocb(iocb)) or IOCB_DIRECT not being set

fuse_file_write_iter
    fuse_direct_write_iter
        fuse_send_dio

3) FOPEN_DIRECT_IO not set

Same as the consolidates path below

The new consolidated code path is always

fuse_file_write_iter
    fuse_cache_write_iter
        generic_file_write_iter
             __generic_file_write_iter
                 generic_file_direct_write
                     mapping->a_ops->direct_IO / fuse_direct_IO
                         fuse_send_dio

So in general for FOPEN_DIRECT_IO the code path gets longer. Additionally
fuse_direct_IO does an allocation of struct fuse_io_priv - might be a bit
slower in micro benchmarks.
Also, the IOCB_DIRECT information gets lost (as we now set it outselves).
But then IOCB_DIRECT is directly related to O_DIRECT set in
struct file::f_flags.

An additional change is for condition 2 above, which might will now do
async IO on the condition ff->fm->fc->async_dio. Given that async IO for
FOPEN_DIRECT_IO was especially introduced in commit
'commit 23c94e1cdcbf ("fuse: Switch to using async direct IO for
 FOPEN_DIRECT_IO")'
it should not matter. Especially as fuse_direct_IO is blocking for
is_sync_kiocb(), at worst it has another slight overhead.

Advantage is the removal of code paths and conditions and it is now also
possible to remove FOPEN_DIRECT_IO conditions in fuse_send_dio
(in a later patch).

Cc: Hao Xu <howeyxu@tencent.com>
Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: Dharmendra Singh <dsingh@ddn.com>
Cc: linux-fsdevel@vger.kernel.org
Signed-off-by: Bernd Schubert <bschubert@ddn.com>
---
 fs/fuse/file.c | 54 ++++----------------------------------------------
 1 file changed, 4 insertions(+), 50 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index f9d21804d313..0b3363eec435 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1602,52 +1602,6 @@ static ssize_t fuse_direct_read_iter(struct kiocb *iocb, struct iov_iter *to)
 	return res;
 }
 
-static ssize_t fuse_direct_write_iter(struct kiocb *iocb, struct iov_iter *from)
-{
-	struct inode *inode = file_inode(iocb->ki_filp);
-	struct fuse_io_priv io = FUSE_IO_PRIV_SYNC(iocb);
-	ssize_t res;
-	bool exclusive_lock = fuse_dio_wr_exclusive_lock(iocb, from);
-
-	/*
-	 * Take exclusive lock if
-	 * - Parallel direct writes are disabled - a user space decision
-	 * - Parallel direct writes are enabled and i_size is being extended.
-	 *   This might not be needed at all, but needs further investigation.
-	 */
-	if (exclusive_lock)
-		inode_lock(inode);
-	else {
-		inode_lock_shared(inode);
-
-		/* A race with truncate might have come up as the decision for
-		 * the lock type was done without holding the lock, check again.
-		 */
-		if (fuse_io_past_eof(iocb, from)) {
-			inode_unlock_shared(inode);
-			inode_lock(inode);
-			exclusive_lock = true;
-		}
-	}
-
-	res = generic_write_checks(iocb, from);
-	if (res > 0) {
-		if (!is_sync_kiocb(iocb) && iocb->ki_flags & IOCB_DIRECT) {
-			res = fuse_direct_IO(iocb, from);
-		} else {
-			res = fuse_send_dio(&io, from, &iocb->ki_pos,
-					    FUSE_DIO_WRITE);
-			fuse_write_update_attr(inode, iocb->ki_pos, res);
-		}
-	}
-	if (exclusive_lock)
-		inode_unlock(inode);
-	else
-		inode_unlock_shared(inode);
-
-	return res;
-}
-
 static ssize_t fuse_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
 {
 	struct file *file = iocb->ki_filp;
@@ -1678,10 +1632,10 @@ static ssize_t fuse_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 	if (FUSE_IS_DAX(inode))
 		return fuse_dax_write_iter(iocb, from);
 
-	if (!(ff->open_flags & FOPEN_DIRECT_IO))
-		return fuse_cache_write_iter(iocb, from);
-	else
-		return fuse_direct_write_iter(iocb, from);
+	if (ff->open_flags & FOPEN_DIRECT_IO)
+		iocb->ki_flags |= IOCB_DIRECT;
+
+	return fuse_cache_write_iter(iocb, from);
 }
 
 static void fuse_writepage_free(struct fuse_writepage_args *wpa)
-- 
2.39.2

[PATCH 6/6] fuse: Remove page flush/invaliation in fuse_direct_io

[Bernd Schubert]

Page flush and invalidation in fuse_direct_io can when FOPEN_DIRECT_IO
is set can be removed, as the code path is now always via
generic_file_direct_write, which already does it.

Cc: Hao Xu <howeyxu@tencent.com>
Cc: Miklos Szeredi <miklos@szeredi.hu>
Cc: Dharmendra Singh <dsingh@ddn.com>
Cc: linux-fsdevel@vger.kernel.org
Signed-off-by: Bernd Schubert <bschubert@ddn.com>
Acked-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/fuse/file.c | 17 -----------------
 1 file changed, 17 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 0b3363eec435..eaafa3fb0e0c 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1486,20 +1486,11 @@ ssize_t fuse_send_dio(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);
 	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);
@@ -1508,14 +1499,6 @@ ssize_t fuse_send_dio(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.39.2

Re: [PATCH 2/6] fuse: Create helper function if DIO write needs exclusive lock

[Miklos Szeredi]

On Tue, 29 Aug 2023 at 18:11, Bernd Schubert <bschubert@ddn.com> wrote:
>
> This is just a preparation to avoid code duplication in the next
> commit.
>
> Cc: Hao Xu <howeyxu@tencent.com>
> Cc: Miklos Szeredi <miklos@szeredi.hu>
> Cc: Dharmendra Singh <dsingh@ddn.com>
> Signed-off-by: Bernd Schubert <bschubert@ddn.com>
> ---
>  fs/fuse/file.c | 48 +++++++++++++++++++++++++++++++++---------------
>  1 file changed, 33 insertions(+), 15 deletions(-)
>
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index b1b9f2b9a37d..6b8b9512c336 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -1298,6 +1298,37 @@ static ssize_t fuse_perform_write(struct kiocb *iocb, struct iov_iter *ii)
>         return res;
>  }
>
> +static bool fuse_io_past_eof(struct kiocb *iocb,
> +                                              struct iov_iter *iter)
> +{
> +       struct inode *inode = file_inode(iocb->ki_filp);
> +
> +       return iocb->ki_pos + iov_iter_count(iter) > i_size_read(inode);
> +}
> +
> +/*
> + * @return true if an exclusive lock direct IO writes is needed
> + */
> +static bool fuse_dio_wr_exclusive_lock(struct kiocb *iocb, struct iov_iter *from)
> +{
> +       struct file *file = iocb->ki_filp;
> +       struct fuse_file *ff = file->private_data;
> +
> +       /* server side has to advise that it supports parallel dio writes */
> +       if (!(ff->open_flags & FOPEN_PARALLEL_DIRECT_WRITES))
> +               return false;

You got the return values the wrong way around.  I can fix this, no
need to resend.

Thanks,
Miklos

Re: [PATCH 2/6] fuse: Create helper function if DIO write needs exclusive lock

[Bernd Schubert]

On 8/30/23 12:57, Miklos Szeredi wrote:
> On Tue, 29 Aug 2023 at 18:11, Bernd Schubert <bschubert@ddn.com> wrote:
>>
>> This is just a preparation to avoid code duplication in the next
>> commit.
>>
>> Cc: Hao Xu <howeyxu@tencent.com>
>> Cc: Miklos Szeredi <miklos@szeredi.hu>
>> Cc: Dharmendra Singh <dsingh@ddn.com>
>> Signed-off-by: Bernd Schubert <bschubert@ddn.com>
>> ---
>>   fs/fuse/file.c | 48 +++++++++++++++++++++++++++++++++---------------
>>   1 file changed, 33 insertions(+), 15 deletions(-)
>>
>> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
>> index b1b9f2b9a37d..6b8b9512c336 100644
>> --- a/fs/fuse/file.c
>> +++ b/fs/fuse/file.c
>> @@ -1298,6 +1298,37 @@ static ssize_t fuse_perform_write(struct kiocb *iocb, struct iov_iter *ii)
>>          return res;
>>   }
>>
>> +static bool fuse_io_past_eof(struct kiocb *iocb,
>> +                                              struct iov_iter *iter)
>> +{
>> +       struct inode *inode = file_inode(iocb->ki_filp);
>> +
>> +       return iocb->ki_pos + iov_iter_count(iter) > i_size_read(inode);
>> +}
>> +
>> +/*
>> + * @return true if an exclusive lock direct IO writes is needed
>> + */
>> +static bool fuse_dio_wr_exclusive_lock(struct kiocb *iocb, struct iov_iter *from)
>> +{
>> +       struct file *file = iocb->ki_filp;
>> +       struct fuse_file *ff = file->private_data;
>> +
>> +       /* server side has to advise that it supports parallel dio writes */
>> +       if (!(ff->open_flags & FOPEN_PARALLEL_DIRECT_WRITES))
>> +               return false;
> 
> You got the return values the wrong way around.  I can fix this, no
> need to resend.

Ooops, sorry! Do you mind to take this series for next merge round? I 
obviously didn't test the latest series yet and I would like to first 
test performance and do several rounds of xfs tests. That should be done 
by Monday, but might be a bit late for 6.6


Thanks,
Bernd

Re: [PATCH 2/6] fuse: Create helper function if DIO write needs exclusive lock

[Miklos Szeredi]

On Wed, 30 Aug 2023 at 14:13, Bernd Schubert <aakef@fastmail.fm> wrote:
>
>
>
> On 8/30/23 12:57, Miklos Szeredi wrote:
> > On Tue, 29 Aug 2023 at 18:11, Bernd Schubert <bschubert@ddn.com> wrote:
> >>
> >> This is just a preparation to avoid code duplication in the next
> >> commit.
> >>
> >> Cc: Hao Xu <howeyxu@tencent.com>
> >> Cc: Miklos Szeredi <miklos@szeredi.hu>
> >> Cc: Dharmendra Singh <dsingh@ddn.com>
> >> Signed-off-by: Bernd Schubert <bschubert@ddn.com>
> >> ---
> >>   fs/fuse/file.c | 48 +++++++++++++++++++++++++++++++++---------------
> >>   1 file changed, 33 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> >> index b1b9f2b9a37d..6b8b9512c336 100644
> >> --- a/fs/fuse/file.c
> >> +++ b/fs/fuse/file.c
> >> @@ -1298,6 +1298,37 @@ static ssize_t fuse_perform_write(struct kiocb *iocb, struct iov_iter *ii)
> >>          return res;
> >>   }
> >>
> >> +static bool fuse_io_past_eof(struct kiocb *iocb,
> >> +                                              struct iov_iter *iter)
> >> +{
> >> +       struct inode *inode = file_inode(iocb->ki_filp);
> >> +
> >> +       return iocb->ki_pos + iov_iter_count(iter) > i_size_read(inode);
> >> +}
> >> +
> >> +/*
> >> + * @return true if an exclusive lock direct IO writes is needed
> >> + */
> >> +static bool fuse_dio_wr_exclusive_lock(struct kiocb *iocb, struct iov_iter *from)
> >> +{
> >> +       struct file *file = iocb->ki_filp;
> >> +       struct fuse_file *ff = file->private_data;
> >> +
> >> +       /* server side has to advise that it supports parallel dio writes */
> >> +       if (!(ff->open_flags & FOPEN_PARALLEL_DIRECT_WRITES))
> >> +               return false;
> >
> > You got the return values the wrong way around.  I can fix this, no
> > need to resend.
>
> Ooops, sorry! Do you mind to take this series for next merge round? I
> obviously didn't test the latest series yet and I would like to first
> test performance and do several rounds of xfs tests. That should be done
> by Monday, but might be a bit late for 6.6

Right, this should aim for 6.7.

Thanks,
Miklos

Re: [PATCH 3/6] fuse: Allow parallel direct writes for O_DIRECT

[Miklos Szeredi]

On Tue, 29 Aug 2023 at 18:11, Bernd Schubert <bschubert@ddn.com> wrote:
>
> Take a shared lock in fuse_cache_write_iter. This was already
> done for FOPEN_DIRECT_IO in
>
> commit 153524053bbb ("fuse: allow non-extending parallel direct
> writes on the same file")
>
> but so far missing for plain O_DIRECT. Server side needs
> to set FOPEN_PARALLEL_DIRECT_WRITES in order to signal that
> it supports parallel dio writes.

Hmm, I think file_remove_privs() needs exclusive lock (due to calling
notify_change()).   And the fallback case also.

Need to be careful with such locking changes...

Thanks,
Miklos

Re: [PATCH 3/6] fuse: Allow parallel direct writes for O_DIRECT

[Bernd Schubert]

On 8/30/23 15:28, Miklos Szeredi wrote:
> On Tue, 29 Aug 2023 at 18:11, Bernd Schubert <bschubert@ddn.com> wrote:
>>
>> Take a shared lock in fuse_cache_write_iter. This was already
>> done for FOPEN_DIRECT_IO in
>>
>> commit 153524053bbb ("fuse: allow non-extending parallel direct
>> writes on the same file")
>>
>> but so far missing for plain O_DIRECT. Server side needs
>> to set FOPEN_PARALLEL_DIRECT_WRITES in order to signal that
>> it supports parallel dio writes.
> 
> Hmm, I think file_remove_privs() needs exclusive lock (due to calling
> notify_change()).   And the fallback case also.
> 
> Need to be careful with such locking changes...

Hrmm, yeah, I missed that :( Really sorry and thanks!

I guess I can fix it if by exporting dentry_needs_remove_privs. I hope 
that is acceptable. Interesting is that btrfs_direct_write seems to have 
the same issue.

btrfs_direct_write
	if (iocb->ki_pos + iov_iter_count(from) <= i_size_read(inode))
		ilock_flags |= BTRFS_ILOCK_SHARED;
...
	err = btrfs_inode_lock(BTRFS_I(inode), ilock_flags);
...	
	err = btrfs_write_check(iocb, from, err);
...
			ret = file_remove_privs(file);


I think that can be fixed as well after exporting 
dentry_needs_remove_privs().


Btw, why is fuse_direct_write_iter not dropping privileges? That is 
another change I need to document...


Another issue I just see is that it needs to check file size again after 
taking the lock.


Thanks a lot for your review,
Bernd

Re: [PATCH 3/6] fuse: Allow parallel direct writes for O_DIRECT

[Miklos Szeredi]

On Wed, 30 Aug 2023 at 16:38, Bernd Schubert <aakef@fastmail.fm> wrote:
>
>
>
> On 8/30/23 15:28, Miklos Szeredi wrote:
> > On Tue, 29 Aug 2023 at 18:11, Bernd Schubert <bschubert@ddn.com> wrote:
> >>
> >> Take a shared lock in fuse_cache_write_iter. This was already
> >> done for FOPEN_DIRECT_IO in
> >>
> >> commit 153524053bbb ("fuse: allow non-extending parallel direct
> >> writes on the same file")
> >>
> >> but so far missing for plain O_DIRECT. Server side needs
> >> to set FOPEN_PARALLEL_DIRECT_WRITES in order to signal that
> >> it supports parallel dio writes.
> >
> > Hmm, I think file_remove_privs() needs exclusive lock (due to calling
> > notify_change()).   And the fallback case also.
> >
> > Need to be careful with such locking changes...
>
> Hrmm, yeah, I missed that :( Really sorry and thanks!
>
> I guess I can fix it if by exporting dentry_needs_remove_privs. I hope
> that is acceptable. Interesting is that btrfs_direct_write seems to have
> the same issue.
>
> btrfs_direct_write
>         if (iocb->ki_pos + iov_iter_count(from) <= i_size_read(inode))
>                 ilock_flags |= BTRFS_ILOCK_SHARED;
> ...
>         err = btrfs_inode_lock(BTRFS_I(inode), ilock_flags);
> ...
>         err = btrfs_write_check(iocb, from, err);
> ...
>                         ret = file_remove_privs(file);
>
>
> I think that can be fixed as well after exporting
> dentry_needs_remove_privs().

Interesting.  Would be worth asking the btrfs devs what they think of this.

>
>
> Btw, why is fuse_direct_write_iter not dropping privileges? That is
> another change I need to document...

Generally read and write can't fail due to lack of privileges, so that
should be okay.  But it would be more consistent to drop privs during
I/O as well.  But this is something that needs some thought as well,
because there could be non-obvious consequences.

> Another issue I just see is that it needs to check file size again after
> taking the lock.

Yes.

> Thanks a lot for your review,

Thanks for doing the actual work, that's the harder one.

Thanks,
Miklos

Re: [PATCH 3/6] fuse: Allow parallel direct writes for O_DIRECT

[Hao Xu]

On 8/30/23 00:11, Bernd Schubert wrote:
> Take a shared lock in fuse_cache_write_iter. This was already
> done for FOPEN_DIRECT_IO in
> 
> commit 153524053bbb ("fuse: allow non-extending parallel direct
> writes on the same file")
> 
> but so far missing for plain O_DIRECT. Server side needs
> to set FOPEN_PARALLEL_DIRECT_WRITES in order to signal that
> it supports parallel dio writes.
> 
> Cc: Hao Xu <howeyxu@tencent.com>
> Cc: Miklos Szeredi <miklos@szeredi.hu>
> Cc: Dharmendra Singh <dsingh@ddn.com>
> Cc: linux-fsdevel@vger.kernel.org
> Signed-off-by: Bernd Schubert <bschubert@ddn.com>
> ---
>   fs/fuse/file.c | 18 ++++++++++++++++--
>   1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 6b8b9512c336..a6b99bc80fe7 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -1314,6 +1314,10 @@ static bool fuse_dio_wr_exclusive_lock(struct kiocb *iocb, struct iov_iter *from
>   	struct file *file = iocb->ki_filp;
>   	struct fuse_file *ff = file->private_data;
>   
> +	/* this function is about direct IO only */
> +	if (!(iocb->ki_flags & IOCB_DIRECT))
> +		return false;

This means for buffered write in fuse_cache_write_iter(), we grab shared 
lock, looks not right.

> +
>   	/* server side has to advise that it supports parallel dio writes */
>   	if (!(ff->open_flags & FOPEN_PARALLEL_DIRECT_WRITES))
>   		return false;
> @@ -1337,6 +1341,7 @@ static ssize_t fuse_cache_write_iter(struct kiocb *iocb, struct iov_iter *from)
>   	struct inode *inode = mapping->host;
>   	ssize_t err;
>   	struct fuse_conn *fc = get_fuse_conn(inode);
> +	bool excl_lock = fuse_dio_wr_exclusive_lock(iocb, from);
>   
>   	if (fc->writeback_cache && !(iocb->ki_flags & IOCB_DIRECT)) {
>   		/* Update size (EOF optimization) and mode (SUID clearing) */
> @@ -1355,7 +1360,10 @@ static ssize_t fuse_cache_write_iter(struct kiocb *iocb, struct iov_iter *from)
>   	}
>   
>   writethrough:
> -	inode_lock(inode);
> +	if (excl_lock)
> +		inode_lock(inode);
> +	else
> +		inode_lock_shared(inode);
>   
>   	err = generic_write_checks(iocb, from);
>   	if (err <= 0)
> @@ -1370,6 +1378,9 @@ static ssize_t fuse_cache_write_iter(struct kiocb *iocb, struct iov_iter *from)
>   		goto out;
>   
>   	if (iocb->ki_flags & IOCB_DIRECT) {
> +		/* file extending writes will trigger i_size_write - exclusive
> +		 * lock is needed
> +		 */
>   		written = generic_file_direct_write(iocb, from);
>   		if (written < 0 || !iov_iter_count(from))
>   			goto out;
> @@ -1379,7 +1390,10 @@ static ssize_t fuse_cache_write_iter(struct kiocb *iocb, struct iov_iter *from)
>   		written = fuse_perform_write(iocb, from);
>   	}
>   out:
> -	inode_unlock(inode);
> +	if (excl_lock)
> +		inode_unlock(inode);
> +	else
> +		inode_unlock_shared(inode);
>   	if (written > 0)
>   		written = generic_write_sync(iocb, written);
>   

Re: [PATCH 3/6] fuse: Allow parallel direct writes for O_DIRECT

[Bernd Schubert]

On 8/31/23 10:30, Hao Xu wrote:
> On 8/30/23 00:11, Bernd Schubert wrote:
>> Take a shared lock in fuse_cache_write_iter. This was already
>> done for FOPEN_DIRECT_IO in
>>
>> commit 153524053bbb ("fuse: allow non-extending parallel direct
>> writes on the same file")
>>
>> but so far missing for plain O_DIRECT. Server side needs
>> to set FOPEN_PARALLEL_DIRECT_WRITES in order to signal that
>> it supports parallel dio writes.
>>
>> Cc: Hao Xu <howeyxu@tencent.com>
>> Cc: Miklos Szeredi <miklos@szeredi.hu>
>> Cc: Dharmendra Singh <dsingh@ddn.com>
>> Cc: linux-fsdevel@vger.kernel.org
>> Signed-off-by: Bernd Schubert <bschubert@ddn.com>
>> ---
>>   fs/fuse/file.c | 18 ++++++++++++++++--
>>   1 file changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
>> index 6b8b9512c336..a6b99bc80fe7 100644
>> --- a/fs/fuse/file.c
>> +++ b/fs/fuse/file.c
>> @@ -1314,6 +1314,10 @@ static bool fuse_dio_wr_exclusive_lock(struct 
>> kiocb *iocb, struct iov_iter *from
>>       struct file *file = iocb->ki_filp;
>>       struct fuse_file *ff = file->private_data;
>> +    /* this function is about direct IO only */
>> +    if (!(iocb->ki_flags & IOCB_DIRECT))
>> +        return false;
> 
> This means for buffered write in fuse_cache_write_iter(), we grab shared 
> lock, looks not right.
> 

Yeah, sorry, I made all values the other way around, consistently. 
Miklos had already noticed.


Thanks,
Bernd

Re: [PATCH 5/6] fuse: Remove fuse_direct_write_iter code path / use IOCB_DIRECT

[Hao Xu]

On 8/30/23 00:11, Bernd Schubert wrote:
> fuse_direct_write_iter is basically duplicating what is already
> in fuse_cache_write_iter/generic_file_direct_write. That can be
> avoided by setting IOCB_DIRECT in fuse_file_write_iter, after that
> fuse_cache_write_iter can be used for the FOPEN_DIRECT_IO code path
> and fuse_direct_write_iter can be removed.
> 
> Before it was using for FOPEN_DIRECT_IO
> 
> 1) async (!is_sync_kiocb(iocb)) && IOCB_DIRECT
> 
> fuse_file_write_iter
>      fuse_direct_write_iter
>          fuse_direct_IO
>              fuse_send_dio
> 
> 2) sync (is_sync_kiocb(iocb)) or IOCB_DIRECT not being set
> 
> fuse_file_write_iter
>      fuse_direct_write_iter
>          fuse_send_dio
> 
> 3) FOPEN_DIRECT_IO not set
> 
> Same as the consolidates path below
> 
> The new consolidated code path is always
> 
> fuse_file_write_iter
>      fuse_cache_write_iter
>          generic_file_write_iter
>               __generic_file_write_iter
>                   generic_file_direct_write
>                       mapping->a_ops->direct_IO / fuse_direct_IO
>                           fuse_send_dio
> 
> So in general for FOPEN_DIRECT_IO the code path gets longer. Additionally
> fuse_direct_IO does an allocation of struct fuse_io_priv - might be a bit
> slower in micro benchmarks.
> Also, the IOCB_DIRECT information gets lost (as we now set it outselves).
> But then IOCB_DIRECT is directly related to O_DIRECT set in
> struct file::f_flags.
> 
> An additional change is for condition 2 above, which might will now do
> async IO on the condition ff->fm->fc->async_dio. Given that async IO for
> FOPEN_DIRECT_IO was especially introduced in commit
> 'commit 23c94e1cdcbf ("fuse: Switch to using async direct IO for
>   FOPEN_DIRECT_IO")'
> it should not matter. Especially as fuse_direct_IO is blocking for
> is_sync_kiocb(), at worst it has another slight overhead.
> 
> Advantage is the removal of code paths and conditions and it is now also
> possible to remove FOPEN_DIRECT_IO conditions in fuse_send_dio
> (in a later patch).
> 
> Cc: Hao Xu <howeyxu@tencent.com>
> Cc: Miklos Szeredi <miklos@szeredi.hu>
> Cc: Dharmendra Singh <dsingh@ddn.com>
> Cc: linux-fsdevel@vger.kernel.org
> Signed-off-by: Bernd Schubert <bschubert@ddn.com>
> ---
>   fs/fuse/file.c | 54 ++++----------------------------------------------
>   1 file changed, 4 insertions(+), 50 deletions(-)
> 
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index f9d21804d313..0b3363eec435 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -1602,52 +1602,6 @@ static ssize_t fuse_direct_read_iter(struct kiocb *iocb, struct iov_iter *to)
>   	return res;
>   }
>   
> -static ssize_t fuse_direct_write_iter(struct kiocb *iocb, struct iov_iter *from)
> -{
> -	struct inode *inode = file_inode(iocb->ki_filp);
> -	struct fuse_io_priv io = FUSE_IO_PRIV_SYNC(iocb);
> -	ssize_t res;
> -	bool exclusive_lock = fuse_dio_wr_exclusive_lock(iocb, from);
> -
> -	/*
> -	 * Take exclusive lock if
> -	 * - Parallel direct writes are disabled - a user space decision
> -	 * - Parallel direct writes are enabled and i_size is being extended.
> -	 *   This might not be needed at all, but needs further investigation.
> -	 */
> -	if (exclusive_lock)
> -		inode_lock(inode);
> -	else {
> -		inode_lock_shared(inode);
> -
> -		/* A race with truncate might have come up as the decision for
> -		 * the lock type was done without holding the lock, check again.
> -		 */
> -		if (fuse_io_past_eof(iocb, from)) {
> -			inode_unlock_shared(inode);
> -			inode_lock(inode);
> -			exclusive_lock = true;
> -		}
> -	}
> -
> -	res = generic_write_checks(iocb, from);
> -	if (res > 0) {
> -		if (!is_sync_kiocb(iocb) && iocb->ki_flags & IOCB_DIRECT) {
> -			res = fuse_direct_IO(iocb, from);
> -		} else {
> -			res = fuse_send_dio(&io, from, &iocb->ki_pos,
> -					    FUSE_DIO_WRITE);
> -			fuse_write_update_attr(inode, iocb->ki_pos, res);
> -		}
> -	}
> -	if (exclusive_lock)
> -		inode_unlock(inode);
> -	else
> -		inode_unlock_shared(inode);
> -
> -	return res;
> -}
> -
>   static ssize_t fuse_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
>   {
>   	struct file *file = iocb->ki_filp;
> @@ -1678,10 +1632,10 @@ static ssize_t fuse_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
>   	if (FUSE_IS_DAX(inode))
>   		return fuse_dax_write_iter(iocb, from);
>   
> -	if (!(ff->open_flags & FOPEN_DIRECT_IO))
> -		return fuse_cache_write_iter(iocb, from);
> -	else
> -		return fuse_direct_write_iter(iocb, from);
> +	if (ff->open_flags & FOPEN_DIRECT_IO)
> +		iocb->ki_flags |= IOCB_DIRECT;

I think this affect the back-end behavior, changes a buffered IO to 
direct io. FOPEN_DIRECT_IO means no cache in front-end, but we should
respect the back-end semantics. We may need another way to indicate
"we need go the direct io code path while IOCB_DIRECT is not set though".

> +
> +	return fuse_cache_write_iter(iocb, from);
>   }
>   
>   static void fuse_writepage_free(struct fuse_writepage_args *wpa)

Re: [PATCH 5/6] fuse: Remove fuse_direct_write_iter code path / use IOCB_DIRECT

[Bernd Schubert]

On 8/31/23 11:19, Hao Xu wrote:
> On 8/30/23 00:11, Bernd Schubert wrote:
>> fuse_direct_write_iter is basically duplicating what is already
>> in fuse_cache_write_iter/generic_file_direct_write. That can be
>> avoided by setting IOCB_DIRECT in fuse_file_write_iter, after that
>> fuse_cache_write_iter can be used for the FOPEN_DIRECT_IO code path
>> and fuse_direct_write_iter can be removed.
>>
>> Before it was using for FOPEN_DIRECT_IO
>>
>> 1) async (!is_sync_kiocb(iocb)) && IOCB_DIRECT
>>
>> fuse_file_write_iter
>>      fuse_direct_write_iter
>>          fuse_direct_IO
>>              fuse_send_dio
>>
>> 2) sync (is_sync_kiocb(iocb)) or IOCB_DIRECT not being set
>>
>> fuse_file_write_iter
>>      fuse_direct_write_iter
>>          fuse_send_dio
>>
>> 3) FOPEN_DIRECT_IO not set
>>
>> Same as the consolidates path below
>>
>> The new consolidated code path is always
>>
>> fuse_file_write_iter
>>      fuse_cache_write_iter
>>          generic_file_write_iter
>>               __generic_file_write_iter
>>                   generic_file_direct_write
>>                       mapping->a_ops->direct_IO / fuse_direct_IO
>>                           fuse_send_dio
>>
>> So in general for FOPEN_DIRECT_IO the code path gets longer. Additionally
>> fuse_direct_IO does an allocation of struct fuse_io_priv - might be a bit
>> slower in micro benchmarks.
>> Also, the IOCB_DIRECT information gets lost (as we now set it outselves).
>> But then IOCB_DIRECT is directly related to O_DIRECT set in
>> struct file::f_flags.
>>
>> An additional change is for condition 2 above, which might will now do
>> async IO on the condition ff->fm->fc->async_dio. Given that async IO for
>> FOPEN_DIRECT_IO was especially introduced in commit
>> 'commit 23c94e1cdcbf ("fuse: Switch to using async direct IO for
>>   FOPEN_DIRECT_IO")'
>> it should not matter. Especially as fuse_direct_IO is blocking for
>> is_sync_kiocb(), at worst it has another slight overhead.
>>
>> Advantage is the removal of code paths and conditions and it is now also
>> possible to remove FOPEN_DIRECT_IO conditions in fuse_send_dio
>> (in a later patch).
>>
>> Cc: Hao Xu <howeyxu@tencent.com>
>> Cc: Miklos Szeredi <miklos@szeredi.hu>
>> Cc: Dharmendra Singh <dsingh@ddn.com>
>> Cc: linux-fsdevel@vger.kernel.org
>> Signed-off-by: Bernd Schubert <bschubert@ddn.com>
>> ---
>>   fs/fuse/file.c | 54 ++++----------------------------------------------
>>   1 file changed, 4 insertions(+), 50 deletions(-)
>>
>> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
>> index f9d21804d313..0b3363eec435 100644
>> --- a/fs/fuse/file.c
>> +++ b/fs/fuse/file.c
>> @@ -1602,52 +1602,6 @@ static ssize_t fuse_direct_read_iter(struct 
>> kiocb *iocb, struct iov_iter *to)
>>       return res;
>>   }
>> -static ssize_t fuse_direct_write_iter(struct kiocb *iocb, struct 
>> iov_iter *from)
>> -{
>> -    struct inode *inode = file_inode(iocb->ki_filp);
>> -    struct fuse_io_priv io = FUSE_IO_PRIV_SYNC(iocb);
>> -    ssize_t res;
>> -    bool exclusive_lock = fuse_dio_wr_exclusive_lock(iocb, from);
>> -
>> -    /*
>> -     * Take exclusive lock if
>> -     * - Parallel direct writes are disabled - a user space decision
>> -     * - Parallel direct writes are enabled and i_size is being 
>> extended.
>> -     *   This might not be needed at all, but needs further 
>> investigation.
>> -     */
>> -    if (exclusive_lock)
>> -        inode_lock(inode);
>> -    else {
>> -        inode_lock_shared(inode);
>> -
>> -        /* A race with truncate might have come up as the decision for
>> -         * the lock type was done without holding the lock, check again.
>> -         */
>> -        if (fuse_io_past_eof(iocb, from)) {
>> -            inode_unlock_shared(inode);
>> -            inode_lock(inode);
>> -            exclusive_lock = true;
>> -        }
>> -    }
>> -
>> -    res = generic_write_checks(iocb, from);
>> -    if (res > 0) {
>> -        if (!is_sync_kiocb(iocb) && iocb->ki_flags & IOCB_DIRECT) {
>> -            res = fuse_direct_IO(iocb, from);
>> -        } else {
>> -            res = fuse_send_dio(&io, from, &iocb->ki_pos,
>> -                        FUSE_DIO_WRITE);
>> -            fuse_write_update_attr(inode, iocb->ki_pos, res);
>> -        }
>> -    }
>> -    if (exclusive_lock)
>> -        inode_unlock(inode);
>> -    else
>> -        inode_unlock_shared(inode);
>> -
>> -    return res;
>> -}
>> -
>>   static ssize_t fuse_file_read_iter(struct kiocb *iocb, struct 
>> iov_iter *to)
>>   {
>>       struct file *file = iocb->ki_filp;
>> @@ -1678,10 +1632,10 @@ static ssize_t fuse_file_write_iter(struct 
>> kiocb *iocb, struct iov_iter *from)
>>       if (FUSE_IS_DAX(inode))
>>           return fuse_dax_write_iter(iocb, from);
>> -    if (!(ff->open_flags & FOPEN_DIRECT_IO))
>> -        return fuse_cache_write_iter(iocb, from);
>> -    else
>> -        return fuse_direct_write_iter(iocb, from);
>> +    if (ff->open_flags & FOPEN_DIRECT_IO)
>> +        iocb->ki_flags |= IOCB_DIRECT;
> 
> I think this affect the back-end behavior, changes a buffered IO to 
> direct io. FOPEN_DIRECT_IO means no cache in front-end, but we should
> respect the back-end semantics. We may need another way to indicate
> "we need go the direct io code path while IOCB_DIRECT is not set though".

I'm confused here, I guess with frontend you mean fuse kernel and with 
backend fuse userspace (daemon/server). IOCB_DIRECT is not passed to 
server/daemon, so there is no change? Although maybe I should document 
in fuse_write_flags() to be careful with returned flags and that 
IOCB_DIRECT cannot be trusted - if this should ever passed, one needs to 
use struct file::flags & O_DIRECT.


Thanks,
Bernd


Thanks,
Bernd

Re: [PATCH 5/6] fuse: Remove fuse_direct_write_iter code path / use IOCB_DIRECT

[Hao Xu]

On 8/31/23 17:34, Bernd Schubert wrote:
>
>
> On 8/31/23 11:19, Hao Xu wrote:
>> On 8/30/23 00:11, Bernd Schubert wrote:
>>> fuse_direct_write_iter is basically duplicating what is already
>>> in fuse_cache_write_iter/generic_file_direct_write. That can be
>>> avoided by setting IOCB_DIRECT in fuse_file_write_iter, after that
>>> fuse_cache_write_iter can be used for the FOPEN_DIRECT_IO code path
>>> and fuse_direct_write_iter can be removed.
>>>
>>> Before it was using for FOPEN_DIRECT_IO
>>>
>>> 1) async (!is_sync_kiocb(iocb)) && IOCB_DIRECT
>>>
>>> fuse_file_write_iter
>>>      fuse_direct_write_iter
>>>          fuse_direct_IO
>>>              fuse_send_dio
>>>
>>> 2) sync (is_sync_kiocb(iocb)) or IOCB_DIRECT not being set
>>>
>>> fuse_file_write_iter
>>>      fuse_direct_write_iter
>>>          fuse_send_dio
>>>
>>> 3) FOPEN_DIRECT_IO not set
>>>
>>> Same as the consolidates path below
>>>
>>> The new consolidated code path is always
>>>
>>> fuse_file_write_iter
>>>      fuse_cache_write_iter
>>>          generic_file_write_iter
>>>               __generic_file_write_iter
>>>                   generic_file_direct_write
>>>                       mapping->a_ops->direct_IO / fuse_direct_IO
>>>                           fuse_send_dio
>>>
>>> So in general for FOPEN_DIRECT_IO the code path gets longer. 
>>> Additionally
>>> fuse_direct_IO does an allocation of struct fuse_io_priv - might be 
>>> a bit
>>> slower in micro benchmarks.
>>> Also, the IOCB_DIRECT information gets lost (as we now set it 
>>> outselves).
>>> But then IOCB_DIRECT is directly related to O_DIRECT set in
>>> struct file::f_flags.
>>>
>>> An additional change is for condition 2 above, which might will now do
>>> async IO on the condition ff->fm->fc->async_dio. Given that async IO 
>>> for
>>> FOPEN_DIRECT_IO was especially introduced in commit
>>> 'commit 23c94e1cdcbf ("fuse: Switch to using async direct IO for
>>>   FOPEN_DIRECT_IO")'
>>> it should not matter. Especially as fuse_direct_IO is blocking for
>>> is_sync_kiocb(), at worst it has another slight overhead.
>>>
>>> Advantage is the removal of code paths and conditions and it is now 
>>> also
>>> possible to remove FOPEN_DIRECT_IO conditions in fuse_send_dio
>>> (in a later patch).
>>>
>>> Cc: Hao Xu <howeyxu@tencent.com>
>>> Cc: Miklos Szeredi <miklos@szeredi.hu>
>>> Cc: Dharmendra Singh <dsingh@ddn.com>
>>> Cc: linux-fsdevel@vger.kernel.org
>>> Signed-off-by: Bernd Schubert <bschubert@ddn.com>
>>> ---
>>>   fs/fuse/file.c | 54 
>>> ++++----------------------------------------------
>>>   1 file changed, 4 insertions(+), 50 deletions(-)
>>>
>>> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
>>> index f9d21804d313..0b3363eec435 100644
>>> --- a/fs/fuse/file.c
>>> +++ b/fs/fuse/file.c
>>> @@ -1602,52 +1602,6 @@ static ssize_t fuse_direct_read_iter(struct 
>>> kiocb *iocb, struct iov_iter *to)
>>>       return res;
>>>   }
>>> -static ssize_t fuse_direct_write_iter(struct kiocb *iocb, struct 
>>> iov_iter *from)
>>> -{
>>> -    struct inode *inode = file_inode(iocb->ki_filp);
>>> -    struct fuse_io_priv io = FUSE_IO_PRIV_SYNC(iocb);
>>> -    ssize_t res;
>>> -    bool exclusive_lock = fuse_dio_wr_exclusive_lock(iocb, from);
>>> -
>>> -    /*
>>> -     * Take exclusive lock if
>>> -     * - Parallel direct writes are disabled - a user space decision
>>> -     * - Parallel direct writes are enabled and i_size is being 
>>> extended.
>>> -     *   This might not be needed at all, but needs further 
>>> investigation.
>>> -     */
>>> -    if (exclusive_lock)
>>> -        inode_lock(inode);
>>> -    else {
>>> -        inode_lock_shared(inode);
>>> -
>>> -        /* A race with truncate might have come up as the decision for
>>> -         * the lock type was done without holding the lock, check 
>>> again.
>>> -         */
>>> -        if (fuse_io_past_eof(iocb, from)) {
>>> -            inode_unlock_shared(inode);
>>> -            inode_lock(inode);
>>> -            exclusive_lock = true;
>>> -        }
>>> -    }
>>> -
>>> -    res = generic_write_checks(iocb, from);
>>> -    if (res > 0) {
>>> -        if (!is_sync_kiocb(iocb) && iocb->ki_flags & IOCB_DIRECT) {
>>> -            res = fuse_direct_IO(iocb, from);
>>> -        } else {
>>> -            res = fuse_send_dio(&io, from, &iocb->ki_pos,
>>> -                        FUSE_DIO_WRITE);
>>> -            fuse_write_update_attr(inode, iocb->ki_pos, res);
>>> -        }
>>> -    }
>>> -    if (exclusive_lock)
>>> -        inode_unlock(inode);
>>> -    else
>>> -        inode_unlock_shared(inode);
>>> -
>>> -    return res;
>>> -}
>>> -
>>>   static ssize_t fuse_file_read_iter(struct kiocb *iocb, struct 
>>> iov_iter *to)
>>>   {
>>>       struct file *file = iocb->ki_filp;
>>> @@ -1678,10 +1632,10 @@ static ssize_t fuse_file_write_iter(struct 
>>> kiocb *iocb, struct iov_iter *from)
>>>       if (FUSE_IS_DAX(inode))
>>>           return fuse_dax_write_iter(iocb, from);
>>> -    if (!(ff->open_flags & FOPEN_DIRECT_IO))
>>> -        return fuse_cache_write_iter(iocb, from);
>>> -    else
>>> -        return fuse_direct_write_iter(iocb, from);
>>> +    if (ff->open_flags & FOPEN_DIRECT_IO)
>>> +        iocb->ki_flags |= IOCB_DIRECT;
>>
>> I think this affect the back-end behavior, changes a buffered IO to 
>> direct io. FOPEN_DIRECT_IO means no cache in front-end, but we should
>> respect the back-end semantics. We may need another way to indicate
>> "we need go the direct io code path while IOCB_DIRECT is not set 
>> though".
>
> I'm confused here, I guess with frontend you mean fuse kernel and with 
> backend fuse userspace (daemon/server). IOCB_DIRECT is not passed to 
> server/daemon, so there is no change? Although maybe I should document 
> in fuse_write_flags() to be careful with returned flags and that 
> IOCB_DIRECT cannot be trusted - if this should ever passed, one needs 
> to use struct file::flags & O_DIRECT.
>

I see, I misunderstood the code, `iocb->ki_filp->f_flags` not 
`iocb->ki_flags`...


Thanks,
Hao


>
> Thanks,
> Bernd
>
>
> Thanks,
> Bernd