On Wed, Aug 16, 2023 at 6:28 PM Jens Axboe <axboe@xxxxxxxxx> wrote:
>
> On 8/16/23 2:54 AM, Amir Goldstein wrote:
> > aio, io_uring, cachefiles and overlayfs, all open code an ugly variant
> > of file_{start,end}_write() to silence lockdep warnings.
> >
> > Create helpers for this lockdep dance and use the helpers in all the
> > callers.
> >
> > Use a new iocb flag IOCB_WRITE_STARTED to indicate if sb_start_write()
> > was called.
>
> Looks better now, but I think you should split this into a prep patch
> that adds the helpers, and then one for each conversion. We've had bugs
> with this accounting before which causes fs freeze issues, would be
> prudent to have it split because of that.
>
Sure, no problem.
> > 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.
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.
> > 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.
Thanks,
Amir.
