| Message ID | 20230829161116.2914040-6-bschubert@ddn.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fuse direct write consolidation and parallel IO | expand |
On 8/30/23 00:11, Bernd Schubert 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. > > Before it was using for FOPEN_DIRECT_IO > > 1) async (!is_sync_kiocb(iocb)) && IOCB_DIRECT > > fuse_file_write_iter > fuse_direct_write_iter > fuse_direct_IO > fuse_send_dio > > 2) sync (is_sync_kiocb(iocb)) or IOCB_DIRECT not being set > > fuse_file_write_iter > fuse_direct_write_iter > fuse_send_dio > > 3) FOPEN_DIRECT_IO not set > > Same as the consolidates path below > > The new consolidated code path is always > > fuse_file_write_iter > fuse_cache_write_iter > generic_file_write_iter > __generic_file_write_iter > generic_file_direct_write > mapping->a_ops->direct_IO / fuse_direct_IO > fuse_send_dio > > So in general for FOPEN_DIRECT_IO the code path gets longer. Additionally > fuse_direct_IO does an allocation of struct fuse_io_priv - might be a bit > slower in micro benchmarks. > Also, the IOCB_DIRECT information gets lost (as we now set it outselves). > But then IOCB_DIRECT is directly related to O_DIRECT set in > struct file::f_flags. > > An additional change is for condition 2 above, which might will now do > async IO on the condition ff->fm->fc->async_dio. Given that async IO for > FOPEN_DIRECT_IO was especially introduced in commit > 'commit 23c94e1cdcbf ("fuse: Switch to using async direct IO for > FOPEN_DIRECT_IO")' > it should not matter. Especially as fuse_direct_IO is blocking for > is_sync_kiocb(), at worst it has another slight overhead. > > Advantage is the removal of code paths and conditions and it is now also > possible to remove FOPEN_DIRECT_IO conditions in fuse_send_dio > (in a later patch). > > 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 f9d21804d313..0b3363eec435 100644 > --- a/fs/fuse/file.c > +++ b/fs/fuse/file.c > @@ -1602,52 +1602,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_io_past_eof(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_send_dio(&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; > @@ -1678,10 +1632,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; I think this affect the back-end behavior, changes a buffered IO to direct io. FOPEN_DIRECT_IO means no cache in front-end, but we should respect the back-end semantics. We may need another way to indicate "we need go the direct io code path while IOCB_DIRECT is not set though". > + > + return fuse_cache_write_iter(iocb, from); > } > > static void fuse_writepage_free(struct fuse_writepage_args *wpa)
On 8/31/23 11:19, Hao Xu wrote: > On 8/30/23 00:11, Bernd Schubert 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. >> >> Before it was using for FOPEN_DIRECT_IO >> >> 1) async (!is_sync_kiocb(iocb)) && IOCB_DIRECT >> >> fuse_file_write_iter >> fuse_direct_write_iter >> fuse_direct_IO >> fuse_send_dio >> >> 2) sync (is_sync_kiocb(iocb)) or IOCB_DIRECT not being set >> >> fuse_file_write_iter >> fuse_direct_write_iter >> fuse_send_dio >> >> 3) FOPEN_DIRECT_IO not set >> >> Same as the consolidates path below >> >> The new consolidated code path is always >> >> fuse_file_write_iter >> fuse_cache_write_iter >> generic_file_write_iter >> __generic_file_write_iter >> generic_file_direct_write >> mapping->a_ops->direct_IO / fuse_direct_IO >> fuse_send_dio >> >> So in general for FOPEN_DIRECT_IO the code path gets longer. Additionally >> fuse_direct_IO does an allocation of struct fuse_io_priv - might be a bit >> slower in micro benchmarks. >> Also, the IOCB_DIRECT information gets lost (as we now set it outselves). >> But then IOCB_DIRECT is directly related to O_DIRECT set in >> struct file::f_flags. >> >> An additional change is for condition 2 above, which might will now do >> async IO on the condition ff->fm->fc->async_dio. Given that async IO for >> FOPEN_DIRECT_IO was especially introduced in commit >> 'commit 23c94e1cdcbf ("fuse: Switch to using async direct IO for >> FOPEN_DIRECT_IO")' >> it should not matter. Especially as fuse_direct_IO is blocking for >> is_sync_kiocb(), at worst it has another slight overhead. >> >> Advantage is the removal of code paths and conditions and it is now also >> possible to remove FOPEN_DIRECT_IO conditions in fuse_send_dio >> (in a later patch). >> >> 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 f9d21804d313..0b3363eec435 100644 >> --- a/fs/fuse/file.c >> +++ b/fs/fuse/file.c >> @@ -1602,52 +1602,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_io_past_eof(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_send_dio(&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; >> @@ -1678,10 +1632,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; > > I think this affect the back-end behavior, changes a buffered IO to > direct io. FOPEN_DIRECT_IO means no cache in front-end, but we should > respect the back-end semantics. We may need another way to indicate > "we need go the direct io code path while IOCB_DIRECT is not set though". I'm confused here, I guess with frontend you mean fuse kernel and with backend fuse userspace (daemon/server). IOCB_DIRECT is not passed to server/daemon, so there is no change? Although maybe I should document in fuse_write_flags() to be careful with returned flags and that IOCB_DIRECT cannot be trusted - if this should ever passed, one needs to use struct file::flags & O_DIRECT. Thanks, Bernd Thanks, Bernd
On 8/31/23 17:34, Bernd Schubert wrote: > > > On 8/31/23 11:19, Hao Xu wrote: >> On 8/30/23 00:11, Bernd Schubert 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. >>> >>> Before it was using for FOPEN_DIRECT_IO >>> >>> 1) async (!is_sync_kiocb(iocb)) && IOCB_DIRECT >>> >>> fuse_file_write_iter >>> fuse_direct_write_iter >>> fuse_direct_IO >>> fuse_send_dio >>> >>> 2) sync (is_sync_kiocb(iocb)) or IOCB_DIRECT not being set >>> >>> fuse_file_write_iter >>> fuse_direct_write_iter >>> fuse_send_dio >>> >>> 3) FOPEN_DIRECT_IO not set >>> >>> Same as the consolidates path below >>> >>> The new consolidated code path is always >>> >>> fuse_file_write_iter >>> fuse_cache_write_iter >>> generic_file_write_iter >>> __generic_file_write_iter >>> generic_file_direct_write >>> mapping->a_ops->direct_IO / fuse_direct_IO >>> fuse_send_dio >>> >>> So in general for FOPEN_DIRECT_IO the code path gets longer. >>> Additionally >>> fuse_direct_IO does an allocation of struct fuse_io_priv - might be >>> a bit >>> slower in micro benchmarks. >>> Also, the IOCB_DIRECT information gets lost (as we now set it >>> outselves). >>> But then IOCB_DIRECT is directly related to O_DIRECT set in >>> struct file::f_flags. >>> >>> An additional change is for condition 2 above, which might will now do >>> async IO on the condition ff->fm->fc->async_dio. Given that async IO >>> for >>> FOPEN_DIRECT_IO was especially introduced in commit >>> 'commit 23c94e1cdcbf ("fuse: Switch to using async direct IO for >>> FOPEN_DIRECT_IO")' >>> it should not matter. Especially as fuse_direct_IO is blocking for >>> is_sync_kiocb(), at worst it has another slight overhead. >>> >>> Advantage is the removal of code paths and conditions and it is now >>> also >>> possible to remove FOPEN_DIRECT_IO conditions in fuse_send_dio >>> (in a later patch). >>> >>> 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 f9d21804d313..0b3363eec435 100644 >>> --- a/fs/fuse/file.c >>> +++ b/fs/fuse/file.c >>> @@ -1602,52 +1602,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_io_past_eof(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_send_dio(&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; >>> @@ -1678,10 +1632,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; >> >> I think this affect the back-end behavior, changes a buffered IO to >> direct io. FOPEN_DIRECT_IO means no cache in front-end, but we should >> respect the back-end semantics. We may need another way to indicate >> "we need go the direct io code path while IOCB_DIRECT is not set >> though". > > I'm confused here, I guess with frontend you mean fuse kernel and with > backend fuse userspace (daemon/server). IOCB_DIRECT is not passed to > server/daemon, so there is no change? Although maybe I should document > in fuse_write_flags() to be careful with returned flags and that > IOCB_DIRECT cannot be trusted - if this should ever passed, one needs > to use struct file::flags & O_DIRECT. > I see, I misunderstood the code, `iocb->ki_filp->f_flags` not `iocb->ki_flags`... Thanks, Hao > > Thanks, > Bernd > > > Thanks, > Bernd
diff --git a/fs/fuse/file.c b/fs/fuse/file.c index f9d21804d313..0b3363eec435 100644 --- a/fs/fuse/file.c +++ b/fs/fuse/file.c @@ -1602,52 +1602,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_io_past_eof(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_send_dio(&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; @@ -1678,10 +1632,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)
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. Before it was using for FOPEN_DIRECT_IO 1) async (!is_sync_kiocb(iocb)) && IOCB_DIRECT fuse_file_write_iter fuse_direct_write_iter fuse_direct_IO fuse_send_dio 2) sync (is_sync_kiocb(iocb)) or IOCB_DIRECT not being set fuse_file_write_iter fuse_direct_write_iter fuse_send_dio 3) FOPEN_DIRECT_IO not set Same as the consolidates path below The new consolidated code path is always fuse_file_write_iter fuse_cache_write_iter generic_file_write_iter __generic_file_write_iter generic_file_direct_write mapping->a_ops->direct_IO / fuse_direct_IO fuse_send_dio So in general for FOPEN_DIRECT_IO the code path gets longer. Additionally fuse_direct_IO does an allocation of struct fuse_io_priv - might be a bit slower in micro benchmarks. Also, the IOCB_DIRECT information gets lost (as we now set it outselves). But then IOCB_DIRECT is directly related to O_DIRECT set in struct file::f_flags. An additional change is for condition 2 above, which might will now do async IO on the condition ff->fm->fc->async_dio. Given that async IO for FOPEN_DIRECT_IO was especially introduced in commit 'commit 23c94e1cdcbf ("fuse: Switch to using async direct IO for FOPEN_DIRECT_IO")' it should not matter. Especially as fuse_direct_IO is blocking for is_sync_kiocb(), at worst it has another slight overhead. Advantage is the removal of code paths and conditions and it is now also possible to remove FOPEN_DIRECT_IO conditions in fuse_send_dio (in a later patch). 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(-)