On Tue, Jun 24, 2025 at 8:16 AM Caleb Sander Mateos
<csander@xxxxxxxxxxxxxxx> wrote:
>
> 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").
Ah never mind, I see my commit removed the call to ublk_fill_io_cmd().
I see that's necessary for the -EIOCBQUEUED return to work correctly.
>
> > 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
> >
