On 6/24/25 7:22 PM, Ming Lei wrote:
> On Tue, Jun 24, 2025 at 08:26:51AM -0700, Caleb Sander Mateos wrote:
>> 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).
>
> Indeed, I will send a V2 for covering different task case.
>
> Jens, can you drop this patch?
Done
--
Jens Axboe
