Re: [PATCH 4/5] [RFC] fuse: Set and use IOCB_DIRECT when FOPEN_DIRECT_IO is set

[Bernd Schubert <bernd.schubert@fastmail.fm>]

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