On Thu, Sep 28, 2023 at 02:38:32AM -0600, Jens Axboe wrote:

On 9/22/23 8:50 PM, Ming Lei wrote:

quoted

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 */

OK.

quoted

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

Indeed, 'void' is better.

quoted

 	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.

io_uring_cmd_mark_cancelable() is actually called from ->uring_cmd(), so
the check isn't necessary.



thanks,
Ming