On Tue, Jun 24, 2025 at 3:41 AM Ming Lei <ming.lei@xxxxxxxxxx> wrote:
>
> If ublk_get_data() fails, -EIOCBQUEUED is returned and the current command
> becomes ASYNC. And the only reason is that mapping data can't move on,
> because of no enough pages or pending signal, then the current ublk request
> has to be requeued.
>
> Once the request need to be requeued, we have to setup `ublk_io` correctly,
> including io->cmd and flags, otherwise the request may not be forwarded to
> ublk server successfully.
>
> Fixes: 9810362a57cb ("ublk: don't call ublk_dispatch_req() for NEED_GET_DATA")
Don't think this is the right Fixes tag; the issue predates this
refactoring. I pointed out this existing issue when we discussed that
patch: https://lore.kernel.org/linux-block/CADUfDZroQ4zHanPjytcEUhn4tQc3BYMPZD2uLOik7jAXvOCjGg@xxxxxxxxxxxxxx/
Looks like the correct commit would be c86019ff75c1 ("ublk_drv: add
support for UBLK_IO_NEED_GET_DATA").
> Reported-by: Changhui Zhong <czhong@xxxxxxxxxx>
> Closes: https://lore.kernel.org/linux-block/CAGVVp+VN9QcpHUz_0nasFf5q9i1gi8H8j-G-6mkBoqa3TyjRHA@xxxxxxxxxxxxxx/
> Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx>
> ---
> V2:
> - move io->addr assignment into ublk_fill_io_cmd()
>
> drivers/block/ublk_drv.c | 35 +++++++++++++++++++++++++----------
> 1 file changed, 25 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index d36f44f5ee80..3566d7c36b8d 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -1148,8 +1148,8 @@ static inline void __ublk_complete_rq(struct request *req)
> blk_mq_end_request(req, res);
> }
>
> -static void ublk_complete_io_cmd(struct ublk_io *io, struct request *req,
> - int res, unsigned issue_flags)
> +static struct io_uring_cmd *__ublk_prep_compl_io_cmd(struct ublk_io *io,
> + struct request *req)
nit: don't think the underscores are necessary
> {
> /* read cmd first because req will overwrite it */
> struct io_uring_cmd *cmd = io->cmd;
> @@ -1164,6 +1164,13 @@ static void ublk_complete_io_cmd(struct ublk_io *io, struct request *req,
> io->flags &= ~UBLK_IO_FLAG_ACTIVE;
>
> io->req = req;
> + return cmd;
> +}
> +
> +static void ublk_complete_io_cmd(struct ublk_io *io, struct request *req,
> + int res, unsigned issue_flags)
> +{
> + struct io_uring_cmd *cmd = __ublk_prep_compl_io_cmd(io, req);
>
> /* tell ublksrv one io request is coming */
> io_uring_cmd_done(cmd, res, 0, issue_flags);
> @@ -2148,10 +2155,9 @@ static int ublk_commit_and_fetch(const struct ublk_queue *ubq,
> return 0;
> }
>
> -static bool ublk_get_data(const struct ublk_queue *ubq, struct ublk_io *io)
> +static bool ublk_get_data(const struct ublk_queue *ubq, struct ublk_io *io,
> + struct request *req)
> {
> - struct request *req = io->req;
> -
> /*
> * We have handled UBLK_IO_NEED_GET_DATA command,
> * so clear UBLK_IO_FLAG_NEED_GET_DATA now and just
> @@ -2178,6 +2184,7 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
> u32 cmd_op = cmd->cmd_op;
> unsigned tag = ub_cmd->tag;
> int ret = -EINVAL;
> + struct request *req;
>
> pr_devel("%s: received: cmd op %d queue %d tag %d result %d\n",
> __func__, cmd->cmd_op, ub_cmd->q_id, tag,
> @@ -2236,11 +2243,19 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
> goto out;
> break;
> case UBLK_IO_NEED_GET_DATA:
> - io->addr = ub_cmd->addr;
> - if (!ublk_get_data(ubq, io))
> - return -EIOCBQUEUED;
> -
> - return UBLK_IO_RES_OK;
> + /*
> + * ublk_get_data() may fail and fallback to requeue, so keep
> + * uring_cmd active first and prepare for handling new requeued
> + * request
> + */
> + req = io->req;
> + ublk_fill_io_cmd(io, cmd, ub_cmd->addr);
> + io->flags &= ~UBLK_IO_FLAG_OWNED_BY_SRV;
> + if (likely(ublk_get_data(ubq, io, req))) {
> + __ublk_prep_compl_io_cmd(io, req);
Isn't __ublk_prep_compl_io_cmd() basically undoing the
ublk_fill_io_cmd() and io->flags &= ~UBLK_IO_FLAG_OWNED_BY_SRV? How
about something like this instead?
io->addr = ub_cmd->addr;
if (likely(ublk_get_data(ubq, io)))
return UBLK_IO_RES_OK;
ublk_fill_io_cmd(io, cmd, ub_cmd->addr);
io->flags &= ~UBLK_IO_FLAG_OWNED_BY_SRV;
break;
Best,
Caleb
> + return UBLK_IO_RES_OK;
> + }
> + break;
> default:
> goto out;
> }
> --
> 2.49.0
>
