Re: [PATCH 4/8] io_uring: add support for IORING_OP_URING_CMD

[Christoph Hellwig <hch@lst.de>]

On Wed, Mar 17, 2021 at 04:10:23PM -0600, Jens Axboe wrote:
> +/*
> + * Called by consumers of io_uring_cmd, if they originally returned
> + * -EIOCBQUEUED upon receiving the command.
> + */
> +void io_uring_cmd_done(struct io_uring_cmd *cmd, ssize_t ret)
> +{
> +	struct io_kiocb *req = container_of(cmd, struct io_kiocb, uring_cmd);
> +
> +	if (ret < 0)
> +		req_set_fail_links(req);
> +	io_req_complete(req, ret);
> +}
> +EXPORT_SYMBOL(io_uring_cmd_done);

This really should be EXPORT_SYMBOL_GPL. But more importantly I'm not
sure it is an all that useful interface.  All useful non-trivial ioctls
tend to access user memory, so something that queues up work in the task
context like in Joshis patch should really be part of the documented
interface.

> +
> +static int io_uring_cmd_prep(struct io_kiocb *req,
> +			     const struct io_uring_sqe *sqe)
> +{
> +	const struct io_uring_cmd_sqe *csqe = (const void *) sqe;

We really should not need this casting.  The struct io_uring_sqe
usage in io_uring.c needs to be replaced with a union or some other
properly type safe way to handle this.

> +	ret = file->f_op->uring_cmd(&req->uring_cmd, issue_flags);
> +	/* queued async, consumer will call io_uring_cmd_done() when complete */
> +	if (ret == -EIOCBQUEUED)
> +		return 0;
> +	io_uring_cmd_done(&req->uring_cmd, ret);
> +	return 0;

This can be simplified to:

	if (ret != -EIOCBQUEUED)
		io_uring_cmd_done(&req->uring_cmd, ret);
	return 0;


> +/*
> + * Note that the first member here must be a struct file, as the
> + * io_uring command layout depends on that.
> + */
> +struct io_uring_cmd {
> +	struct file	*file;
> +	__u16		op;
> +	__u16		unused;
> +	__u32		len;
> +	__u64		pdu[5];	/* 40 bytes available inline for free use */
> +};

I am a little worried about exposting this internal structure to random
drivers.  OTOH we need something that eventually allows a container_of
to io_kiocb for the completion, so I can't think of anything better
at the moment either.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme