On Wed, Aug 16, 2023 at 10:13 PM Jens Axboe <axboe@xxxxxxxxx> wrote:
>
> 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.
>
All right.
> >> 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.
>
Ok. I got rid of IOCB_WRITE_STARTED, so there is no conflict now.
Thanks,
Amir.
