Re: [PATCH] io_uring: cancelable uring_cmd

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

On Fri, Sep 22, 2023 at 08:50:48AM +0800, Ming Lei wrote:
> 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[]).

Another way is to reserve some cmd op range for io_uring use, then
we can define CANCEL_CMD and pass it to ->uring_cmd(), such as reserving
the following range:

	_IOWR(0xFF, 0, 0xFF) ~ _IOWR(0xFF, 0x7F, 0xFF)	//user visible
	_IOWR(0xFF, 0x80, 0xFF) ~ _IOWR(0xFF, 0xFF, 0xFF)	//io_uring internal

even SOCKET_URING_OP_SIOCINQ/SOCKET_URING_OP_SIOCOUTQ can be covered
since they are just merged to v6.6-rc1, then we have fixed cmd op range
for io_uring for future use cases.

Jens & Gabriel, which way do you think better?

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

new API of io_uring_cmd_get_task() can be added which should be just
used for handling CANCEL_CMD if we take ->uring_cmd().

Thanks,
Ming