| Message ID | 20230824150533.2788317-5-bschubert@ddn.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fuse direct write consolidation and parallel IO | expand |
On Thu, 24 Aug 2023 at 17:07, Bernd Schubert <bschubert@ddn.com> 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. > > 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 905ce3bb0047..09277a54b711 100644 > --- a/fs/fuse/file.c > +++ b/fs/fuse/file.c > @@ -1589,52 +1589,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_direct_write_extending_i_size(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_direct_io(&io, from, &iocb->ki_pos, > - FUSE_DIO_WRITE); > - fuse_write_update_attr(inode, iocb->ki_pos, res); While I think this is correct, I'd really like if the code to be replaced and the replacement are at least somewhat comparable. Currently fuse_direct_IO() handles all cases (of which are many since the requester can be sync or async and the server can be sync or async). Could this mess be cleaned up somehow? Also could we make the function names of fuse_direct_IO() and fuse_direct_io() less similar, as this is a very annoying (though minor) issue. Thanks, Miklos
On 8/28/23 13:59, Miklos Szeredi wrote: > On Thu, 24 Aug 2023 at 17:07, Bernd Schubert <bschubert@ddn.com> 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. >> >> 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 905ce3bb0047..09277a54b711 100644 >> --- a/fs/fuse/file.c >> +++ b/fs/fuse/file.c >> @@ -1589,52 +1589,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_direct_write_extending_i_size(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_direct_io(&io, from, &iocb->ki_pos, >> - FUSE_DIO_WRITE); >> - fuse_write_update_attr(inode, iocb->ki_pos, res); > > While I think this is correct, I'd really like if the code to be > replaced and the replacement are at least somewhat comparable. Sorry, I have a hard to time to understand "I'd really like if the code to be replaced". > > Currently fuse_direct_IO() handles all cases (of which are many since > the requester can be sync or async and the server can be sync or > async). > > Could this mess be cleaned up somehow? I guess what you mean is to make the the replacement more obvious? I can try... I need to think about how to do that. Before submitting the patch I had looked up different code paths and I think fuse_direct_IO (called by fuse_cache_write_iter -> generic_file_direct_write) all handles it. Maybe a new patch like this in fuse_file_write_iter if (condition1) fuse_cache_write_iter if (condition2) fuse_cache_write_iter ... and once all conditions in fuse_direct_write_iter are handled in fuse_file_write_iter another the final patch (what is current this 4/5) to remove fuse_direct_write_iter? > > Also could we make the function names of fuse_direct_IO() and > fuse_direct_io() less similar, as this is a very annoying (though > minor) issue. Entirely agreed, I had already thought about it, but wasn't sure why it was named like this and didn't want to change too much. Thanks, Bernd
On Mon, 28 Aug 2023 at 16:48, Bernd Schubert <bernd.schubert@fastmail.fm> wrote: > > On 8/28/23 13:59, Miklos Szeredi wrote: > > On Thu, 24 Aug 2023 at 17:07, Bernd Schubert <bschubert@ddn.com> wrote: > >> - if (!is_sync_kiocb(iocb) && iocb->ki_flags & IOCB_DIRECT) { > >> - res = fuse_direct_IO(iocb, from); > >> - } else { > >> - res = fuse_direct_io(&io, from, &iocb->ki_pos, > >> - FUSE_DIO_WRITE); > >> - fuse_write_update_attr(inode, iocb->ki_pos, res); > > > > While I think this is correct, I'd really like if the code to be > > replaced and the replacement are at least somewhat comparable. > > Sorry, I have a hard to time to understand "I'd really like if the code > to be replaced". What I meant is that generic_file_direct_write() is not an obvious replacement for the above lines of code. The reason is that fuse_direct_IO() is handling the sync and async cases in one function, while the above splits handling it based on IOCB_DIRECT (which is now lost) and is_sync_kiocb(iocb). If it's okay to lose IOCB_DIRECT then what's the explanation for the above condition? It could be historic garbage, but we still need to understand what is exactly happening. Thanks, Miklos
On 8/28/23 13:59, Miklos Szeredi wrote: > Also could we make the function names of fuse_direct_IO() and > fuse_direct_io() less similar, as this is a very annoying (though > minor) issue. What about _fuse_do_direct_io()? '_' to mark that it is a follow up and 'do' that it initiates the transfer? Or maybe just '_fuse_direct_io'? Or 'fuse_send_dio'? Thanks, Bernd
On Mon, 28 Aug 2023 at 22:03, Bernd Schubert <bernd.schubert@fastmail.fm> wrote: > > > > On 8/28/23 13:59, Miklos Szeredi wrote: > > Also could we make the function names of fuse_direct_IO() and > > fuse_direct_io() less similar, as this is a very annoying (though > > minor) issue. > > What about _fuse_do_direct_io()? '_' to mark that it is a follow up and > 'do' that it initiates the transfer? Or maybe just '_fuse_direct_io'? Or > 'fuse_send_dio'? I'd prefer a non-underscore variant. fuse_send_dio() sounds good. Thanks, Miklos
On 8/28/23 17:05, Miklos Szeredi wrote: > On Mon, 28 Aug 2023 at 16:48, Bernd Schubert <bernd.schubert@fastmail.fm> wrote: >> >> On 8/28/23 13:59, Miklos Szeredi wrote: >>> On Thu, 24 Aug 2023 at 17:07, Bernd Schubert <bschubert@ddn.com> wrote: > >>>> - if (!is_sync_kiocb(iocb) && iocb->ki_flags & IOCB_DIRECT) { >>>> - res = fuse_direct_IO(iocb, from); >>>> - } else { >>>> - res = fuse_direct_io(&io, from, &iocb->ki_pos, >>>> - FUSE_DIO_WRITE); >>>> - fuse_write_update_attr(inode, iocb->ki_pos, res); >>> >>> While I think this is correct, I'd really like if the code to be >>> replaced and the replacement are at least somewhat comparable. >> >> Sorry, I have a hard to time to understand "I'd really like if the code >> to be replaced". > > What I meant is that generic_file_direct_write() is not an obvious > replacement for the above lines of code. > > The reason is that fuse_direct_IO() is handling the sync and async > cases in one function, while the above splits handling it based on > IOCB_DIRECT (which is now lost) and is_sync_kiocb(iocb). If it's okay > to lose IOCB_DIRECT then what's the explanation for the above > condition? It could be historic garbage, but we still need to > understand what is exactly happening. While checking all code path again, I found an additional difference, which I had missed before. FOPEN_DIRECT_IO will now act on ff->fm->fc->async_dio when is is_sync_kiocb(iocb) is set. Do you think that is a problem? If so, I could fix it in fuse_direct_IO. Thanks, Bernd
On 8/29/23 15:08, Bernd Schubert wrote: > > > On 8/28/23 17:05, Miklos Szeredi wrote: >> On Mon, 28 Aug 2023 at 16:48, Bernd Schubert >> <bernd.schubert@fastmail.fm> wrote: >>> >>> On 8/28/23 13:59, Miklos Szeredi wrote: >>>> On Thu, 24 Aug 2023 at 17:07, Bernd Schubert <bschubert@ddn.com> wrote: >> >>>>> - if (!is_sync_kiocb(iocb) && iocb->ki_flags & >>>>> IOCB_DIRECT) { >>>>> - res = fuse_direct_IO(iocb, from); >>>>> - } else { >>>>> - res = fuse_direct_io(&io, from, &iocb->ki_pos, >>>>> - FUSE_DIO_WRITE); >>>>> - fuse_write_update_attr(inode, iocb->ki_pos, >>>>> res); >>>> >>>> While I think this is correct, I'd really like if the code to be >>>> replaced and the replacement are at least somewhat comparable. >>> >>> Sorry, I have a hard to time to understand "I'd really like if the code >>> to be replaced". >> >> What I meant is that generic_file_direct_write() is not an obvious >> replacement for the above lines of code. >> >> The reason is that fuse_direct_IO() is handling the sync and async >> cases in one function, while the above splits handling it based on >> IOCB_DIRECT (which is now lost) and is_sync_kiocb(iocb). If it's okay >> to lose IOCB_DIRECT then what's the explanation for the above >> condition? It could be historic garbage, but we still need to >> understand what is exactly happening. > > While checking all code path again, I found an additional difference, > which I had missed before. FOPEN_DIRECT_IO will now act on > ff->fm->fc->async_dio when is is_sync_kiocb(iocb) is set. > > Do you think that is a problem? If so, I could fix it in fuse_direct_IO. What I mean is something like this diff --git a/fs/fuse/file.c b/fs/fuse/file.c index 85f2f9d3813e..3b383dc8a944 100644 --- a/fs/fuse/file.c +++ b/fs/fuse/file.c @@ -1635,8 +1635,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) + if (ff->open_flags & FOPEN_DIRECT_IO) { iocb->ki_flags |= IOCB_DIRECT; + ff->iocb_direct = 1; + } return fuse_cache_write_iter(iocb, from); } @@ -2905,6 +2907,15 @@ fuse_direct_IO(struct kiocb *iocb, struct iov_iter *iter) io->iocb = iocb; io->blocking = is_sync_kiocb(iocb); + /* FOPEN_DIRECT_IO historically does not use async for blocking O_DIRECT */ + if (ff->open_flags & FOPEN_DIRECT_IO) { + if (!is_sync_kiocb(iocb) && ff->iocb_direct) { + /* no change */ + } else { + io->async = 0; + } + } + /* optimization for short read */ if (io->async && !io->write && offset + count > i_size) { iov_iter_truncate(iter, fuse_round_up(ff->fm->fc, i_size - offset)); diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h index a56e83b7d29a..d77046875ad5 100644 --- a/fs/fuse/fuse_i.h +++ b/fs/fuse/fuse_i.h @@ -231,6 +231,9 @@ struct fuse_file { /** Has flock been performed on this file? */ bool flock:1; + + /** Has the file been opened with O_DIRECT? */ + bool iocb_direct:1; }; /** One input argument of a request */ Thanks, Bernd
On 8/29/23 15:26, Bernd Schubert wrote: > > > On 8/29/23 15:08, Bernd Schubert wrote: >> >> >> On 8/28/23 17:05, Miklos Szeredi wrote: >>> On Mon, 28 Aug 2023 at 16:48, Bernd Schubert >>> <bernd.schubert@fastmail.fm> wrote: >>>> >>>> On 8/28/23 13:59, Miklos Szeredi wrote: >>>>> On Thu, 24 Aug 2023 at 17:07, Bernd Schubert <bschubert@ddn.com> >>>>> wrote: >>> >>>>>> - if (!is_sync_kiocb(iocb) && iocb->ki_flags & >>>>>> IOCB_DIRECT) { >>>>>> - res = fuse_direct_IO(iocb, from); >>>>>> - } else { >>>>>> - res = fuse_direct_io(&io, from, >>>>>> &iocb->ki_pos, >>>>>> - FUSE_DIO_WRITE); >>>>>> - fuse_write_update_attr(inode, >>>>>> iocb->ki_pos, res); >>>>> >>>>> While I think this is correct, I'd really like if the code to be >>>>> replaced and the replacement are at least somewhat comparable. >>>> >>>> Sorry, I have a hard to time to understand "I'd really like if the code >>>> to be replaced". >>> >>> What I meant is that generic_file_direct_write() is not an obvious >>> replacement for the above lines of code. >>> >>> The reason is that fuse_direct_IO() is handling the sync and async >>> cases in one function, while the above splits handling it based on >>> IOCB_DIRECT (which is now lost) and is_sync_kiocb(iocb). If it's okay >>> to lose IOCB_DIRECT then what's the explanation for the above >>> condition? It could be historic garbage, but we still need to >>> understand what is exactly happening. >> >> While checking all code path again, I found an additional difference, >> which I had missed before. FOPEN_DIRECT_IO will now act on >> ff->fm->fc->async_dio when is is_sync_kiocb(iocb) is set. >> >> Do you think that is a problem? If so, I could fix it in fuse_direct_IO. > > What I mean is something like this > > + /* FOPEN_DIRECT_IO historically does not use async for blocking > O_DIRECT */ > + if (ff->open_flags & FOPEN_DIRECT_IO) { > + if (!is_sync_kiocb(iocb) && ff->iocb_direct) { > + /* no change */ > + } else { > + io->async = 0; > + } > + } Besides that it could use file->f_flags & O_DIRECT, I guess we can keep async. It relates to commit 23c94e1cdcbf5953cd380555d0781caa42311870, which actually introduced async for FOPEN_DIRECT_IO. I'm just going to add it to the commit message. Thanks, Bernd
diff --git a/fs/fuse/file.c b/fs/fuse/file.c index 905ce3bb0047..09277a54b711 100644 --- a/fs/fuse/file.c +++ b/fs/fuse/file.c @@ -1589,52 +1589,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_direct_write_extending_i_size(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_direct_io(&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; @@ -1665,10 +1619,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. 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(-)