splice(2) support for io_uring

Preparation without functional changes. Adds io_get_file(), that allows to grab files not only into req->file.

Signed-off-by Pavel Begunkov <[email protected]>

---
 fs/io_uring.c | 66 ++++++++++++++++++++++++++++++++-------------------
 1 file changed, 41 insertions(+), 25 deletions(-)

fs/io_uring.c +0 -0

index 8f7846cb1ebf..e9e4aee0fb99 100644
@@ -1161,6 +1161,15 @@ static struct io_kiocb *io_get_req(struct io_ring_ctx *ctx,
 	return NULL;

It works well for basic cases, but there is still work to be done. E.g. it misses @hash_reg_file checks for the second (output) file. Anyway, there are some questions I want to discuss:

- why sqe->len is __u32? Splice uses size_t, and I think it's better to have something wider (e.g. u64) for fututre use. That's the story behind added sqe->splice_len.

- it requires 2 fds, and it's painful. Currently file managing is done by common path (e.g. io_req_set_file(), __io_req_aux_free()). I'm thinking to make each opcode function handle file grabbing/putting themself with some helpers, as it's done in the patch for splice's out-file.
1. Opcode handler knows, whether it have/needs a file, and thus doesn't need extra checks done in common path.
2. It will be more consistent with splice.
Objections? Ideas?

- do we need offset pointers with fallback to file->f_pos? Or is it enough to have offset value. Jens, I remember you added the first option somewhere, could you tell the reasoning?

fs/io_uring.c +2 -0

@@ -373,6 +374,15 @@ struct io_rw {
 	u64				len;
 };
 
+struct io_splice {
+	struct file			*file_in;

fs/io_uring.c +27 -0

+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);

include/uapi/linux/io_uring.h +8 -2

index 57d05cc5e271..f234b13e7ed3 100644
@@ -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;

Quoted 16 lines · Pavel Begunkov

Preparation without functional changes. Adds io_get_file(), that allows to grab files not only into req->file.

Signed-off-by Pavel Begunkov <[email protected]>

---
 fs/io_uring.c | 66 ++++++++++++++++++++++++++++++++-------------------
 1 file changed, 41 insertions(+), 25 deletions(-)

fs/io_uring.c +0 -0

index 8f7846cb1ebf..e9e4aee0fb99 100644
@@ -1161,6 +1161,15 @@ static struct io_kiocb *io_get_req(struct io_ring_ctx *ctx,
 	return NULL;
 }
 

fs/io_uring.c static struct io_kiocb *io_get_req(struct io_ring_ctx *ctx, · line 1171

Quoted 8 lines

fs/io_uring.c +8 -0

+static inline void io_put_file(struct io_ring_ctx *ctx, struct file *file,
+			  bool fixed)
+{
+	if (fixed)
+		percpu_ref_put(&ctx->file_data->refs);
+	else
+		fput(file);
+}

Just make this take struct io_kiocb?

Apart from that, looks fine to me.

Quoted 4 lines · Pavel Begunkov

fs/io_uring.c +0 -0

@@ -373,6 +374,15 @@ struct io_rw {
 	u64				len;
 };
 

fs/io_uring.c struct io_rw { · line 388

Quoted 12 lines

fs/io_uring.c +9 -0

+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.

Quoted 12 lines

fs/io_uring.c +5 -0

@@ -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,
+	}
 };
 

fs/io_uring.c static const struct io_op_def io_op_defs[] = { · line 740

Quoted 1 line

fs/io_uring.c +0 -0

 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.

Quoted 28 lines

fs/io_uring.c +27 -0

+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);

fs/io_uring.c static int io_write(struct io_kiocb *req, struct io_kiocb **nxt, · line 2398

Quoted 10 lines

fs/io_uring.c +10 -0

+	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...

Quoted 29 lines

include/uapi/linux/io_uring.h +9 -2

index 57d05cc5e271..f234b13e7ed3 100644
@@ -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 {

include/uapi/linux/io_uring.h struct io_uring_sqe { · line 54

Quoted 5 lines

include/uapi/linux/io_uring.h +1 -0

 		__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...

Quoted 5 lines

include/uapi/linux/io_uring.h +0 -0

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

include/uapi/linux/io_uring.h enum { · line 80

Quoted 3 lines

include/uapi/linux/io_uring.h +3 -0

+/* 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?

Quoted 11 lines

Quoted 11 lines

fs/io_uring.c +5 -0

@@ -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,
+	}
 };
 

Quoted 5 lines

fs/io_uring.c static const struct io_op_def io_op_defs[] = { · line 740

Quoted 1 line

fs/io_uring.c +0 -0

 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?

Quoted 29 lines

Quoted 29 lines

include/uapi/linux/io_uring.h +9 -2

index 57d05cc5e271..f234b13e7ed3 100644
@@ -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 {

Quoted 8 lines

include/uapi/linux/io_uring.h struct io_uring_sqe { · line 54

Quoted 5 lines

include/uapi/linux/io_uring.h +1 -0

 		__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.

Quoted 4 lines

Quoted 4 lines

include/uapi/linux/io_uring.h +0 -0

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

Quoted 8 lines

include/uapi/linux/io_uring.h enum { · line 80

Quoted 3 lines

include/uapi/linux/io_uring.h +3 -0

+/* 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.

Quoted 7 lines · Pavel Begunkov

It works well for basic cases, but there is still work to be done. E.g. it misses @hash_reg_file checks for the second (output) file. Anyway, there are some questions I want to discuss:

- why sqe->len is __u32? Splice uses size_t, and I think it's better to have something wider (e.g. u64) for fututre use. That's the story behind added sqe->splice_len.

IO operations in Linux generally are INT_MAX, so the u32 is plenty big. That's why I chose it. For this specifically, if you look at splice:

	if (unlikely(len > MAX_RW_COUNT))
		len = MAX_RW_COUNT;

so anything larger is truncated anyway.

Quoted 10 lines

- it requires 2 fds, and it's painful. Currently file managing is done by common path (e.g. io_req_set_file(), __io_req_aux_free()). I'm thinking to make each opcode function handle file grabbing/putting themself with some helpers, as it's done in the patch for splice's out-file.
1. Opcode handler knows, whether it have/needs a file, and thus doesn't need extra checks done in common path.
2. It will be more consistent with splice.
Objections? Ideas?

Sounds reasonable to me, but always easier to judge in patch form :-)

Quoted 3 lines

- do we need offset pointers with fallback to file->f_pos? Or is it enough to have offset value. Jens, I remember you added the first option somewhere, could you tell the reasoning?

I recently added support for -1/cur position, which splice also uses. So you should be fine with that.

Quoted 11 lines · Pavel Begunkov

Quoted 11 lines

Quoted 11 lines

fs/io_uring.c +5 -0

@@ -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,
+	}
 };
 

Quoted 7 lines

Quoted 5 lines

fs/io_uring.c static const struct io_op_def io_op_defs[] = { · line 740

Quoted 1 line

fs/io_uring.c +0 -0

 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.

Quoted 38 lines

Quoted 29 lines

Quoted 29 lines

include/uapi/linux/io_uring.h +9 -2

index 57d05cc5e271..f234b13e7ed3 100644
@@ -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 {

Quoted 8 lines

include/uapi/linux/io_uring.h struct io_uring_sqe { · line 54

Quoted 5 lines

include/uapi/linux/io_uring.h +1 -0

 		__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...

Quoted 2 lines

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.

Quoted 13 lines

Quoted 12 lines

include/uapi/linux/io_uring.h enum { · line 80

Quoted 7 lines

include/uapi/linux/io_uring.h +3 -0

@@ -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?

Quoted 3 lines

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.

Quoted 86 lines · Jens Axboe

Quoted 11 lines · Pavel Begunkov

Quoted 11 lines

Quoted 11 lines

fs/io_uring.c +5 -0

@@ -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,
+	}
 };
 

Quoted 7 lines

Quoted 5 lines

fs/io_uring.c static const struct io_op_def io_op_defs[] = { · line 740

Quoted 1 line

fs/io_uring.c +0 -0

 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.

Quoted 38 lines

Quoted 29 lines

Quoted 29 lines

include/uapi/linux/io_uring.h +9 -2

index 57d05cc5e271..f234b13e7ed3 100644
@@ -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 {

Quoted 8 lines

include/uapi/linux/io_uring.h struct io_uring_sqe { · line 54

Quoted 5 lines

include/uapi/linux/io_uring.h +1 -0

 		__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...

Quoted 2 lines

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.

Quoted 13 lines

Quoted 12 lines

include/uapi/linux/io_uring.h enum { · line 80

Quoted 7 lines

include/uapi/linux/io_uring.h +3 -0

@@ -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?

Quoted 8 lines

Quoted 3 lines

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.

Quoted 12 lines · Jens Axboe

Quoted 7 lines · Pavel Begunkov

It works well for basic cases, but there is still work to be done. E.g. it misses @hash_reg_file checks for the second (output) file. Anyway, there are some questions I want to discuss:

- why sqe->len is __u32? Splice uses size_t, and I think it's better to have something wider (e.g. u64) for fututre use. That's the story behind added sqe->splice_len.

IO operations in Linux generally are INT_MAX, so the u32 is plenty big. That's why I chose it. For this specifically, if you look at splice:

Quoted 4 lines

	if (unlikely(len > MAX_RW_COUNT))
		len = MAX_RW_COUNT;

so anything larger is truncated anyway.

Yeah, I saw this one, but that was rather an argument for the future. It's pretty easy to transfer more than 4GB with sg list, but that would be the case for splice.

Quoted 10 lines

Quoted 9 lines

- it requires 2 fds, and it's painful. Currently file managing is done by common path (e.g. io_req_set_file(), __io_req_aux_free()). I'm thinking to make each opcode function handle file grabbing/putting themself with some helpers, as it's done in the patch for splice's out-file.
1. Opcode handler knows, whether it have/needs a file, and thus doesn't need extra checks done in common path.
2. It will be more consistent with splice.
Objections? Ideas?

Quoted 9 lines

Sounds reasonable to me, but always easier to judge in patch form :-)

Quoted 3 lines

- do we need offset pointers with fallback to file->f_pos? Or is it enough to have offset value. Jens, I remember you added the first option somewhere, could you tell the reasoning?

I recently added support for -1/cur position, which splice also uses. So you should be fine with that.

I always have been thinking about it as a legacy from old days, and one of the problems of posix. It's not hard to count it in the userspace especially in C++ or high-level languages, and is just another obstacle for having a performant API. So, I'd rather get rid of it here. But is there any reasons against?