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
