On 8/22/23 11:53, Miklos Szeredi wrote:
On Mon, 21 Aug 2023 at 19:48, Bernd Schubert <bschubert@xxxxxxx> 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
