Re: [PATCH V4 2/2] io_uring: cancelable uring_cmd

[Jens Axboe <axboe@kernel.dk>]

On 9/22/23 8:50 PM, Ming Lei wrote:
> diff --git a/include/linux/io_uring.h b/include/linux/io_uring.h
> index ae08d6f66e62..a0307289bdc7 100644
> --- a/include/linux/io_uring.h
> +++ b/include/linux/io_uring.h
> @@ -20,9 +20,13 @@ enum io_uring_cmd_flags {
>  	IO_URING_F_SQE128		= (1 << 8),
>  	IO_URING_F_CQE32		= (1 << 9),
>  	IO_URING_F_IOPOLL		= (1 << 10),
> +
> +	/* set when uring wants to cancel one issued command */
> +	IO_URING_F_CANCEL		= (1 << 11),
>  };

I'd make that comment:

/* set when uring wants to cancel a previously issued command */

> @@ -125,6 +132,15 @@ static inline int io_uring_cmd_sock(struct io_uring_cmd *cmd,
>  {
>  	return -EOPNOTSUPP;
>  }
> +static inline int io_uring_cmd_mark_cancelable(struct io_uring_cmd *cmd,
> +		unsigned int issue_flags)
> +{
> +	return -EOPNOTSUPP;
> +}

Do we need this to return an error? Presumably this will never get
called if IO_URING isn't defined, but if it does, it obviously doesn't
need to do anything anyway. Seems like it should just be a void, and
ditto for the enabled version which can't return an error anyway.

>  	return ret;
>  }
>  
> +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;
> +
> +	lockdep_assert_held(&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);
> +		struct file *file = req->file;
> +
> +		if (WARN_ON_ONCE(!file->f_op->uring_cmd))
> +			continue;

That check belongs in the function that marks it cancelable and adds it
to the list.

Outside of those minor nits, looks fine to me, and patch 1 does too.

-- 
Jens Axboe