Re: [PATCH 3/3] io_uring: add splice(2) support

Jens Axboe
2020-01-22T02:03:37Z

On 1/21/20 5:05 PM, Pavel Begunkov wrote:
> @@ -373,6 +374,15 @@ struct io_rw {
> u64 len;
> };
>
> +struct io_splice {
> + struct file *file_in;
> + struct file *file_out;
> + loff_t __user *off_in;
> + loff_t __user *off_out;
> + u64 len;
> + unsigned int flags;
> +};
> +
> struct io_connect {
> struct file *file;
> struct sockaddr __user *addr;

Probably just make that len u32 as per previous email.

> @@ -719,6 +730,11 @@ static const struct io_op_def io_op_defs[] = {
> .needs_file = 1,
> .fd_non_neg = 1,
> },
> + [IORING_OP_SPLICE] = {
> + .needs_file = 1,
> + .hash_reg_file = 1,
> + .unbound_nonreg_file = 1,
> + }
> };
>
> static void io_wq_submit_work(struct io_wq_work **workptr);

I probably want to queue up a reservation for the EPOLL_CTL that I
haven't included yet, but which has been tested. But that's easily
manageable, so no biggy on my end.

> +static bool io_splice_punt(struct file *file)
> +{
> + if (get_pipe_info(file))
> + return false;
> + if (!io_file_supports_async(file))
> + return true;
> + return !(file->f_mode & O_NONBLOCK);
> +}
> +
> +static int io_splice(struct io_kiocb *req, struct io_kiocb **nxt,
> + bool force_nonblock)
> +{
> + struct io_splice* sp = &req->splice;
> + struct file *in = sp->file_in;
> + struct file *out = sp->file_out;
> + unsigned int flags = sp->flags;
> + long ret;
> +
> + if (force_nonblock) {
> + if (io_splice_punt(in) || io_splice_punt(out)) {
> + req->flags |= REQ_F_MUST_PUNT;
> + return -EAGAIN;
> + }
> + flags |= SPLICE_F_NONBLOCK;
> + }
> +
> + ret = do_splice(in, sp->off_in, out, sp->off_out, sp->len, flags);
> + if (force_nonblock && ret == -EAGAIN)
> + return -EAGAIN;
> +
> + io_put_file(req->ctx, out, (flags & IOSQE_SPLICE_FIXED_OUT));
> + io_cqring_add_event(req, ret);
> + if (ret != sp->len)
> + req_set_fail_links(req);
> + io_put_req_find_next(req, nxt);
> + return 0;
> +}

This looks good. And this is why the put_file() needs to take separate
arguments...

> diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
> index 57d05cc5e271..f234b13e7ed3 100644
> --- a/include/uapi/linux/io_uring.h
> +++ b/include/uapi/linux/io_uring.h
> @@ -23,8 +23,14 @@ struct io_uring_sqe {
> __u64 off; /* offset into file */
> __u64 addr2;
> };
> - __u64 addr; /* pointer to buffer or iovecs */
> - __u32 len; /* buffer size or number of iovecs */
> + union {
> + __u64 addr; /* pointer to buffer or iovecs */
> + __u64 off_out;
> + };
> + union {
> + __u32 len; /* buffer size or number of iovecs */
> + __s32 fd_out;
> + };
> union {
> __kernel_rwf_t rw_flags;
> __u32 fsync_flags;
> @@ -37,10 +43,12 @@ struct io_uring_sqe {
> __u32 open_flags;
> __u32 statx_flags;
> __u32 fadvise_advice;
> + __u32 splice_flags;
> };
> __u64 user_data; /* data to be passed back at completion time */
> union {
> __u16 buf_index; /* index into fixed buffers, if used */
> + __u64 splice_len;
> __u64 __pad2[3];
> };
> };

Not a huge fan of this, also mean splice can't ever used fixed buffers.
Hmm...

> @@ -67,6 +75,9 @@ enum {
> /* always go async */
> #define IOSQE_ASYNC (1U << IOSQE_ASYNC_BIT)
>
> +/* op custom flags */
> +#define IOSQE_SPLICE_FIXED_OUT (1U << 16)
> +

I don't think it's unreasonable to say that if you specify
IOSQE_FIXED_FILE, then both are fixed. If not, then none of them are.
What do you think?

--
Jens Axboe