Re: [PATCH] io_uring: cancelable uring_cmd

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

Hello Gabriel,

On Thu, Sep 21, 2023 at 02:46:31PM -0400, Gabriel Krisman Bertazi wrote:
> Ming Lei <ming.lei@redhat.com> writes:
> 
> > uring_cmd may never complete, such as ublk, in which uring cmd isn't
> > completed until one new block request is coming from ublk block device.
> >
> > Add cancelable uring_cmd to provide mechanism to driver to cancel
> > pending commands in its own way.
> >
> > Add API of io_uring_cmd_mark_cancelable() for driver to mark one
> > command as cancelable, then io_uring will cancel this command in
> > io_uring_cancel_generic(). Driver callback is provided for canceling
> > command in driver's way, meantime driver gets notified with exiting of
> > io_uring task or context.
> >
> > Suggested-by: Jens Axboe <axboe@kernel.dk>
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >
> > ublk patches:
> >
> > 	https://github.com/ming1/linux/commits/uring_exit_and_ublk
> >
> >  include/linux/io_uring.h       | 22 +++++++++++++++++-
> >  include/linux/io_uring_types.h |  6 +++++
> >  include/uapi/linux/io_uring.h  |  7 ++++--
> >  io_uring/io_uring.c            | 30 ++++++++++++++++++++++++
> >  io_uring/uring_cmd.c           | 42 ++++++++++++++++++++++++++++++++++
> >  5 files changed, 104 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/linux/io_uring.h b/include/linux/io_uring.h
> > index 106cdc55ff3b..5b98308a154f 100644
> > --- a/include/linux/io_uring.h
> > +++ b/include/linux/io_uring.h
> > @@ -22,6 +22,9 @@ enum io_uring_cmd_flags {
> >  	IO_URING_F_IOPOLL		= (1 << 10),
> >  };
> >  
> > +typedef void (uring_cmd_cancel_fn)(struct io_uring_cmd *,
> > +		unsigned int issue_flags, struct task_struct *task);
> > +
> 
> Hi Ming,
> 
> I wonder if uring_cmd_cancel shouldn't just be a new file operation, pairing
> with f_op->uring_cmd.  it would, of course, also mean don't need to pass
> it here occupying the pdu or explicitly registering it. iiuc, would also
> allow you to drop the flag and just assume it is cancelable if the operation
> exists, further simplifying the code.

If there are more such use cases, it is probably not a bad idea to add
new operation for canceling command.

But definitely there are not, so not good to add one new operation now,
since new operation field is added to all drivers/FSs actually, 99% of
them shouldn't pay for such cost.

Also I don't see how much simplification is made with new operation,
cause the approach in this patch just needs one flag & callback for canceling,
both are freely available, only driver with such feature needs to pay
the extra callback cost(8bytes in uring_cmd->pdu[]).

> 
> > +static bool io_uring_try_cancel_uring_cmd(struct io_ring_ctx *ctx,
> > +					  struct task_struct *task,
> > +					  bool cancel_all)
> > +{
> > +	struct hlist_node *tmp;
> > +	struct io_kiocb *req;
> > +	bool ret = false;
> > +
> > +	mutex_lock(&ctx->uring_lock);
> > +	hlist_for_each_entry_safe(req, tmp, &ctx->cancelable_uring_cmd,
> > +			hash_node) {
> > +		struct io_uring_cmd *cmd = io_kiocb_to_cmd(req,
> > +				struct io_uring_cmd);
> > +
> > +		if (!cancel_all && req->task != task)
> > +			continue;
> > +
> > +		/* safe to call ->cancel_fn() since cmd isn't done yet */
> > +		if (cmd->flags & IORING_URING_CMD_CANCELABLE) {
> > +			cmd->cancel_fn(cmd, 0, task);
> 
> I find it weird to pass task here.  Also, it seems you use it only to
> sanity check it is the same as ubq->ubq_daemon.  Can you just recover it
> from cmd_to_io_kiocb(cmd)->task? it should be guaranteed to be the same
> as task by the check immediately before.

'task' parameter is very important for ublk use case, in future I plan to
support multiple tasks per queue(io_uring_ctx) for ublk queue for
relaxing the current (more stict) limit of single task/context for
single queue. So when one task is exiting, ublk driver will just need to
cancel commands queued from this task, not necessarily to cancel the
whole queue/device.

Also cmd_to_io_kiocb(cmd)->task shouldn't work since io_kiocb can be
thought as being not exported to driver strictly speaking.


Thanks,
Ming