Re: [PATCH 3/3] io_uring/rw: add support for IORING_OP_READ_MULTISHOT

[Gabriel Krisman Bertazi <krisman@suse.de>]

Jens Axboe <axboe@kernel.dk> writes:

> This behaves like IORING_OP_READ, except:
>
> 1) It only supports pollable files (eg pipes, sockets, etc). Note that
>    for sockets, you probably want to use recv/recvmsg with multishot
>    instead.
>
> 2) It supports multishot mode, meaning it will repeatedly trigger a
>    read and fill a buffer when data is available. This allows similar
>    use to recv/recvmsg but on non-sockets, where a single request will
>    repeatedly post a CQE whenever data is read from it.
>
> 3) Because of #2, it must be used with provided buffers. This is
>    uniformly true across any request type that supports multishot and
>    transfers data, with the reason being that it's obviously not
>    possible to pass in a single buffer for the data, as multiple reads
>    may very well trigger before an application has a chance to process
>    previous CQEs and the data passed from them.
>
> Signed-off-by: Jens Axboe <axboe@kernel.dk>

This is a really cool feature.  Just two comments inline.


> +/*
> + * Multishot read is prepared just like a normal read/write request, only
> + * difference is that we set the MULTISHOT flag.
> + */
> +int io_read_mshot_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
> +{
> +	int ret;
> +
> +	ret = io_prep_rw(req, sqe);
> +	if (unlikely(ret))
> +		return ret;
> +
> +	req->flags |= REQ_F_APOLL_MULTISHOT;
> +	return 0;
> +}
> +
>  void io_readv_writev_cleanup(struct io_kiocb *req)
>  {
>  	struct io_async_rw *io = req->async_data;
> @@ -869,6 +885,56 @@ int io_read(struct io_kiocb *req, unsigned int issue_flags)
>  	return kiocb_done(req, ret, issue_flags);
>  }
>  
> +int io_read_mshot(struct io_kiocb *req, unsigned int issue_flags)
> +{
> +	unsigned int cflags = 0;
> +	int ret;
> +
> +	/*
> +	 * Multishot MUST be used on a pollable file
> +	 */
> +	if (!file_can_poll(req->file))
> +		return -EBADFD;

io_uring is pollable, so I think you want to also reject when
req->file->f_ops == io_uring_fops to avoid the loop where a ring
monitoring itself will cause a recursive completion? Maybe this can't
happen here for some reason I miss?

> +
> +	ret = __io_read(req, issue_flags);
> +
> +	/*
> +	 * If we get -EAGAIN, recycle our buffer and just let normal poll
> +	 * handling arm it.
> +	 */
> +	if (ret == -EAGAIN) {
> +		io_kbuf_recycle(req, issue_flags);
> +		return -EAGAIN;
> +	}
> +
> +	/*
> +	 * Any error will terminate a multishot request
> +	 */
> +	if (ret <= 0) {
> +finish:
> +		io_req_set_res(req, ret, cflags);
> +		if (issue_flags & IO_URING_F_MULTISHOT)
> +			return IOU_STOP_MULTISHOT;
> +		return IOU_OK;

Just a style detail, but I'd prefer to unfold this on the end of the function
instead of jumping backwards here..

> +	}
> +
> +	/*
> +	 * Put our buffer and post a CQE. If we fail to post a CQE, then
> +	 * jump to the termination path. This request is then done.
> +	 */
> +	cflags = io_put_kbuf(req, issue_flags);
> +
> +	if (io_fill_cqe_req_aux(req, issue_flags & IO_URING_F_COMPLETE_DEFER,
> +				ret, cflags | IORING_CQE_F_MORE)) {
> +		if (issue_flags & IO_URING_F_MULTISHOT)
> +			return IOU_ISSUE_SKIP_COMPLETE;
> +		else
> +			return -EAGAIN;
> +	}
> +
> +	goto finish;
> +}
> +
>  int io_write(struct io_kiocb *req, unsigned int issue_flags)
>  {
>  	struct io_rw *rw = io_kiocb_to_cmd(req, struct io_rw);
> diff --git a/io_uring/rw.h b/io_uring/rw.h
> index 4b89f9659366..c5aed03d42a4 100644
> --- a/io_uring/rw.h
> +++ b/io_uring/rw.h
> @@ -23,3 +23,5 @@ int io_writev_prep_async(struct io_kiocb *req);
>  void io_readv_writev_cleanup(struct io_kiocb *req);
>  void io_rw_fail(struct io_kiocb *req);
>  void io_req_rw_complete(struct io_kiocb *req, struct io_tw_state *ts);
> +int io_read_mshot_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe);
> +int io_read_mshot(struct io_kiocb *req, unsigned int issue_flags);

-- 
Gabriel Krisman Bertazi