public inbox for [email protected]
 help / color / mirror / Atom feed
From: Pavel Begunkov <[email protected]>
To: Jens Axboe <[email protected]>,
	[email protected], [email protected],
	[email protected]
Cc: Alexander Viro <[email protected]>
Subject: Re: [PATCH 3/3] io_uring: add splice(2) support
Date: Wed, 22 Jan 2020 06:16:40 +0300	[thread overview]
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>


[-- Attachment #1.1: Type: text/plain, Size: 3708 bytes --]

On 22/01/2020 05:47, Jens Axboe wrote:
> On 1/21/20 7:40 PM, Pavel Begunkov wrote:
>>>> @@ -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?
> 
> Yeah that's all I meant, sorry wasn't too clear. But you can disregard,
> I'll just pop a reservation in front if/when this is ready to go in if
> it's before EPOLL_CTL op.
> 
>>>> 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.
> 
> I guess that's true, I had vmsplice on my mind for this as well. But
> won't be a problem there, since it doesn't take 6 arguments like splice
> does.
> 
> Another option is to do an indirect for splice, stuff the arguments in a
> struct that's passed in as a pointer in ->addr. A bit slower, but
> probably not a huge deal.
> 
>>>> @@ -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.
> 
> You're probably right, though it's a bit nasty to add an unrelated flag
> in the splice flag space... We should probably reserve it in splice
> instead, and just not have it available from the regular system call.
> 
Agree, it looks bad. I don't want to add it into sqe->splice_flags to not clash
with splice(2) in the future, but could have a separate field in @sqe...
or can leave in in sqe->flags, as it's done in the patch, but that's like a
portion of bits would be opcode specific and we would need to set rules for
their use.

-- 
Pavel Begunkov


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

  reply	other threads:[~2020-01-22  3:17 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-22  0:05 [POC RFC 0/3] splice(2) support for io_uring Pavel Begunkov
2020-01-22  0:05 ` [PATCH 1/3] splice: make do_splice public Pavel Begunkov
2020-01-22  0:05 ` [PATCH 2/3] io_uring: add interface for getting files Pavel Begunkov
2020-01-22  1:54   ` Jens Axboe
2020-01-22  2:24     ` Pavel Begunkov
2020-01-22  0:05 ` [PATCH 3/3] io_uring: add splice(2) support Pavel Begunkov
2020-01-22  2:03   ` Jens Axboe
2020-01-22  2:40     ` Pavel Begunkov
2020-01-22  2:47       ` Jens Axboe
2020-01-22  3:16         ` Pavel Begunkov [this message]
2020-01-22  3:22           ` Jens Axboe
2020-01-24 12:31   ` kbuild test robot
2020-01-25 18:28   ` kbuild test robot
2020-01-22  1:55 ` [POC RFC 0/3] splice(2) support for io_uring Jens Axboe
2020-01-22  3:11   ` Pavel Begunkov
2020-01-22  3:30     ` Jens Axboe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox