Re: [PATCH] io_uring: complete request via task work in case of DEFER_TASKRUN

[Ming Lei <ming.lei@redhat.com>]

On Fri, Apr 14, 2023 at 04:07:52PM +0100, Pavel Begunkov wrote:
> 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.

Not sure it is true, cause it is allowed to call io_uring_cmd_done from other
task technically. And it could be more friendly for driver to not limit
its caller in the task context. Especially we have another API of
io_uring_cmd_complete_in_task().

> 
> I have several more not so related questions:
> 
> 1) Can requests be submitted by some other task than ->ubq_daemon?

Yeah, requests can be submitted by other task, but ublk driver doesn't
allow it because ublk driver has not knowledge when the io_uring context
goes away, so has to limit requests submitted from ->ubq_daemon only,
then use this task's information for checking if the io_uring context
is going to exit. When the io_uring context is dying, we need to
abort these uring commands(may never complete), see ublk_cancel_queue().

The only difference is that the uring command may never complete,
because one uring cmd is only completed when the associated block request
is coming. The situation could be improved by adding API/callback for
notifying io_uring exit.


> 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.

Yeah, it has been in my todo list to kill task work. In ublk early time,
task work just performs better than io_uring_cmd_complete_in_task(), but
the gap becomes pretty small or even not visible now. 

> 
> 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.

task work isn't available in case of building ublk as module.

> 
> 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?

The thing is that we don't know if there is io_uring request for the
coming blk request. UBLK_IO_FLAG_ABORTED just means that the io_uring
context is dead, and we can't use io_uring_cmd_complete_in_task() any
more.

> 
> > > 
> > > 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*)

IMO, it is reasonable for io_uring_cmd_done to hide any io_uring
implementation details, even the task context concept, but not
sure if it is doable.


Thanks,
Ming