Re: [PATCH 5/6] fuse: Remove fuse_direct_write_iter code path / use IOCB_DIRECT

[Hao Xu <hao.xu@linux.dev>]

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