On 22/01/2020 05:03, Jens Axboe wrote:
> 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.
Right, I don't want to have multiple types and names for it myself.
>
>> @@ -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.
I didn't quite get it. Do you mean collision of opcode numbers?
>
>> +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...
But it's not like splice() ever uses user buffers. Isn't it? vmsplice does, but
that's another opcode.
>
>> @@ -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?
>
It's plausible to register only one end for splicing, e.g. splice from
short-lived sockets to pre-registered buffers-pipes. And it's clearer do it now.
--
Pavel Begunkov
Attachment:
signature.asc
Description: OpenPGP digital signature
