| Message ID | 20230821174753.2736850-2-bschubert@ddn.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fuse: Parallel DIO writes with O_DIRECT | expand |
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
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
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
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 > >
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
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
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);
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(-)