| Message ID | 20240826130612.2641750-1-yangyun50@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fuse: remove useless IOCB_DIRECT in fuse_direct_read/write_iter | expand |
On Mon, 26 Aug 2024 at 15:07, yangyun <yangyun50@huawei.com> wrote: > > Commit 23c94e1cdcbf ("fuse: Switch to using async direct IO > for FOPEN_DIRECT_IO") gave the async direct IO code path in the > fuse_direct_read_iter() and fuse_direct_write_iter(). But since > these two functions are only called under FOPEN_DIRECT_IO is set, > it seems that we can also use the async direct IO even the flag > IOCB_DIRECT is not set to enjoy the async direct IO method. Also > move the definition of fuse_io_priv to where it is used in fuse_ > direct_write_iter. I'm interested in the motivation for this patch. There's a minor risk of regressions when introducing such a behavior change, so there should also be a strong supporting argument, which seems to be missing in this case. Thanks, Miklos
Hi Miklos, On 8/27/24 3:12 AM, Miklos Szeredi wrote: > On Mon, 26 Aug 2024 at 15:07, yangyun <yangyun50@huawei.com> wrote: >> >> Commit 23c94e1cdcbf ("fuse: Switch to using async direct IO >> for FOPEN_DIRECT_IO") gave the async direct IO code path in the >> fuse_direct_read_iter() and fuse_direct_write_iter(). But since >> these two functions are only called under FOPEN_DIRECT_IO is set, >> it seems that we can also use the async direct IO even the flag >> IOCB_DIRECT is not set to enjoy the async direct IO method. Also >> move the definition of fuse_io_priv to where it is used in fuse_ >> direct_write_iter. > > I'm interested in the motivation for this patch. > > There's a minor risk of regressions when introducing such a behavior > change, so there should also be a strong supporting argument, which > seems to be missing in this case. > I'm not sure what yangyun's use case is, but we indeed also observed a potential performance optimization for FOPEN_DIRECT_IO path. When the buffer IO is submitted to a file flagged with FOPEN_DIRECT_IO, the code path is like: fuse_direct_read_iter __fuse_direct_read fuse_direct_io # split the request to multiple fuse requests according to # max_read and max_pages constraint, for each split request: fuse_send_read fuse_simple_request When the size of the user requested IO is greater than max_read and max_pages constraint, it's split into multiple requests and these split requests can not be sent to the fuse server until the previous split request *completes* (since fuse_simple_request()), even when the user request is submitted from async IO e.g. io-uring.
On Mon, Aug 26, 2024 at 09:12:39PM +0200, Miklos Szeredi wrote: > On Mon, 26 Aug 2024 at 15:07, yangyun <yangyun50@huawei.com> wrote: > > > > Commit 23c94e1cdcbf ("fuse: Switch to using async direct IO > > for FOPEN_DIRECT_IO") gave the async direct IO code path in the > > fuse_direct_read_iter() and fuse_direct_write_iter(). But since > > these two functions are only called under FOPEN_DIRECT_IO is set, > > it seems that we can also use the async direct IO even the flag > > IOCB_DIRECT is not set to enjoy the async direct IO method. Also > > move the definition of fuse_io_priv to where it is used in fuse_ > > direct_write_iter. > > I'm interested in the motivation for this patch. > > There's a minor risk of regressions when introducing such a behavior > change, so there should also be a strong supporting argument, which > seems to be missing in this case. Thanks for your reply! It seems that there is a risk of regressions. But I think adding an argument in this case is not so graceful, whatever adding this argument to the `struct fuse_file->open_flags` or adding it to the init flags in `struct fuse_init_args`. The reasons are: 1. Commit 23c94e1cdcbf ("fuse: Switch to using async direct IO for FOPEN_DIRECT_IO") also changes the behavior from sync to async direct io, but does not import a new argument to avoid the risk of regressions. 2. Fuse already has an init flags FUSE_ASYNC_DIO in `fuse_init_args`, which indicates that the direct io should be submitted asynchrounously. The comment in function `fuse_direct_IO()` also indicates the situation: " /* * By default, we want to optimize all I/Os with async request * submission to the client filesystem if supported. */ " But the code does not go through the async direct io code path in the case described in current patch. 3. If adding a argument, it would be so many arguments about async and direct io (FUSE_ASYNC_DIO, FUSE_ASYNC_READ, FOPEN_DIRECT_IO, etc), which may be redundant and confuse the developers about their differences. What do you think ? > > Thanks, > Miklos
On Tue, Aug 27, 2024 at 04:30:04PM +0800, Jingbo Xu wrote: > Hi Miklos, > > On 8/27/24 3:12 AM, Miklos Szeredi wrote: > > On Mon, 26 Aug 2024 at 15:07, yangyun <yangyun50@huawei.com> wrote: > >> > >> Commit 23c94e1cdcbf ("fuse: Switch to using async direct IO > >> for FOPEN_DIRECT_IO") gave the async direct IO code path in the > >> fuse_direct_read_iter() and fuse_direct_write_iter(). But since > >> these two functions are only called under FOPEN_DIRECT_IO is set, > >> it seems that we can also use the async direct IO even the flag > >> IOCB_DIRECT is not set to enjoy the async direct IO method. Also > >> move the definition of fuse_io_priv to where it is used in fuse_ > >> direct_write_iter. > > > > I'm interested in the motivation for this patch. > > > > There's a minor risk of regressions when introducing such a behavior > > change, so there should also be a strong supporting argument, which > > seems to be missing in this case. > > > > > I'm not sure what yangyun's use case is, but we indeed also observed a > potential performance optimization for FOPEN_DIRECT_IO path. When the > buffer IO is submitted to a file flagged with FOPEN_DIRECT_IO, the code > path is like: > > fuse_direct_read_iter > __fuse_direct_read > fuse_direct_io > # split the request to multiple fuse requests according to > # max_read and max_pages constraint, for each split request: > fuse_send_read > fuse_simple_request > > When the size of the user requested IO is greater than max_read and > max_pages constraint, it's split into multiple requests and these split > requests can not be sent to the fuse server until the previous split > request *completes* (since fuse_simple_request()), even when the user > request is submitted from async IO e.g. io-uring. The same use case. Your explanation is more explicit. And I just don't know why commit 23c94e1cdcbf ("fuse: Switch to using async direct IO for FOPEN_DIRECT_IO") adds the check of IOCB_DIRECT flag when using async direct_io. It seems unnessary. > > -- > Thanks, > Jingbo
On Tue, 27 Aug 2024 at 13:53, yangyun <yangyun50@huawei.com> wrote: > > On Tue, Aug 27, 2024 at 04:30:04PM +0800, Jingbo Xu wrote: > > When the size of the user requested IO is greater than max_read and > > max_pages constraint, it's split into multiple requests and these split > > requests can not be sent to the fuse server until the previous split > > request *completes* (since fuse_simple_request()), even when the user > > request is submitted from async IO e.g. io-uring. > > The same use case. Your explanation is more explicit. Applied, thanks. Miklos
diff --git a/fs/fuse/file.c b/fs/fuse/file.c index f39456c65ed7..03809ecc23ec 100644 --- a/fs/fuse/file.c +++ b/fs/fuse/file.c @@ -1649,7 +1649,7 @@ static ssize_t fuse_direct_read_iter(struct kiocb *iocb, struct iov_iter *to) { ssize_t res; - if (!is_sync_kiocb(iocb) && iocb->ki_flags & IOCB_DIRECT) { + if (!is_sync_kiocb(iocb)) { res = fuse_direct_IO(iocb, to); } else { struct fuse_io_priv io = FUSE_IO_PRIV_SYNC(iocb); @@ -1663,7 +1663,6 @@ static ssize_t fuse_direct_read_iter(struct kiocb *iocb, struct iov_iter *to) 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; @@ -1671,9 +1670,11 @@ static ssize_t fuse_direct_write_iter(struct kiocb *iocb, struct iov_iter *from) res = generic_write_checks(iocb, from); if (res > 0) { task_io_account_write(res); - if (!is_sync_kiocb(iocb) && iocb->ki_flags & IOCB_DIRECT) { + if (!is_sync_kiocb(iocb)) { res = fuse_direct_IO(iocb, from); } else { + struct fuse_io_priv io = FUSE_IO_PRIV_SYNC(iocb); + res = fuse_direct_io(&io, from, &iocb->ki_pos, FUSE_DIO_WRITE); fuse_write_update_attr(inode, iocb->ki_pos, res);
Commit 23c94e1cdcbf ("fuse: Switch to using async direct IO for FOPEN_DIRECT_IO") gave the async direct IO code path in the fuse_direct_read_iter() and fuse_direct_write_iter(). But since these two functions are only called under FOPEN_DIRECT_IO is set, it seems that we can also use the async direct IO even the flag IOCB_DIRECT is not set to enjoy the async direct IO method. Also move the definition of fuse_io_priv to where it is used in fuse_ direct_write_iter. Signed-off-by: yangyun <yangyun50@huawei.com> --- fs/fuse/file.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)