On Thu, Mar 20, 2025 at 08:22:47AM +0100, Christoph Hellwig wrote:
> On Fri, Mar 14, 2025 at 10:11:45AM +0800, Ming Lei wrote:
> > Add hint for using IOCB_NOWAIT to handle loop aio command for avoiding
> > to cause write(especially randwrite) perf regression on sparse file.
> >
> > Try IOCB_NOWAIT in the following situations:
> >
> > - backing file is block device
>
> Why limit yourself to block devices?
It doesn't limit to block device, just submit NOWAIT unconditionally.
I should have added 'OR' among the three lines.
>
> > - READ aio command
> > - there isn't queued aio non-NOWAIT WRITE, since retry of NOWAIT won't
> > cause contention on WRITE and non-NOWAIT WRITE often implies exclusive
> > lock.
>
> This reads really odd because to me the list implies that you only
> support reads, but the code also supports writes. Maybe try to
> explain this more clearly.
Will improve the comment log.
>
> > With this simple policy, perf regression of randwrte/write on sparse
> > backing file is fixed. Meantime this way addresses perf problem[1] in
> > case of stable FS block mapping via NOWAIT.
>
> This needs to go in with the patch implementing the logic.
OK.
>
> > @@ -70,6 +70,7 @@ struct loop_device {
> > struct rb_root worker_tree;
> > struct timer_list timer;
> > bool sysfs_inited;
> > + unsigned queued_wait_write;
>
> lo_nr_blocking_writes?
>
> What serializes access to this variable?
The write is serialized by the loop spin lock, and the read is done
via READ_ONCE(), since it is just a hint.
>
> > +static inline bool lo_aio_need_try_nowait(struct loop_device *lo,
> > + struct loop_cmd *cmd)
>
> Drop the need_ in the name, it not only is superfluous, but also
> makes it really hard to read the function name.
OK.
Thanks,
Ming
