On Mon, Sep 1, 2025 at 3:03 AM Ming Lei <ming.lei@xxxxxxxxxx> wrote:
>
> Refactor auto buffer register code and prepare for supporting batch IO
> feature, and the main motivation is to put 'ublk_io' operation code
> together, so that per-io lock can be applied for the code block.
>
> The key changes are:
> - Rename ublk_auto_buf_reg() as ublk_do_auto_buf_reg()
Thanks, the type and the function having the same name was a minor annoyance.
> - Introduce an enum `auto_buf_reg_res` to represent the result of
> the buffer registration attempt (FAIL, FALLBACK, OK).
> - Split the existing `ublk_do_auto_buf_reg` function into two:
> - `__ublk_do_auto_buf_reg`: Performs the actual buffer registration
> and returns the `auto_buf_reg_res` status.
> - `ublk_do_auto_buf_reg`: A wrapper that calls the internal function
> and handles the I/O preparation based on the result.
> - Introduce `ublk_prep_auto_buf_reg_io` to encapsulate the logic for
> preparing the I/O for completion after buffer registration.
> - Pass the `tag` directly to `ublk_auto_buf_reg_fallback` to avoid
> recalculating it.
>
> This refactoring makes the control flow clearer and isolates the different
> stages of the auto buffer registration process.
>
> Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx>
> ---
> drivers/block/ublk_drv.c | 65 +++++++++++++++++++++++++++-------------
> 1 file changed, 44 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index 9185978abeb7..e53f623b0efe 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -1205,17 +1205,36 @@ static inline void __ublk_abort_rq(struct ublk_queue *ubq,
> }
>
> static void
> -ublk_auto_buf_reg_fallback(const struct ublk_queue *ubq, struct ublk_io *io)
> +ublk_auto_buf_reg_fallback(const struct ublk_queue *ubq, unsigned tag)
> {
> - unsigned tag = io - ubq->ios;
The reason to calculate the tag like this was to avoid the pointer
dereference in req->tag. But req->tag is already accessed just prior
in ublk_dispatch_req(), so it should be cached and not too expensive
to load again.
> struct ublksrv_io_desc *iod = ublk_get_iod(ubq, tag);
>
> iod->op_flags |= UBLK_IO_F_NEED_REG_BUF;
> }
>
> -static bool ublk_auto_buf_reg(const struct ublk_queue *ubq, struct request *req,
> - struct ublk_io *io, struct io_uring_cmd *cmd,
> - unsigned int issue_flags)
> +enum auto_buf_reg_res {
> + AUTO_BUF_REG_FAIL,
> + AUTO_BUF_REG_FALLBACK,
> + AUTO_BUF_REG_OK,
> +};
nit: move this enum definition next to the function that returns it?
> +
> +static void ublk_prep_auto_buf_reg_io(const struct ublk_queue *ubq,
> + struct request *req, struct ublk_io *io,
> + struct io_uring_cmd *cmd, bool registered)
How about passing enum auto_buf_reg_res instead of bool registered to
avoid the duplicated == AUTO_BUF_REG_OK in the callers?
> +{
> + if (registered) {
> + io->task_registered_buffers = 1;
> + io->buf_ctx_handle = io_uring_cmd_ctx_handle(cmd);
> + io->flags |= UBLK_IO_FLAG_AUTO_BUF_REG;
> + }
> + ublk_init_req_ref(ubq, io);
> + __ublk_prep_compl_io_cmd(io, req);
> +}
> +
> +static enum auto_buf_reg_res
> +__ublk_do_auto_buf_reg(const struct ublk_queue *ubq, struct request *req,
> + struct ublk_io *io, struct io_uring_cmd *cmd,
> + unsigned int issue_flags)
> {
> int ret;
>
> @@ -1223,29 +1242,27 @@ static bool ublk_auto_buf_reg(const struct ublk_queue *ubq, struct request *req,
> io->buf.auto_reg.index, issue_flags);
> if (ret) {
> if (io->buf.auto_reg.flags & UBLK_AUTO_BUF_REG_FALLBACK) {
> - ublk_auto_buf_reg_fallback(ubq, io);
> - return true;
> + ublk_auto_buf_reg_fallback(ubq, req->tag);
> + return AUTO_BUF_REG_FALLBACK;
> }
> blk_mq_end_request(req, BLK_STS_IOERR);
> - return false;
> + return AUTO_BUF_REG_FAIL;
> }
>
> - io->task_registered_buffers = 1;
> - io->buf_ctx_handle = io_uring_cmd_ctx_handle(cmd);
> - io->flags |= UBLK_IO_FLAG_AUTO_BUF_REG;
> - return true;
> + return AUTO_BUF_REG_OK;
> }
>
> -static bool ublk_prep_auto_buf_reg(struct ublk_queue *ubq,
> - struct request *req, struct ublk_io *io,
> - struct io_uring_cmd *cmd,
> - unsigned int issue_flags)
> +static void ublk_do_auto_buf_reg(const struct ublk_queue *ubq, struct request *req,
> + struct ublk_io *io, struct io_uring_cmd *cmd,
> + unsigned int issue_flags)
> {
> - ublk_init_req_ref(ubq, io);
> - if (ublk_support_auto_buf_reg(ubq) && ublk_rq_has_data(req))
> - return ublk_auto_buf_reg(ubq, req, io, cmd, issue_flags);
> + enum auto_buf_reg_res res = __ublk_do_auto_buf_reg(ubq, req, io, cmd,
> + issue_flags);
>
> - return true;
> + if (res != AUTO_BUF_REG_FAIL) {
> + ublk_prep_auto_buf_reg_io(ubq, req, io, cmd, res == AUTO_BUF_REG_OK);
> + io_uring_cmd_done(cmd, UBLK_IO_RES_OK, 0, issue_flags);
> + }
> }
>
> static bool ublk_start_io(const struct ublk_queue *ubq, struct request *req,
> @@ -1318,8 +1335,14 @@ static void ublk_dispatch_req(struct ublk_queue *ubq,
> if (!ublk_start_io(ubq, req, io))
> return;
>
> - if (ublk_prep_auto_buf_reg(ubq, req, io, io->cmd, issue_flags))
> + if (ublk_support_auto_buf_reg(ubq) && ublk_rq_has_data(req)) {
> + struct io_uring_cmd *cmd = io->cmd;
Don't really see the need for this intermediate variable
Best,
Caleb
> +
> + ublk_do_auto_buf_reg(ubq, req, io, cmd, issue_flags);
> + } else {
> + ublk_init_req_ref(ubq, io);
> ublk_complete_io_cmd(io, req, UBLK_IO_RES_OK, issue_flags);
> + }
> }
>
> static void ublk_cmd_tw_cb(struct io_uring_cmd *cmd,
> --
> 2.47.0
>
