On 4/14/23 14:53, Ming Lei wrote:
On Fri, Apr 14, 2023 at 02:01:26PM +0100, Pavel Begunkov wrote:
On 4/14/23 08:53, Ming Lei wrote:
So far io_req_complete_post() only covers DEFER_TASKRUN by completing
request via task work when the request is completed from IOWQ.
However, uring command could be completed from any context, and if io
uring is setup with DEFER_TASKRUN, the command is required to be
completed from current context, otherwise wait on IORING_ENTER_GETEVENTS
can't be wakeup, and may hang forever.
fwiw, there is one legit exception, when the task is half dead
task_work will be executed by a kthread. It should be fine as it
locks the ctx down, but I can't help but wonder whether it's only
ublk_cancel_queue() affected or there are more places in ublk?
No, it isn't.
It isn't triggered on nvme-pt just because command is always done
in task context.
And we know more uring command cases are coming.
Because all requests and cmds but ublk complete it from another
task, ublk is special in this regard.
I have several more not so related questions:
1) Can requests be submitted by some other task than ->ubq_daemon?
Looking at
static int ublk_ch_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags)
{
...
if (ubq->ubq_daemon && ubq->ubq_daemon != current)
goto out;
}
ublk_queue_cmd() avoiding io_uring way of delivery and using
raw task_work doesn't seem great. Especially with TWA_SIGNAL_NO_IPI.
2) What the purpose of the two lines below? I see how
UBLK_F_URING_CMD_COMP_IN_TASK is used, but don't understand
why it changes depending on whether it's a module or not.
3) The long comment in ublk_queue_cmd() seems quite scary.
If you have a cmd / io_uring request it hold a ctx reference
and is always allowed to use io_uring's task_work infra like
io_uring_cmd_complete_in_task(). Why it's different for ublk?
One more thing, cmds should not be setting issue_flags but only
forwarding what the core io_uring code passed, it'll get tons of
bugs in no time otherwise.
Here io_uring_cmd_done() is changed to this way recently, and it
could be another topic.
And it's abused, but as you said, not particularly related
to this patch.
static void ublk_cancel_queue(struct ublk_queue *ubq)
{
...
io_uring_cmd_done(io->cmd, UBLK_IO_RES_ABORT, 0,
IO_URING_F_UNLOCKED);
}
Can we replace it with task_work? It should be cold, and I
assume ublk_cancel_queue() doesn't assume that all requests will
put down by the end of the function as io_uring_cmd_done()
can offload it in any case.
But it isn't specific for ublk, any caller of io_uring_cmd_done()
has such issue since io_uring_cmd_done() is one generic API.
Well, fair enough, considering that IO_URING_F_UNLOCKED was
just added (*still naively hoping it'll be clean up*)
--
Pavel Begunkov
