Re: [RESEND PATCH 4/5] loop: try to handle loop aio command via NOWAIT IO first

[Christoph Hellwig <hch@infradead.org>]

On Sun, Mar 09, 2025 at 12:23:08AM +0800, Ming Lei wrote:
> Try to handle loop aio command via NOWAIT IO first, then we can avoid to
> queue the aio command into workqueue.
> 
> Fallback to workqueue in case of -EAGAIN.
> 
> BLK_MQ_F_BLOCKING has to be set for calling into .read_iter() or
> .write_iter() which might sleep even though it is NOWAIT.

This needs performance numbers (or other reasons) justifying the
change, especially as BLK_MQ_F_BLOCKING is a bit of an overhead.

>  static DEFINE_IDR(loop_index_idr);
>  static DEFINE_MUTEX(loop_ctl_mutex);
>  static DEFINE_MUTEX(loop_validate_mutex);
> @@ -380,8 +382,17 @@ static void lo_rw_aio_do_completion(struct loop_cmd *cmd)
>  
>  	if (!atomic_dec_and_test(&cmd->ref))
>  		return;
> +
> +	if (cmd->ret == -EAGAIN) {
> +		struct loop_device *lo = rq->q->queuedata;
> +
> +		loop_queue_work(lo, cmd);
> +		return;
> +	}

This looks like the wrong place for the rety, as -EAGAIN can only come from
the submissions path.  i.e. we should never make it to the full completion
path for that case.

>  static int lo_rw_aio(struct loop_device *lo, struct loop_cmd *cmd, loff_t pos)
> +{
> +	unsigned int nr_bvec = lo_cmd_nr_bvec(cmd);
> +	int ret;
> +
> +	cmd->iocb.ki_flags &= ~IOCB_NOWAIT;
> +	ret = lo_submit_rw_aio(lo, cmd, pos, nr_bvec);
> +	if (ret != -EIOCBQUEUED)
> +		lo_rw_aio_complete(&cmd->iocb, ret);
> +	return 0;

This needs an explanation that it is for the fallback path and thus
clears the nowait flag.

> +}
> +
> +static int lo_rw_aio_nowait(struct loop_device *lo, struct loop_cmd *cmd, loff_t pos)

Overly long line.

> @@ -1926,6 +1955,17 @@ static blk_status_t loop_queue_rq(struct blk_mq_hw_ctx *hctx,
>  		break;
>  	}
>  
> +	if (cmd->use_aio) {
> +		loff_t pos = ((loff_t) blk_rq_pos(rq) << 9) + lo->lo_offset;
> +		int ret = lo_rw_aio_nowait(lo, cmd, pos);
> +
> +		if (!ret)
> +			return BLK_STS_OK;
> +		if (ret != -EAGAIN)
> +			return BLK_STS_IOERR;
> +		/* fallback to workqueue for handling aio */
> +	}

Why isn't all the logic in this branch in lo_rw_aio_nowait?