On Mon, Jun 23, 2025 at 6:24 PM Ming Lei <ming.lei@xxxxxxxxxx> wrote:
>
> On Mon, Jun 23, 2025 at 10:51:00AM -0700, Caleb Sander Mateos wrote:
> > On Sun, Jun 22, 2025 at 6:19 PM Ming Lei <ming.lei@xxxxxxxxxx> wrote:
> > >
> > > ublk_queue_cmd_list() dispatches the whole batch list by scheduling task
> > > work via the tail request's io_uring_cmd, this way is fine even though
> > > more than one io_ring_ctx are involved for this batch since it is just
> > > one running context.
> > >
> > > However, the task work handler ublk_cmd_list_tw_cb() takes `issue_flags`
> > > of tail uring_cmd's io_ring_ctx for completing all commands. This way is
> > > wrong if any uring_cmd is issued from different io_ring_ctx.
> > >
> > > Fixes it by always building per-io-ring-ctx batch list.
> > >
> > > For typical per-queue or per-io daemon implementation, this way shouldn't
> > > make difference from performance viewpoint, because single io_ring_ctx is
> > > often taken in each daemon.
> > >
> > > Fixes: d796cea7b9f3 ("ublk: implement ->queue_rqs()")
> > > Fixes: ab03a61c6614 ("ublk: have a per-io daemon instead of a per-queue daemon")
> > > Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx>
> > > ---
> > > drivers/block/ublk_drv.c | 17 +++++++++--------
> > > 1 file changed, 9 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> > > index c637ea010d34..e79b04e61047 100644
> > > --- a/drivers/block/ublk_drv.c
> > > +++ b/drivers/block/ublk_drv.c
> > > @@ -1336,9 +1336,8 @@ static void ublk_cmd_list_tw_cb(struct io_uring_cmd *cmd,
> > > } while (rq);
> > > }
> > >
> > > -static void ublk_queue_cmd_list(struct ublk_io *io, struct rq_list *l)
> > > +static void ublk_queue_cmd_list(struct io_uring_cmd *cmd, struct rq_list *l)
> > > {
> > > - struct io_uring_cmd *cmd = io->cmd;
> > > struct ublk_uring_cmd_pdu *pdu = ublk_get_uring_cmd_pdu(cmd);
> > >
> > > pdu->req_list = rq_list_peek(l);
> > > @@ -1420,16 +1419,18 @@ static void ublk_queue_rqs(struct rq_list *rqlist)
> > > {
> > > struct rq_list requeue_list = { };
> > > struct rq_list submit_list = { };
> > > - struct ublk_io *io = NULL;
> > > + struct io_uring_cmd *cmd = NULL;
> > > struct request *req;
> > >
> > > while ((req = rq_list_pop(rqlist))) {
> > > struct ublk_queue *this_q = req->mq_hctx->driver_data;
> > > - struct ublk_io *this_io = &this_q->ios[req->tag];
> > > + struct io_uring_cmd *this_cmd = this_q->ios[req->tag].cmd;
> > >
> > > - if (io && io->task != this_io->task && !rq_list_empty(&submit_list))
> > > - ublk_queue_cmd_list(io, &submit_list);
> > > - io = this_io;
> > > + if (cmd && io_uring_cmd_ctx_handle(cmd) !=
> > > + io_uring_cmd_ctx_handle(this_cmd) &&
> > > + !rq_list_empty(&submit_list))
> > > + ublk_queue_cmd_list(cmd, &submit_list);
> >
> > I don't think we can assume that ublk commands submitted to the same
> > io_uring have the same daemon task. It's possible for multiple tasks
> > to submit to the same io_uring, even though that's not a common or
> > performant way to use io_uring. Probably we need to check that both
> > the task and io_ring_ctx match.
>
> Here the problem is in 'issue_flags' passed from io_uring, especially for
> grabbing io_ring_ctx lock.
>
> If two uring_cmd are issued via same io_ring_ctx from two tasks, it is
> fine to share 'issue_flags' from one of tasks, what matters is that the
> io_ring_ctx lock is handled correctly when calling io_uring_cmd_done().
Right, I understand the issue you are trying to solve. I agree it's a
problem for submit_list to contain commands from multiple
io_ring_ctxs. But it's also a problem if it contains commands with
different daemon tasks, because ublk_queue_cmd_list() will schedule
ublk_cmd_list_tw_cb() to be called in the *last command's task*. But
ublk_cmd_list_tw_cb() will call ublk_dispatch_req() for all the
commands in the list. So if submit_list contains commands with
multiple daemon tasks, ublk_dispatch_req() will fail on the current !=
io->task check. So I still feel we need to call
ublk_queue_cmd_list(io, &submit_list) if io->task != this_io->task (as
well as if the io_ring_ctxs differ).
Best,
Caleb
