On Thu, Sep 28, 2023 at 02:38:32AM -0600, Jens Axboe wrote:
> 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 */
OK.
>
> > @@ -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.
>
> > 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
