On Tue, 22 Aug 2023 at 20:47, Bernd Schubert <bernd.schubert@xxxxxxxxxxx> wrote:
>
>
>
> 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)) {
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
