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.
