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

[Ming Lei <ming.lei@redhat.com>]

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