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

[PATCH 0/5 v2] 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.

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/5] 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>
---
 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/5] 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 | 36 ++++++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 14 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index b1b9f2b9a37d..a16f9b6888de 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1298,6 +1298,27 @@ static ssize_t fuse_perform_write(struct kiocb *iocb, struct iov_iter *ii)
 	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);
+}
+
+/*
+ * @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;
+
+	return  !(ff->open_flags & FOPEN_PARALLEL_DIRECT_WRITES) ||
+		iocb->ki_flags & IOCB_APPEND ||
+		fuse_direct_write_extending_i_size(iocb, from);
+}
+
 static ssize_t fuse_cache_write_iter(struct kiocb *iocb, struct iov_iter *from)
 {
 	struct file *file = iocb->ki_filp;
@@ -1557,25 +1578,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
-- 
2.39.2

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

[Bernd Schubert]

Take a shared lock in fuse_cache_write_iter.

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 | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index a16f9b6888de..905ce3bb0047 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1314,9 +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;
 
-	return  !(ff->open_flags & FOPEN_PARALLEL_DIRECT_WRITES) ||
-		iocb->ki_flags & IOCB_APPEND ||
-		fuse_direct_write_extending_i_size(iocb, from);
+	return ((!(iocb->ki_flags & IOCB_DIRECT)) ||
+		(!(ff->open_flags & FOPEN_PARALLEL_DIRECT_WRITES)) ||
+		 iocb->ki_flags & IOCB_APPEND ||
+		 fuse_direct_write_extending_i_size(iocb, from));
 }
 
 static ssize_t fuse_cache_write_iter(struct kiocb *iocb, struct iov_iter *from)
@@ -1327,6 +1328,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) */
@@ -1345,7 +1347,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)
@@ -1360,6 +1365,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;
@@ -1369,7 +1377,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/5] [RFC] fuse: Set and use IOCB_DIRECT when FOPEN_DIRECT_IO is set

[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.

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 905ce3bb0047..09277a54b711 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1589,52 +1589,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_direct_write_extending_i_size(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_direct_io(&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;
@@ -1665,10 +1619,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 5/5] 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>
---
 fs/fuse/file.c | 17 -----------------
 1 file changed, 17 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 09277a54b711..eab105ef4640 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1473,20 +1473,11 @@ 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);
 	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);
@@ -1495,14 +1486,6 @@ 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.39.2

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

[Miklos Szeredi]

On Thu, 24 Aug 2023 at 17:06, 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 | 36 ++++++++++++++++++++++--------------
>  1 file changed, 22 insertions(+), 14 deletions(-)
>
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index b1b9f2b9a37d..a16f9b6888de 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -1298,6 +1298,27 @@ static ssize_t fuse_perform_write(struct kiocb *iocb, struct iov_iter *ii)
>         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);
> +}

The name suggests that this helper test for write as well as for
direct IO.  It does neither, instead it just checks whether the I/O
reaches past EOF.  So I'd name it e.g. io_past_eof().

> +/*
> + * @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;
> +
> +       return  !(ff->open_flags & FOPEN_PARALLEL_DIRECT_WRITES) ||
> +               iocb->ki_flags & IOCB_APPEND ||
> +               fuse_direct_write_extending_i_size(iocb, from);
> +}
> +

Now that this is a function it's possible to rewrite it as a series of
single-condition ifs.

Thanks,
Miklos

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

[Miklos Szeredi]

On Thu, 24 Aug 2023 at 17:08, Bernd Schubert <bschubert@ddn.com> wrote:
>
> Take a shared lock in fuse_cache_write_iter.
>
> 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 | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index a16f9b6888de..905ce3bb0047 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -1314,9 +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;
>
> -       return  !(ff->open_flags & FOPEN_PARALLEL_DIRECT_WRITES) ||
> -               iocb->ki_flags & IOCB_APPEND ||
> -               fuse_direct_write_extending_i_size(iocb, from);
> +       return ((!(iocb->ki_flags & IOCB_DIRECT)) ||
> +               (!(ff->open_flags & FOPEN_PARALLEL_DIRECT_WRITES)) ||

Why the extra parenthesis around the negation in the above two conditions?

So this condition will always be true at this point when called from
fuse_cache_write_iter()?  If so, you need to explain in the commit
message why are you doing this at this point (e.g. future patches
depend on this).


Thanks,
Miklos

Re: [PATCH 4/5] [RFC] fuse: Set and use IOCB_DIRECT when FOPEN_DIRECT_IO is set

[Miklos Szeredi]

On Thu, 24 Aug 2023 at 17:07, Bernd Schubert <bschubert@ddn.com> 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.
>
> 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 905ce3bb0047..09277a54b711 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -1589,52 +1589,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_direct_write_extending_i_size(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_direct_io(&io, from, &iocb->ki_pos,
> -                                            FUSE_DIO_WRITE);
> -                       fuse_write_update_attr(inode, iocb->ki_pos, res);

While I think this is correct, I'd really like if the code to be
replaced and the replacement are at least somewhat comparable.

Currently fuse_direct_IO() handles all cases (of which are many since
the requester can be sync or async and the server can be sync or
async).

Could this mess be cleaned up somehow?

Also could we make the function names of fuse_direct_IO() and
fuse_direct_io() less similar, as this is a very annoying (though
minor) issue.

Thanks,
Miklos

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

[Miklos Szeredi]

On Thu, 24 Aug 2023 at 17:07, Bernd Schubert <bschubert@ddn.com> wrote:
>
> 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>

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

[Miklos Szeredi]

On Thu, 24 Aug 2023 at 17:07, Bernd Schubert <bschubert@ddn.com> wrote:
>
> 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>

Thanks,
Miklos

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

[Bernd Schubert]

On 8/28/23 12:42, Miklos Szeredi wrote:
> On Thu, 24 Aug 2023 at 17:08, Bernd Schubert <bschubert@ddn.com> wrote:
>>
>> Take a shared lock in fuse_cache_write_iter.
>>
>> 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 | 21 ++++++++++++++++-----
>>   1 file changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
>> index a16f9b6888de..905ce3bb0047 100644
>> --- a/fs/fuse/file.c
>> +++ b/fs/fuse/file.c
>> @@ -1314,9 +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;
>>
>> -       return  !(ff->open_flags & FOPEN_PARALLEL_DIRECT_WRITES) ||
>> -               iocb->ki_flags & IOCB_APPEND ||
>> -               fuse_direct_write_extending_i_size(iocb, from);
>> +       return ((!(iocb->ki_flags & IOCB_DIRECT)) ||
>> +               (!(ff->open_flags & FOPEN_PARALLEL_DIRECT_WRITES)) ||
> 
> Why the extra parenthesis around the negation in the above two conditions?
> 
> So this condition will always be true at this point when called from
> fuse_cache_write_iter()?  If so, you need to explain in the commit
> message why are you doing this at this point (e.g. future patches
> depend on this).

Oh, thanks for spotting, the double parenthesis were accidentally. 
Although I don't think it would have an effect, it just results in

return ((!(condition1)) || ...

I.e. does not change the condition itself?

Anyway, yeah, agreed on your comment in the patch before, with one 
condition per line it becomes easier to read and avoids parenthesis. I 
had just tried to keep the code as it is to make the patch easier to read.


Thanks,
Bernd

Re: [PATCH 4/5] [RFC] fuse: Set and use IOCB_DIRECT when FOPEN_DIRECT_IO is set

[Bernd Schubert]

On 8/28/23 13:59, Miklos Szeredi wrote:
> On Thu, 24 Aug 2023 at 17:07, Bernd Schubert <bschubert@ddn.com> 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.
>>
>> 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 905ce3bb0047..09277a54b711 100644
>> --- a/fs/fuse/file.c
>> +++ b/fs/fuse/file.c
>> @@ -1589,52 +1589,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_direct_write_extending_i_size(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_direct_io(&io, from, &iocb->ki_pos,
>> -                                            FUSE_DIO_WRITE);
>> -                       fuse_write_update_attr(inode, iocb->ki_pos, res);
> 
> While I think this is correct, I'd really like if the code to be
> replaced and the replacement are at least somewhat comparable.

Sorry, I have a hard to time to understand "I'd really like if the code 
to be replaced".

> 
> Currently fuse_direct_IO() handles all cases (of which are many since
> the requester can be sync or async and the server can be sync or
> async).
> 
> Could this mess be cleaned up somehow?


I guess what you mean is to make the the replacement more obvious? I can 
try... I need to think about how to do that. Before submitting the patch 
I had looked up different code paths and I think fuse_direct_IO (called 
by fuse_cache_write_iter -> generic_file_direct_write) all handles it.

Maybe a new patch like this in fuse_file_write_iter

if (condition1)
     fuse_cache_write_iter

if (condition2)
     fuse_cache_write_iter

...

and once all conditions in fuse_direct_write_iter are handled in 
fuse_file_write_iter another the final patch (what is current this 4/5) 
to remove fuse_direct_write_iter?


> 
> Also could we make the function names of fuse_direct_IO() and
> fuse_direct_io() less similar, as this is a very annoying (though
> minor) issue.


Entirely agreed, I had already thought about it, but wasn't sure why it 
was named like this and didn't want to change too much.


Thanks,
Bernd

Re: [PATCH 4/5] [RFC] fuse: Set and use IOCB_DIRECT when FOPEN_DIRECT_IO is set

[Miklos Szeredi]

On Mon, 28 Aug 2023 at 16:48, Bernd Schubert <bernd.schubert@fastmail.fm> wrote:
>
> On 8/28/23 13:59, Miklos Szeredi wrote:
> > On Thu, 24 Aug 2023 at 17:07, Bernd Schubert <bschubert@ddn.com> wrote:

> >> -               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);
> >> -                       fuse_write_update_attr(inode, iocb->ki_pos, res);
> >
> > While I think this is correct, I'd really like if the code to be
> > replaced and the replacement are at least somewhat comparable.
>
> Sorry, I have a hard to time to understand "I'd really like if the code
> to be replaced".

What I meant is that generic_file_direct_write() is not an obvious
replacement for the  above lines of code.

The reason is that fuse_direct_IO() is handling the sync and async
cases in one function, while the above splits handling it based on
IOCB_DIRECT (which is now lost) and is_sync_kiocb(iocb).  If it's okay
to lose IOCB_DIRECT then what's the explanation for the above
condition?  It could be historic garbage, but we still need to
understand what is exactly happening.

Thanks,
Miklos

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

[Miklos Szeredi]

On Mon, 28 Aug 2023 at 16:21, Bernd Schubert <bernd.schubert@fastmail.fm> wrote:
>
>
>
> On 8/28/23 12:42, Miklos Szeredi wrote:
> > On Thu, 24 Aug 2023 at 17:08, Bernd Schubert <bschubert@ddn.com> wrote:
> >>
> >> Take a shared lock in fuse_cache_write_iter.
> >>
> >> 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 | 21 ++++++++++++++++-----
> >>   1 file changed, 16 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> >> index a16f9b6888de..905ce3bb0047 100644
> >> --- a/fs/fuse/file.c
> >> +++ b/fs/fuse/file.c
> >> @@ -1314,9 +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;
> >>
> >> -       return  !(ff->open_flags & FOPEN_PARALLEL_DIRECT_WRITES) ||
> >> -               iocb->ki_flags & IOCB_APPEND ||
> >> -               fuse_direct_write_extending_i_size(iocb, from);
> >> +       return ((!(iocb->ki_flags & IOCB_DIRECT)) ||
> >> +               (!(ff->open_flags & FOPEN_PARALLEL_DIRECT_WRITES)) ||
> >
> > Why the extra parenthesis around the negation in the above two conditions?
> >
> > So this condition will always be true at this point when called from
> > fuse_cache_write_iter()?  If so, you need to explain in the commit
> > message why are you doing this at this point (e.g. future patches
> > depend on this).
>
> Oh, thanks for spotting, the double parenthesis were accidentally.
> Although I don't think it would have an effect, it just results in
>
> return ((!(condition1)) || ...
>
> I.e. does not change the condition itself?

It doesn't change the condition, but degrades readability.

Thanks,
Miklos

Re: [PATCH 4/5] [RFC] fuse: Set and use IOCB_DIRECT when FOPEN_DIRECT_IO is set

[Bernd Schubert]

On 8/28/23 13:59, Miklos Szeredi wrote:
> Also could we make the function names of fuse_direct_IO() and
> fuse_direct_io() less similar, as this is a very annoying (though
> minor) issue.

What about _fuse_do_direct_io()? '_' to mark that it is a follow up and 
'do' that it initiates the transfer? Or maybe just '_fuse_direct_io'? Or 
'fuse_send_dio'?


Thanks,
Bernd

Re: [PATCH 4/5] [RFC] fuse: Set and use IOCB_DIRECT when FOPEN_DIRECT_IO is set

[Miklos Szeredi]

On Mon, 28 Aug 2023 at 22:03, Bernd Schubert <bernd.schubert@fastmail.fm> wrote:
>
>
>
> On 8/28/23 13:59, Miklos Szeredi wrote:
> > Also could we make the function names of fuse_direct_IO() and
> > fuse_direct_io() less similar, as this is a very annoying (though
> > minor) issue.
>
> What about _fuse_do_direct_io()? '_' to mark that it is a follow up and
> 'do' that it initiates the transfer? Or maybe just '_fuse_direct_io'? Or
> 'fuse_send_dio'?

I'd prefer a non-underscore variant.  fuse_send_dio() sounds good.

Thanks,
Miklos

Re: [PATCH 4/5] [RFC] fuse: Set and use IOCB_DIRECT when FOPEN_DIRECT_IO is set

[Bernd Schubert]

On 8/28/23 17:05, Miklos Szeredi wrote:
> On Mon, 28 Aug 2023 at 16:48, Bernd Schubert <bernd.schubert@fastmail.fm> wrote:
>>
>> On 8/28/23 13:59, Miklos Szeredi wrote:
>>> On Thu, 24 Aug 2023 at 17:07, Bernd Schubert <bschubert@ddn.com> wrote:
> 
>>>> -               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);
>>>> -                       fuse_write_update_attr(inode, iocb->ki_pos, res);
>>>
>>> While I think this is correct, I'd really like if the code to be
>>> replaced and the replacement are at least somewhat comparable.
>>
>> Sorry, I have a hard to time to understand "I'd really like if the code
>> to be replaced".
> 
> What I meant is that generic_file_direct_write() is not an obvious
> replacement for the  above lines of code.
> 
> The reason is that fuse_direct_IO() is handling the sync and async
> cases in one function, while the above splits handling it based on
> IOCB_DIRECT (which is now lost) and is_sync_kiocb(iocb).  If it's okay
> to lose IOCB_DIRECT then what's the explanation for the above
> condition?  It could be historic garbage, but we still need to
> understand what is exactly happening.

While checking all code path again, I found an additional difference, 
which I had missed before. FOPEN_DIRECT_IO will now act on 
ff->fm->fc->async_dio when is is_sync_kiocb(iocb) is set.

Do you think that is a problem? If so, I could fix it in fuse_direct_IO.


Thanks,
Bernd

Re: [PATCH 4/5] [RFC] fuse: Set and use IOCB_DIRECT when FOPEN_DIRECT_IO is set

[Bernd Schubert]

On 8/29/23 15:08, Bernd Schubert wrote:
> 
> 
> On 8/28/23 17:05, Miklos Szeredi wrote:
>> On Mon, 28 Aug 2023 at 16:48, Bernd Schubert 
>> <bernd.schubert@fastmail.fm> wrote:
>>>
>>> On 8/28/23 13:59, Miklos Szeredi wrote:
>>>> On Thu, 24 Aug 2023 at 17:07, Bernd Schubert <bschubert@ddn.com> wrote:
>>
>>>>> -               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);
>>>>> -                       fuse_write_update_attr(inode, iocb->ki_pos, 
>>>>> res);
>>>>
>>>> While I think this is correct, I'd really like if the code to be
>>>> replaced and the replacement are at least somewhat comparable.
>>>
>>> Sorry, I have a hard to time to understand "I'd really like if the code
>>> to be replaced".
>>
>> What I meant is that generic_file_direct_write() is not an obvious
>> replacement for the  above lines of code.
>>
>> The reason is that fuse_direct_IO() is handling the sync and async
>> cases in one function, while the above splits handling it based on
>> IOCB_DIRECT (which is now lost) and is_sync_kiocb(iocb).  If it's okay
>> to lose IOCB_DIRECT then what's the explanation for the above
>> condition?  It could be historic garbage, but we still need to
>> understand what is exactly happening.
> 
> While checking all code path again, I found an additional difference, 
> which I had missed before. FOPEN_DIRECT_IO will now act on 
> ff->fm->fc->async_dio when is is_sync_kiocb(iocb) is set.
> 
> Do you think that is a problem? If so, I could fix it in fuse_direct_IO.

What I mean is something like this

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 85f2f9d3813e..3b383dc8a944 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1635,8 +1635,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)
+	if (ff->open_flags & FOPEN_DIRECT_IO) {
  		iocb->ki_flags |= IOCB_DIRECT;
+		ff->iocb_direct = 1;
+	}
  
  	return fuse_cache_write_iter(iocb, from);
  }
@@ -2905,6 +2907,15 @@ fuse_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
  	io->iocb = iocb;
  	io->blocking = is_sync_kiocb(iocb);
  
+	/* FOPEN_DIRECT_IO historically does not use async for blocking O_DIRECT */
+	if (ff->open_flags & FOPEN_DIRECT_IO) {
+		if (!is_sync_kiocb(iocb) && ff->iocb_direct) {
+			/* no change */
+		} else {
+			io->async = 0;
+		}
+	}
+
  	/* optimization for short read */
  	if (io->async && !io->write && offset + count > i_size) {
  		iov_iter_truncate(iter, fuse_round_up(ff->fm->fc, i_size - offset));
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index a56e83b7d29a..d77046875ad5 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -231,6 +231,9 @@ struct fuse_file {
  
  	/** Has flock been performed on this file? */
  	bool flock:1;
+
+	/** Has the file been opened with O_DIRECT? */
+	bool iocb_direct:1;
  };
  
  /** One input argument of a request */


Thanks,
Bernd

Re: [PATCH 4/5] [RFC] fuse: Set and use IOCB_DIRECT when FOPEN_DIRECT_IO is set

[Bernd Schubert]

On 8/29/23 15:26, Bernd Schubert wrote:
> 
> 
> On 8/29/23 15:08, Bernd Schubert wrote:
>>
>>
>> On 8/28/23 17:05, Miklos Szeredi wrote:
>>> On Mon, 28 Aug 2023 at 16:48, Bernd Schubert 
>>> <bernd.schubert@fastmail.fm> wrote:
>>>>
>>>> On 8/28/23 13:59, Miklos Szeredi wrote:
>>>>> On Thu, 24 Aug 2023 at 17:07, Bernd Schubert <bschubert@ddn.com> 
>>>>> wrote:
>>>
>>>>>> -               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);
>>>>>> -                       fuse_write_update_attr(inode, 
>>>>>> iocb->ki_pos, res);
>>>>>
>>>>> While I think this is correct, I'd really like if the code to be
>>>>> replaced and the replacement are at least somewhat comparable.
>>>>
>>>> Sorry, I have a hard to time to understand "I'd really like if the code
>>>> to be replaced".
>>>
>>> What I meant is that generic_file_direct_write() is not an obvious
>>> replacement for the  above lines of code.
>>>
>>> The reason is that fuse_direct_IO() is handling the sync and async
>>> cases in one function, while the above splits handling it based on
>>> IOCB_DIRECT (which is now lost) and is_sync_kiocb(iocb).  If it's okay
>>> to lose IOCB_DIRECT then what's the explanation for the above
>>> condition?  It could be historic garbage, but we still need to
>>> understand what is exactly happening.
>>
>> While checking all code path again, I found an additional difference, 
>> which I had missed before. FOPEN_DIRECT_IO will now act on 
>> ff->fm->fc->async_dio when is is_sync_kiocb(iocb) is set.
>>
>> Do you think that is a problem? If so, I could fix it in fuse_direct_IO.
> 
> What I mean is something like this
> 
> +    /* FOPEN_DIRECT_IO historically does not use async for blocking 
> O_DIRECT */
> +    if (ff->open_flags & FOPEN_DIRECT_IO) {
> +        if (!is_sync_kiocb(iocb) && ff->iocb_direct) {
> +            /* no change */
> +        } else {
> +            io->async = 0;
> +        }
> +    }

Besides that it could use file->f_flags & O_DIRECT, I guess we can keep 
async. It relates to commit
23c94e1cdcbf5953cd380555d0781caa42311870, which actually introduced 
async for FOPEN_DIRECT_IO. I'm just going to add it to the commit message.


Thanks,
Bernd