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
