Re: [PATCH] io_uring: cancelable uring_cmd

[Gabriel Krisman Bertazi <krisman@suse.de>]

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.

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

Thanks,

-- 
Gabriel Krisman Bertazi