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

[Christoph Hellwig <hch@lst.de>]

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?

> - 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.

> 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.

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

> +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.

Also the inline looks spurious.

> +LOOP_ATTR_RO(nr_wait_write);

nr_blocking_writes?

> +static inline void loop_inc_wait_write(struct loop_device *lo, struct loop_cmd *cmd)

Overly long line.

> +	if (cmd->use_aio){

missing whitespace.

> +		struct request *rq = blk_mq_rq_from_pdu(cmd);
> +
> +		if (req_op(rq) == REQ_OP_WRITE)
> +			lo->queued_wait_write += 1;


	if (cmd->use_aio && req_op(blk_mq_rq_from_pdu(cmd)) == REQ_OP_WRITE)
			lo->queued_wait_write++;

> +	}
> +}
> +
> +static inline void loop_dec_wait_write(struct loop_device *lo, struct loop_cmd *cmd)
> +{
> +	lockdep_assert_held(&lo->lo_mutex);
> +
> +	if (cmd->use_aio){
> +		struct request *rq = blk_mq_rq_from_pdu(cmd);
> +
> +		if (req_op(rq) == REQ_OP_WRITE)
> +			lo->queued_wait_write -= 1;
> +	}
> +}

All the things above apply here as well.