On Fri, Sep 22, 2023 at 08:50:48AM +0800, Ming Lei wrote: > Hello Gabriel, > > On Thu, Sep 21, 2023 at 02:46:31PM -0400, Gabriel Krisman Bertazi wrote: > > Ming Lei <ming.lei@xxxxxxxxxx> writes: > > > > > uring_cmd may never complete, such as ublk, in which uring cmd isn't > > > completed until one new block request is coming from ublk block device. > > > > > > Add cancelable uring_cmd to provide mechanism to driver to cancel > > > pending commands in its own way. > > > > > > Add API of io_uring_cmd_mark_cancelable() for driver to mark one > > > command as cancelable, then io_uring will cancel this command in > > > io_uring_cancel_generic(). Driver callback is provided for canceling > > > command in driver's way, meantime driver gets notified with exiting of > > > io_uring task or context. > > > > > > Suggested-by: Jens Axboe <axboe@xxxxxxxxx> > > > Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx> > > > --- > > > > > > ublk patches: > > > > > > https://github.com/ming1/linux/commits/uring_exit_and_ublk > > > > > > include/linux/io_uring.h | 22 +++++++++++++++++- > > > include/linux/io_uring_types.h | 6 +++++ > > > include/uapi/linux/io_uring.h | 7 ++++-- > > > io_uring/io_uring.c | 30 ++++++++++++++++++++++++ > > > io_uring/uring_cmd.c | 42 ++++++++++++++++++++++++++++++++++ > > > 5 files changed, 104 insertions(+), 3 deletions(-) > > > > > > diff --git a/include/linux/io_uring.h b/include/linux/io_uring.h > > > index 106cdc55ff3b..5b98308a154f 100644 > > > --- a/include/linux/io_uring.h > > > +++ b/include/linux/io_uring.h > > > @@ -22,6 +22,9 @@ enum io_uring_cmd_flags { > > > IO_URING_F_IOPOLL = (1 << 10), > > > }; > > > > > > +typedef void (uring_cmd_cancel_fn)(struct io_uring_cmd *, > > > + unsigned int issue_flags, struct task_struct *task); > > > + > > > > Hi Ming, > > > > I wonder if uring_cmd_cancel shouldn't just be a new file operation, pairing > > with f_op->uring_cmd. it would, of course, also mean don't need to pass > > it here occupying the pdu or explicitly registering it. iiuc, would also > > allow you to drop the flag and just assume it is cancelable if the operation > > exists, further simplifying the code. > > If there are more such use cases, it is probably not a bad idea to add > new operation for canceling command. > > But definitely there are not, so not good to add one new operation now, > since new operation field is added to all drivers/FSs actually, 99% of > them shouldn't pay for such cost. > > Also I don't see how much simplification is made with new operation, > cause the approach in this patch just needs one flag & callback for canceling, > both are freely available, only driver with such feature needs to pay > the extra callback cost(8bytes in uring_cmd->pdu[]). Another way is to reserve some cmd op range for io_uring use, then we can define CANCEL_CMD and pass it to ->uring_cmd(), such as reserving the following range: _IOWR(0xFF, 0, 0xFF) ~ _IOWR(0xFF, 0x7F, 0xFF) //user visible _IOWR(0xFF, 0x80, 0xFF) ~ _IOWR(0xFF, 0xFF, 0xFF) //io_uring internal even SOCKET_URING_OP_SIOCINQ/SOCKET_URING_OP_SIOCOUTQ can be covered since they are just merged to v6.6-rc1, then we have fixed cmd op range for io_uring for future use cases. Jens & Gabriel, which way do you think better? > > > > > > +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; > > > + > > > + mutex_lock(&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); > > > + > > > + if (!cancel_all && req->task != task) > > > + continue; > > > + > > > + /* safe to call ->cancel_fn() since cmd isn't done yet */ > > > + if (cmd->flags & IORING_URING_CMD_CANCELABLE) { > > > + cmd->cancel_fn(cmd, 0, task); > > > > I find it weird to pass task here. Also, it seems you use it only to > > sanity check it is the same as ubq->ubq_daemon. Can you just recover it > > from cmd_to_io_kiocb(cmd)->task? it should be guaranteed to be the same > > as task by the check immediately before. > > 'task' parameter is very important for ublk use case, in future I plan to > support multiple tasks per queue(io_uring_ctx) for ublk queue for > relaxing the current (more stict) limit of single task/context for > single queue. So when one task is exiting, ublk driver will just need to > cancel commands queued from this task, not necessarily to cancel the > whole queue/device. > > Also cmd_to_io_kiocb(cmd)->task shouldn't work since io_kiocb can be > thought as being not exported to driver strictly speaking. new API of io_uring_cmd_get_task() can be added which should be just used for handling CANCEL_CMD if we take ->uring_cmd(). Thanks, Ming
