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

[Jens Axboe <axboe@kernel.dk>]

On 8/16/23 12:58 PM, Amir Goldstein wrote:
>>> diff --git a/fs/aio.c b/fs/aio.c
>>> index 77e33619de40..16fb3ac2093b 100644
>>> --- a/fs/aio.c
>>> +++ b/fs/aio.c
>>> @@ -1444,17 +1444,8 @@ static void aio_complete_rw(struct kiocb *kiocb, long res)
>>>       if (!list_empty_careful(&iocb->ki_list))
>>>               aio_remove_iocb(iocb);
>>>
>>> -     if (kiocb->ki_flags & IOCB_WRITE) {
>>> -             struct inode *inode = file_inode(kiocb->ki_filp);
>>> -
>>> -             /*
>>> -              * Tell lockdep we inherited freeze protection from submission
>>> -              * thread.
>>> -              */
>>> -             if (S_ISREG(inode->i_mode))
>>> -                     __sb_writers_acquired(inode->i_sb, SB_FREEZE_WRITE);
>>> -             file_end_write(kiocb->ki_filp);
>>> -     }
>>> +     if (kiocb->ki_flags & IOCB_WRITE)
>>> +             kiocb_end_write(kiocb);
>>
>> Can't we just call kiocb_end_write() here, it checks WRITE_STARTED
>> anyway? Not a big deal, and honestly I'd rather just get rid of
>> WRITE_STARTED if we're not using it like that. It doesn't serve much of
>> a purpose, if we're gating this one IOCB_WRITE anyway (which I do like
>> better than WRITE_STARTED). And it avoids writing to the kiocb at the
>> end too, which is a nice win.
>>
> 
> What I don't like about it is that kiocb_{start,end}_write() calls will
> not be balanced, so it is harder for a code reviewer to realize that the
> code is correct (as opposed to have excess end_write calls).
> That's the reason I had ISREG check inside the helpers in v1, so like
> file_{start,end}_write(), the calls will be balanced and easy to review.

If you just gate it on IOCB_WRITE and local IS_REG test, then it should
be balanced.

> I am not sure, but I think that getting rid of WRITE_STARTED will
> make the code more subtle, because right now, IOCB_WRITE is
> not set by kiocb_start_write() and many times it is set much sooner.
> So I'd rather keep the WRITE_STARTED flag for a more roust code
> if that's ok with you.

Adding random flags to protect against that is not a good idea imho. It
adds overhead and while it may seem like it's hardening, it's also then
just making it easier to misuse.

I would start with just adding the helpers, and having the callers gate
them with IOCB_WRITE && IS_REG like they do now.

>> index b2adee67f9b2..8e5d410a1be5 100644
>>> --- a/include/linux/fs.h
>>> +++ b/include/linux/fs.h
>>> @@ -338,6 +338,8 @@ enum rw_hint {
>>>  #define IOCB_NOIO            (1 << 20)
>>>  /* can use bio alloc cache */
>>>  #define IOCB_ALLOC_CACHE     (1 << 21)
>>> +/* file_start_write() was called */
>>> +#define IOCB_WRITE_STARTED   (1 << 22)
>>>
>>>  /* for use in trace events */
>>>  #define TRACE_IOCB_STRINGS \
>>> @@ -351,7 +353,8 @@ enum rw_hint {
>>>       { IOCB_WRITE,           "WRITE" }, \
>>>       { IOCB_WAITQ,           "WAITQ" }, \
>>>       { IOCB_NOIO,            "NOIO" }, \
>>> -     { IOCB_ALLOC_CACHE,     "ALLOC_CACHE" }
>>> +     { IOCB_ALLOC_CACHE,     "ALLOC_CACHE" }, \
>>> +     { IOCB_WRITE_STARTED,   "WRITE_STARTED" }
>>>
>>>  struct kiocb {
>>>       struct file             *ki_filp;
>>
>> These changes will conflict with other changes in linux-next that are
>> going upstream. I'd prefer to stage this one after those changes, once
>> we get to a version that looks good to everybody.
> 
> Changes coming to linux-next from your tree?
> I was basing my patches on Christian's vfs.all branch.
> Do you mean that you would like to stage this cleanup?
> It's fine by me.

From the xfs tree, if you look at linux-next you should see them. I
don't care who stages it, but I don't want to create needless merge
conflicts because people weren't aware of these conflicts.

-- 
Jens Axboe