[PATCH RFC -next 0/2] fuse: Parallel DIO writes with O_DIRECT

[PATCH RFC -next 0/2] fuse: Parallel DIO writes with O_DIRECT

[Bernd Schubert]

In commit 153524053bbb fuse gained the possibility to do parallel
DIO writes, when FOPEN_DIRECT_IO and FOPEN_PARALLEL_DIRECT_WRITES
are set. If server side only sets FOPEN_PARALLEL_DIRECT_WRITES,
but does not set FOPEN_DIRECT_IO, O_DIRECT from the application
is still serialized.
fuse-next has changes in commits b5a2a3a0b776/80e4f25262f9, which
allow to take the optimized (in respect to parallel DIO) code path,
dirty page flush and page invalidation have to be done unconditionally,
though.

v2:
Rebase to 6.5/6.6-fuse-next

Bernd Schubert (2):
  [RFC for fuse-next ] fuse: DIO writes always use the same code path
  libfs: Remove export of direct_write_fallback

 fs/fuse/file.c | 27 +++++++++------------------
 fs/libfs.c     |  1 -
 2 files changed, 9 insertions(+), 19 deletions(-)

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

-- 
2.39.2

[PATCH 1/2] [RFC for fuse-next ] fuse: DIO writes always use the same code path

[Bernd Schubert]

There were two code paths direct-io writes could
take. When daemon/server side did not set FOPEN_DIRECT_IO
    fuse_cache_write_iter -> direct_write_fallback
and with FOPEN_DIRECT_IO being set
    fuse_direct_write_iter

Advantage of fuse_direct_write_iter is that it has optimizations
for parallel DIO writes - it might only take a shared inode lock,
instead of the exclusive lock.

With commits b5a2a3a0b776/80e4f25262f9 the fuse_direct_write_iter
path also handles concurrent page IO (dirty flush and page release),
just the condition on fc->direct_io_relax had to be removed.

Performance wise this basically gives the same improvements as
commit 153524053bbb, just O_DIRECT is sufficient, without the need
that server side sets FOPEN_DIRECT_IO
(it has to set FOPEN_PARALLEL_DIRECT_WRITES), though.

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
---
 fs/fuse/file.c | 27 +++++++++------------------
 1 file changed, 9 insertions(+), 18 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 1cdb6327511e..a5414f46d254 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1338,15 +1338,8 @@ static ssize_t fuse_cache_write_iter(struct kiocb *iocb, struct iov_iter *from)
 	if (err)
 		goto out;
 
-	if (iocb->ki_flags & IOCB_DIRECT) {
-		written = generic_file_direct_write(iocb, from);
-		if (written < 0 || !iov_iter_count(from))
-			goto out;
-		written = direct_write_fallback(iocb, from, written,
-				fuse_perform_write(iocb, from));
-	} else {
-		written = fuse_perform_write(iocb, from);
-	}
+	written = fuse_perform_write(iocb, from);
+
 out:
 	inode_unlock(inode);
 	if (written > 0)
@@ -1441,19 +1434,16 @@ 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;
-		}
+	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)
@@ -1463,7 +1453,7 @@ ssize_t fuse_direct_io(struct fuse_io_priv *io, struct iov_iter *iter,
 			inode_unlock(inode);
 	}
 
-	if (fopen_direct_io && write) {
+	if (write) {
 		res = invalidate_inode_pages2_range(mapping, idx_from, idx_to);
 		if (res) {
 			fuse_io_free(ia);
@@ -1646,7 +1636,8 @@ 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))
 		return fuse_cache_write_iter(iocb, from);
 	else
 		return fuse_direct_write_iter(iocb, from);
-- 
2.39.2

[PATCH 2/2] libfs: Remove export of direct_write_fallback

[Bernd Schubert]

The last external user of direct_write_fallback (fuse)
is not using this function anymore - exporting the symbol can
be removed.

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
---
 fs/libfs.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/libfs.c b/fs/libfs.c
index 5b851315eeed..db106065c187 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -1653,4 +1653,3 @@ ssize_t direct_write_fallback(struct kiocb *iocb, struct iov_iter *iter,
 	invalidate_mapping_pages(mapping, pos >> PAGE_SHIFT, end >> PAGE_SHIFT);
 	return direct_written + buffered_written;
 }
-EXPORT_SYMBOL_GPL(direct_write_fallback);
-- 
2.39.2

Re: [PATCH 1/2] [RFC for fuse-next ] fuse: DIO writes always use the same code path

[Miklos Szeredi]

On Mon, 21 Aug 2023 at 19:48, Bernd Schubert <bschubert@ddn.com> wrote:
>
> There were two code paths direct-io writes could
> take. When daemon/server side did not set FOPEN_DIRECT_IO
>     fuse_cache_write_iter -> direct_write_fallback
> and with FOPEN_DIRECT_IO being set
>     fuse_direct_write_iter
>
> Advantage of fuse_direct_write_iter is that it has optimizations
> for parallel DIO writes - it might only take a shared inode lock,
> instead of the exclusive lock.
>
> With commits b5a2a3a0b776/80e4f25262f9 the fuse_direct_write_iter
> path also handles concurrent page IO (dirty flush and page release),
> just the condition on fc->direct_io_relax had to be removed.
>
> Performance wise this basically gives the same improvements as
> commit 153524053bbb, just O_DIRECT is sufficient, without the need
> that server side sets FOPEN_DIRECT_IO
> (it has to set FOPEN_PARALLEL_DIRECT_WRITES), though.

Consolidating the various direct IO paths would be really nice.

Problem is that fuse_direct_write_iter() lacks some code from
generic_file_direct_write() and also completely lacks
direct_write_fallback().   So more thought needs to go into this.

Thanks,
Miklos

Re: [PATCH 1/2] [RFC for fuse-next ] fuse: DIO writes always use the same code path

[Bernd Schubert]

On 8/22/23 11:53, Miklos Szeredi wrote:
> On Mon, 21 Aug 2023 at 19:48, Bernd Schubert <bschubert@ddn.com> wrote:
>>
>> There were two code paths direct-io writes could
>> take. When daemon/server side did not set FOPEN_DIRECT_IO
>>      fuse_cache_write_iter -> direct_write_fallback
>> and with FOPEN_DIRECT_IO being set
>>      fuse_direct_write_iter
>>
>> Advantage of fuse_direct_write_iter is that it has optimizations
>> for parallel DIO writes - it might only take a shared inode lock,
>> instead of the exclusive lock.
>>
>> With commits b5a2a3a0b776/80e4f25262f9 the fuse_direct_write_iter
>> path also handles concurrent page IO (dirty flush and page release),
>> just the condition on fc->direct_io_relax had to be removed.
>>
>> Performance wise this basically gives the same improvements as
>> commit 153524053bbb, just O_DIRECT is sufficient, without the need
>> that server side sets FOPEN_DIRECT_IO
>> (it has to set FOPEN_PARALLEL_DIRECT_WRITES), though.
> 
> Consolidating the various direct IO paths would be really nice.
> 
> Problem is that fuse_direct_write_iter() lacks some code from
> generic_file_direct_write() and also completely lacks
> direct_write_fallback().   So more thought needs to go into this.

Thanks for looking at it! Hmm, right, I see. I guess at least
direct_write_fallback() should be done for the new relaxed
mmap mode.

Entirely duplicating generic_file_direct_write()
to generic_file_direct_write doesn't seem to be nice either.

Regarding the inode lock, it might be easier to
change fuse_cache_write_iter() to a shared lock, although that
does not help when fc->writeback_cache is enabled, which has yet
another code path. Although I'm not sure that is needed
direct IO. For the start, what do you think about

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);


Thanks,
Bernd

Re: [PATCH 1/2] [RFC for fuse-next ] fuse: DIO writes always use the same code path

[Miklos Szeredi]

On Tue, 22 Aug 2023 at 20:47, Bernd Schubert <bernd.schubert@fastmail.fm> wrote:
>
>
>
> On 8/22/23 11:53, Miklos Szeredi wrote:
> > On Mon, 21 Aug 2023 at 19:48, Bernd Schubert <bschubert@ddn.com> wrote:
> >>
> >> There were two code paths direct-io writes could
> >> take. When daemon/server side did not set FOPEN_DIRECT_IO
> >>      fuse_cache_write_iter -> direct_write_fallback
> >> and with FOPEN_DIRECT_IO being set
> >>      fuse_direct_write_iter
> >>
> >> Advantage of fuse_direct_write_iter is that it has optimizations
> >> for parallel DIO writes - it might only take a shared inode lock,
> >> instead of the exclusive lock.
> >>
> >> With commits b5a2a3a0b776/80e4f25262f9 the fuse_direct_write_iter
> >> path also handles concurrent page IO (dirty flush and page release),
> >> just the condition on fc->direct_io_relax had to be removed.
> >>
> >> Performance wise this basically gives the same improvements as
> >> commit 153524053bbb, just O_DIRECT is sufficient, without the need
> >> that server side sets FOPEN_DIRECT_IO
> >> (it has to set FOPEN_PARALLEL_DIRECT_WRITES), though.
> >
> > Consolidating the various direct IO paths would be really nice.
> >
> > Problem is that fuse_direct_write_iter() lacks some code from
> > generic_file_direct_write() and also completely lacks
> > direct_write_fallback().   So more thought needs to go into this.
>
> Thanks for looking at it! Hmm, right, I see. I guess at least
> direct_write_fallback() should be done for the new relaxed
> mmap mode.
>
> Entirely duplicating generic_file_direct_write()
> to generic_file_direct_write doesn't seem to be nice either.
>
> Regarding the inode lock, it might be easier to
> change fuse_cache_write_iter() to a shared lock, although that
> does not help when fc->writeback_cache is enabled, which has yet
> another code path. Although I'm not sure that is needed
> direct IO. For the start, what do you think about
>
> 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)) {

This makes sense.  No point in doing cached write + sync when we can
do write-through.  The fallback thing makes sense only in the case
when the page invalidation fails.  Otherwise the fallback code should
not even be invoked, I think.

Thanks,
Miklos

Re: [fuse-devel] [PATCH 1/2] [RFC for fuse-next ] fuse: DIO writes always use the same code path

[Hao Xu]

On 8/22/23 17:53, Miklos Szeredi via fuse-devel wrote:
> On Mon, 21 Aug 2023 at 19:48, Bernd Schubert <bschubert@ddn.com> wrote:
>> There were two code paths direct-io writes could
>> take. When daemon/server side did not set FOPEN_DIRECT_IO
>>      fuse_cache_write_iter -> direct_write_fallback
>> and with FOPEN_DIRECT_IO being set
>>      fuse_direct_write_iter
>>
>> Advantage of fuse_direct_write_iter is that it has optimizations
>> for parallel DIO writes - it might only take a shared inode lock,
>> instead of the exclusive lock.
>>
>> With commits b5a2a3a0b776/80e4f25262f9 the fuse_direct_write_iter
>> path also handles concurrent page IO (dirty flush and page release),
>> just the condition on fc->direct_io_relax had to be removed.
>>
>> Performance wise this basically gives the same improvements as
>> commit 153524053bbb, just O_DIRECT is sufficient, without the need
>> that server side sets FOPEN_DIRECT_IO
>> (it has to set FOPEN_PARALLEL_DIRECT_WRITES), though.
> Consolidating the various direct IO paths would be really nice.
>
> Problem is that fuse_direct_write_iter() lacks some code from
> generic_file_direct_write() and also completely lacks


I see, seems the page invalidation post direct write is needed

as well.


> direct_write_fallback().   So more thought needs to go into this.
>
> Thanks,
> Miklos
>
>

Re: [fuse-devel] [PATCH 1/2] [RFC for fuse-next ] fuse: DIO writes always use the same code path

[Bernd Schubert]

On 8/24/23 06:32, Hao Xu wrote:
> 
> On 8/22/23 17:53, Miklos Szeredi via fuse-devel wrote:
>> On Mon, 21 Aug 2023 at 19:48, Bernd Schubert <bschubert@ddn.com> wrote:
>>> There were two code paths direct-io writes could
>>> take. When daemon/server side did not set FOPEN_DIRECT_IO
>>>      fuse_cache_write_iter -> direct_write_fallback
>>> and with FOPEN_DIRECT_IO being set
>>>      fuse_direct_write_iter
>>>
>>> Advantage of fuse_direct_write_iter is that it has optimizations
>>> for parallel DIO writes - it might only take a shared inode lock,
>>> instead of the exclusive lock.
>>>
>>> With commits b5a2a3a0b776/80e4f25262f9 the fuse_direct_write_iter
>>> path also handles concurrent page IO (dirty flush and page release),
>>> just the condition on fc->direct_io_relax had to be removed.
>>>
>>> Performance wise this basically gives the same improvements as
>>> commit 153524053bbb, just O_DIRECT is sufficient, without the need
>>> that server side sets FOPEN_DIRECT_IO
>>> (it has to set FOPEN_PARALLEL_DIRECT_WRITES), though.
>> Consolidating the various direct IO paths would be really nice.
>>
>> Problem is that fuse_direct_write_iter() lacks some code from
>> generic_file_direct_write() and also completely lacks
> 
> 
> I see, seems the page invalidation post direct write is needed
> 
> as well.
> 

I'm in the middle of verifying code paths, but I wonder if we can
remove the entire function at all.


https://github.com/bsbernd/linux/commit/fe082a0795fe5839211488e9645732b5f3809bea

on this branch

https://github.com/bsbernd/linux/commits/o-direct-shared-lock


Also totally untested - I hope I did not miss anything...


Thanks,
Bernd

Re: [fuse-devel] [PATCH 1/2] [RFC for fuse-next ] fuse: DIO writes always use the same code path

[Bernd Schubert]

On 8/24/23 11:43, Bernd Schubert wrote:
> 
> 
> On 8/24/23 06:32, Hao Xu wrote:
>>
>> On 8/22/23 17:53, Miklos Szeredi via fuse-devel wrote:
>>> On Mon, 21 Aug 2023 at 19:48, Bernd Schubert <bschubert@ddn.com> wrote:
>>>> There were two code paths direct-io writes could
>>>> take. When daemon/server side did not set FOPEN_DIRECT_IO
>>>>      fuse_cache_write_iter -> direct_write_fallback
>>>> and with FOPEN_DIRECT_IO being set
>>>>      fuse_direct_write_iter
>>>>
>>>> Advantage of fuse_direct_write_iter is that it has optimizations
>>>> for parallel DIO writes - it might only take a shared inode lock,
>>>> instead of the exclusive lock.
>>>>
>>>> With commits b5a2a3a0b776/80e4f25262f9 the fuse_direct_write_iter
>>>> path also handles concurrent page IO (dirty flush and page release),
>>>> just the condition on fc->direct_io_relax had to be removed.
>>>>
>>>> Performance wise this basically gives the same improvements as
>>>> commit 153524053bbb, just O_DIRECT is sufficient, without the need
>>>> that server side sets FOPEN_DIRECT_IO
>>>> (it has to set FOPEN_PARALLEL_DIRECT_WRITES), though.
>>> Consolidating the various direct IO paths would be really nice.
>>>
>>> Problem is that fuse_direct_write_iter() lacks some code from
>>> generic_file_direct_write() and also completely lacks
>>
>>
>> I see, seems the page invalidation post direct write is needed
>>
>> as well.
>>
> 
> I'm in the middle of verifying code paths, but I wonder if we can
> remove the entire function at all.

Sorry, doesn't remove fuse_direct_io(), but would go via 
generic_file_direct_write, which already has page invalidation.

> 
> 
> https://github.com/bsbernd/linux/commit/fe082a0795fe5839211488e9645732b5f3809bea
> 
> on this branch
> 
> https://github.com/bsbernd/linux/commits/o-direct-shared-lock
> 
> 
> Also totally untested - I hope I did not miss anything...
> 
> 
> Thanks,
> Bernd