Re: [PATCH V2 5/5] loop: add hint for handling aio via IOCB_NOWAIT

[Ming Lei <ming.lei@redhat.com>]

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