[PATCH] fuse: remove useless IOCB_DIRECT in fuse_direct_read/write_iter

[PATCH] fuse: remove useless IOCB_DIRECT in fuse_direct_read/write_iter

[yangyun]

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(-)

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);
-- 
2.33.0

Re: [PATCH] fuse: remove useless IOCB_DIRECT in fuse_direct_read/write_iter

[Miklos Szeredi]

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

Re: [PATCH] fuse: remove useless IOCB_DIRECT in fuse_direct_read/write_iter

[Jingbo Xu]

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.

-- 
Thanks,
Jingbo

Re: [PATCH] fuse: remove useless IOCB_DIRECT in fuse_direct_read/write_iter

[yangyun]

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

Re: [PATCH] fuse: remove useless IOCB_DIRECT in fuse_direct_read/write_iter

[Miklos Szeredi]

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

Re:[PATCH] fuse: remove useless IOCB_DIRECT in fuse_direct_read/write_iter

[yangyun]

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