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@xxxxxxxxx> 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?
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.
