Re: [PATCH] fs: create kiocb_{start,end}_write() helpers

[Jens Axboe <axboe@kernel.dk>]

On 8/16/23 2:51 AM, Christian Brauner wrote:
> On Tue, Aug 15, 2023 at 03:16:15PM -0600, Jens Axboe wrote:
>> On 8/15/23 12:48 PM, Amir Goldstein wrote:
>>> On Tue, Aug 15, 2023 at 8:06?PM Jens Axboe <axboe@kernel.dk> wrote:
>>>>
>>>> On 8/15/23 11:02 AM, Jens Axboe wrote:
>>>>> On 8/15/23 10:57 AM, Amir Goldstein wrote:
>>>>>> +/**
>>>>>> + * kiocb_start_write - get write access to a superblock for async file io
>>>>>> + * @iocb: the io context we want to submit the write with
>>>>>> + *
>>>>>> + * This is a variant of file_start_write() for async io submission.
>>>>>> + * Should be matched with a call to kiocb_end_write().
>>>>>> + */
>>>>>> +static inline void kiocb_start_write(struct kiocb *iocb)
>>>>>> +{
>>>>>> +    struct inode *inode = file_inode(iocb->ki_filp);
>>>>>> +
>>>>>> +    iocb->ki_flags |= IOCB_WRITE;
>>>>>> +    if (WARN_ON_ONCE(iocb->ki_flags & IOCB_WRITE_STARTED))
>>>>>> +            return;
>>>>>> +    if (!S_ISREG(inode->i_mode))
>>>>>> +            return;
>>>>>> +    sb_start_write(inode->i_sb);
>>>>>> +    /*
>>>>>> +     * Fool lockdep by telling it the lock got released so that it
>>>>>> +     * doesn't complain about the held lock when we return to userspace.
>>>>>> +     */
>>>>>> +    __sb_writers_release(inode->i_sb, SB_FREEZE_WRITE);
>>>>>> +    iocb->ki_flags |= IOCB_WRITE_STARTED;
>>>>>> +}
>>>>>> +
>>>>>> +/**
>>>>>> + * kiocb_end_write - drop write access to a superblock after async file io
>>>>>> + * @iocb: the io context we sumbitted the write with
>>>>>> + *
>>>>>> + * Should be matched with a call to kiocb_start_write().
>>>>>> + */
>>>>>> +static inline void kiocb_end_write(struct kiocb *iocb)
>>>>>> +{
>>>>>> +    struct inode *inode = file_inode(iocb->ki_filp);
>>>>>> +
>>>>>> +    if (!(iocb->ki_flags & IOCB_WRITE_STARTED))
>>>>>> +            return;
>>>>>> +    if (!S_ISREG(inode->i_mode))
>>>>>> +            return;
>>>>
>>>> And how would IOCB_WRITE_STARTED ever be set, if S_ISREG() isn't true?
>>>
>>> Good point.
>>> I will pass is_reg argument from callers of kiocb_start_write() and
>>> will only check IOCB_WRITE_STARTED in kiocb_end_write().
>>
>> Please don't pass in an argument that just makes the function do
>> nothing. Just gate calling the function on it instead.
> 
> Your commit about avoiding dipping into inodes unnecessarily when not
> all callers need it is for perf reasons or what's the worry?

Right, it's to avoid pulling in dependent loads for the cases where you
don't need to access it. Granted for this it's not super important as
most workloads using read/write would be a regular file to begin with,
and then we'd dip into the inode anyway for the lock dance. But it's
good practice in general, and if you were eg using pipes then it becomes
just wasted dependent loads.

> Fwiw, I don't mind if we force the callers to check for prerequisites
> instead of the helpers. I'm just curious what the thinking behind it is.
> 
> Otherwise I think a cleanup like this might be useful.

Agree, it's a useful cleanup.

-- 
Jens Axboe