splice(2) support for io_uring

RFC Superseded Pavel Begunkov 3 participants 16 messages Jan 22, 2020 – Jan 25, 2020

Subsystems (2)

VFS io_uring

lore

Share & tools

lore

Pavel Begunkov

author

2020-01-22 00:06 UTC #0 lore raw

cover splice(2) support for io_uring

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?

Pavel Begunkov (3):
splice: make do_splice public
io_uring: add interface for getting files
io_uring: add splice(2) support

 fs/io_uring.c                 | 152 ++++++++++++++++++++++++++++------
 fs/splice.c                   |   6 +-
 include/linux/splice.h        |   3 +
 include/uapi/linux/io_uring.h |  16 +++-
 4 files changed, 147 insertions(+), 30 deletions(-)

Signature · 1 line

2.24.0

Pavel Begunkov

author

2020-01-22 00:06 UTC #1 lore raw

PATCH 1/3 splice: make do_splice public

Make do_splice(), so other kernel parts can reuse it

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

---
 fs/splice.c            | 6 +++---
 include/linux/splice.h | 3 +++
 2 files changed, 6 insertions(+), 3 deletions(-)

fs/splice.c +3 -3

index 3009652a41c8..6a6f30432688 100644
@@ -1109,9 +1109,9 @@ static int splice_pipe_to_pipe(struct pipe_inode_info *ipipe,
 /*
  * Determine where to splice to/from.
  */
-static long do_splice(struct file *in, loff_t __user *off_in,
-		      struct file *out, loff_t __user *off_out,
-		      size_t len, unsigned int flags)
+long do_splice(struct file *in, loff_t __user *off_in,
+		struct file *out, loff_t __user *off_out,
+		size_t len, unsigned int flags)
 {
 	struct pipe_inode_info *ipipe;
 	struct pipe_inode_info *opipe;

include/linux/splice.h +3 -0

index 74b4911ac16d..ebbbfea48aa0 100644
@@ -78,6 +78,9 @@ extern ssize_t add_to_pipe(struct pipe_inode_info *,
 			      struct pipe_buffer *);
 extern ssize_t splice_direct_to_actor(struct file *, struct splice_desc *,
 				      splice_direct_actor *);
+extern long do_splice(struct file *in, loff_t __user *off_in,
+		      struct file *out, loff_t __user *off_out,
+		      size_t len, unsigned int flags);
 
 /*
  * for dynamic pipe sizing

Signature · 1 line

2.24.0

Pavel Begunkov

author

2020-01-22 00:06 UTC #2 lore raw

PATCH 2/3 io_uring: add interface for getting files

In reply to 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 +41 ~~-25~~

index 8f7846cb1ebf..e9e4aee0fb99 100644
@@ -1161,6 +1161,15 @@ static struct io_kiocb *io_get_req(struct io_ring_ctx *ctx,
 	return NULL;
 }
 
+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);
+}
+
 static void __io_req_do_free(struct io_kiocb *req)
 {
 	if (likely(!io_is_fallback_req(req)))

@@ -1174,12 +1183,8 @@ static void __io_req_aux_free(struct io_kiocb *req)
 	struct io_ring_ctx *ctx = req->ctx;
 
 	kfree(req->io);
-	if (req->file) {
-		if (req->flags & REQ_F_FIXED_FILE)
-			percpu_ref_put(&ctx->file_data->refs);
-		else
-			fput(req->file);
-	}
+	if (req->file)
+		io_put_file(ctx, req->file, (req->flags & REQ_F_FIXED_FILE));
 }
 
 static void __io_free_req(struct io_kiocb *req)

@@ -4355,41 +4360,52 @@ static inline struct file *io_file_from_index(struct io_ring_ctx *ctx,
 	return table->files[index & IORING_FILE_TABLE_MASK];;
 }
 
-static int io_req_set_file(struct io_submit_state *state, struct io_kiocb *req,
-			   const struct io_uring_sqe *sqe)
+static int io_get_file(struct io_submit_state *state, struct io_ring_ctx *ctx,
+			int fd, struct file **out_file, bool fixed)
 {
-	struct io_ring_ctx *ctx = req->ctx;
-	unsigned flags;
-	int fd;
-
-	flags = READ_ONCE(sqe->flags);
-	fd = READ_ONCE(sqe->fd);
-
-	if (!io_req_needs_file(req, fd))
-		return 0;
+	struct file *file;
 
-	if (flags & IOSQE_FIXED_FILE) {
+	if (fixed) {
 		if (unlikely(!ctx->file_data ||
 		    (unsigned) fd >= ctx->nr_user_files))
 			return -EBADF;
 		fd = array_index_nospec(fd, ctx->nr_user_files);
-		req->file = io_file_from_index(ctx, fd);
-		if (!req->file)
+		file = io_file_from_index(ctx, fd);
+		if (!file)
 			return -EBADF;
-		req->flags |= REQ_F_FIXED_FILE;
 		percpu_ref_get(&ctx->file_data->refs);
 	} else {
-		if (req->needs_fixed_file)
-			return -EBADF;
 		trace_io_uring_file_get(ctx, fd);
-		req->file = io_file_get(state, fd);
-		if (unlikely(!req->file))
+		file = io_file_get(state, fd);
+		if (unlikely(!file))
 			return -EBADF;
 	}
 
+	*out_file = file;
 	return 0;
 }
 
+static int io_req_set_file(struct io_submit_state *state, struct io_kiocb *req,
+			   const struct io_uring_sqe *sqe)
+{
+	struct io_ring_ctx *ctx = req->ctx;
+	unsigned flags;
+	int fd;
+	bool fixed;
+
+	flags = READ_ONCE(sqe->flags);
+	fd = READ_ONCE(sqe->fd);
+
+	if (!io_req_needs_file(req, fd))
+		return 0;
+
+	fixed = (flags & IOSQE_FIXED_FILE);
+	if (unlikely(!fixed && req->needs_fixed_file))
+		return -EBADF;
+
+	return io_get_file(state, ctx, fd, &req->file, fixed);
+}
+
 static int io_grab_files(struct io_kiocb *req)
 {
 	int ret = -EBADF;

Signature · 1 line

2.24.0

Jens Axboe

maintainer

2020-01-22 01:54 UTC #3 lore raw

Re: [PATCH 2/3] io_uring: add interface for getting files

Quoted 14 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 10 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.

Signature · 1 line

Jens Axboe

Pavel Begunkov

author

2020-01-22 02:25 UTC #4 lore raw

Re: [PATCH 2/3] io_uring: add interface for getting files

Quoted 17 lines · Jens Axboe

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;
 }
 

Quoted 11 lines

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?

Ok, I'll make it io_put_file(req, file, is_fixed);
It still needs @file, as there can be many per req as in splice.

Quoted 2 lines

Apart from that, looks fine to me.

Signature · 1 line

Pavel Begunkov

Pavel Begunkov

author

2020-01-22 00:06 UTC #5 lore raw

PATCH 3/3 io_uring: add splice(2) support

In reply to Pavel Begunkov

Add support for splice(2). Nothing new, just reuse do_splice().

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

---
 fs/io_uring.c                 | 86 +++++++++++++++++++++++++++++++++++
 include/uapi/linux/io_uring.h | 16 ++++++-
 2 files changed, 100 insertions(+), 2 deletions(-)

fs/io_uring.c +86 -0

index e9e4aee0fb99..44ec9c63c41d 100644
@@ -74,6 +74,7 @@
 #include <linux/namei.h>
 #include <linux/fsnotify.h>
 #include <linux/fadvise.h>
+#include <linux/splice.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/io_uring.h>

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

@@ -534,6 +544,7 @@ struct io_kiocb {
 		struct io_files_update	files_update;
 		struct io_fadvise	fadvise;
 		struct io_madvise	madvise;
+		struct io_splice	splice;
 	};
 
 	struct io_async_ctx		*io;

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

@@ -730,6 +746,10 @@ static void io_queue_linked_timeout(struct io_kiocb *req);
 static int __io_sqe_files_update(struct io_ring_ctx *ctx,
 				 struct io_uring_files_update *ip,
 				 unsigned nr_args);
+static int io_get_file(struct io_submit_state *state,
+		       struct io_ring_ctx *ctx,
+		       int fd, struct file **out_file,
+		       bool fixed);
 
 static struct kmem_cache *req_cachep;

@@ -2322,6 +2342,61 @@ static int io_write(struct io_kiocb *req, struct io_kiocb **nxt,
 	return ret;
 }
 
+static int io_splice_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
+{
+	struct io_splice* sp = &req->splice;
+
+	sp->file_out = NULL;
+	sp->off_in = u64_to_user_ptr(READ_ONCE(sqe->off));
+	sp->off_out = u64_to_user_ptr(READ_ONCE(sqe->off_out));
+	sp->len = READ_ONCE(sqe->splice_len);
+	sp->flags = READ_ONCE(sqe->splice_flags);
+
+	if (unlikely(READ_ONCE(sqe->ioprio) || (sp->flags & ~SPLICE_F_ALL)))
+		return -EINVAL;
+
+	return io_get_file(NULL, req->ctx, READ_ONCE(sqe->fd_out),
+			   &sp->file_out, (sp->flags & IOSQE_SPLICE_FIXED_OUT));
+}
+
+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;
+}
+
 /*
  * IORING_OP_NOP just posts a completion event, nothing else.
  */

@@ -4044,6 +4119,9 @@ static int io_req_defer_prep(struct io_kiocb *req,
 	case IORING_OP_OPENAT2:
 		ret = io_openat2_prep(req, sqe);
 		break;
+	case IORING_OP_SPLICE:
+		ret = io_splice_prep(req, sqe);
+		break;
 	default:
 		printk_once(KERN_WARNING "io_uring: unhandled opcode %d\n",
 				req->opcode);

@@ -4272,6 +4350,14 @@ static int io_issue_sqe(struct io_kiocb *req, const struct io_uring_sqe *sqe,
 		}
 		ret = io_openat2(req, nxt, force_nonblock);
 		break;
+	case IORING_OP_SPLICE:
+		if (sqe) {
+			ret = io_splice_prep(req, sqe);
+			if (ret < 0)
+				break;
+		}
+		ret = io_splice(req, nxt, force_nonblock);
+		break;
 	default:
 		ret = -EINVAL;
 		break;

include/uapi/linux/io_uring.h +14 -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 {
 		__u16	buf_index;	/* index into fixed buffers, if used */
+		__u64	splice_len;
 		__u64	__pad2[3];
 	};
 };

@@ -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)
+
 /*
  * io_uring_setup() flags
  */

@@ -106,6 +117,7 @@ enum {
 	IORING_OP_SEND,
 	IORING_OP_RECV,
 	IORING_OP_OPENAT2,
+	IORING_OP_SPLICE,
 
 	/* this goes last, obviously */
 	IORING_OP_LAST,

Signature · 1 line

2.24.0

Jens Axboe

maintainer

2020-01-22 02:03 UTC #6 lore raw

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

Quoted 6 lines · Pavel Begunkov

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 struct io_rw { · line 388

Quoted 10 lines

fs/io_uring.c +7 -0

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

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

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,
+	}
 };
 
 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 27 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 24 lines

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;

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

Quoted 10 lines

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

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

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?

Signature · 1 line

Jens Axboe

Pavel Begunkov

author

2020-01-22 02:40 UTC #7 lore raw

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

Quoted 5 lines · Jens Axboe

Quoted 4 lines · Pavel Begunkov
fs/io_uring.c +0 -0
@@ -373,6 +374,15 @@ struct io_rw {
 	u64				len;
 };
 

Quoted 14 lines

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.

Right, I don't want to have multiple types and names for it myself.

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

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 71 lines

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 {

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

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.

Signature · 1 line

Pavel Begunkov

Jens Axboe

maintainer

2020-01-22 02:47 UTC #8 lore raw

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

Quoted 11 lines · Pavel Begunkov

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

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

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.

Signature · 1 line

Jens Axboe

Pavel Begunkov

author

2020-01-22 03:17 UTC #9 lore raw

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

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.

Signature · 1 line

Pavel Begunkov

Jens Axboe

maintainer

2020-01-22 03:23 UTC #10 lore raw

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

Quoted 95 lines · Pavel Begunkov

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.

Quoted 5 lines

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.

It won't clash with splice(2), just make that flag illegal if done through splice(2) directly. Honestly I think that's (by FAR) the best way to do it, having a private io_uring flag that acts as a splice flag is really confusing and prone to breakage. Not that it's a huge issue with splice as the flags have been stable for years, so don't really see a high risk of collision. But we should still do it right, which means adding SPLICE_F_OUT_FIXED or whatever you want to call it. Do that as a prep patch, make do_splice() into __do_splice(), and have io_uring call __do_splice(). Currently splice(2) is permissive in terms of flags, so maybe just mask it in do_splice() to be on the safe side. Then we know only internal users will set SPLICE_F_OUT_FIXED, and we'll never run into the risk of having a collision as it's part of the flag space anyway.

The sqe->flags space is very tight, so adding a splice specific opcode there would be bad.

Signature · 1 line

Jens Axboe

kbuild test robot

build bot

2020-01-24 12:31 UTC #11 lore raw

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

In reply to Pavel Begunkov

Thank you for the patch! Yet something to improve:

Hi Pavel,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on next-20200121]
[cannot apply to linus/master v5.5-rc7 v5.5-rc6 v5.5-rc5 v5.5-rc7] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Pavel-Begunkov/splice-2-support-for-io_uring/20200124-114107
base:    bc80e6ad8ee12b0ee6c7d05faf1ebd3f2fb8f1e5
config: powerpc64-defconfig (attached as .config)
compiler: powerpc64-linux-gcc (GCC) 7.5.0

reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.5.0 make.cross ARCH=powerpc64

If you fix the issue, kindly add following tag

Reported-by kbuild test robot <[email protected]>

All errors (new ones prefixed by >>):

fs/io_uring.c: In function 'io_splice_punt':

>> fs/io_uring.c:2364:6: error: too few arguments to function 'get_pipe_info'
     if (get_pipe_info(file))
         ^~~~~~~~~~~~~
   In file included from include/linux/splice.h:12:0,
                    from include/linux/skbuff.h:36,
                    from include/linux/if_ether.h:19,
                    from include/uapi/linux/ethtool.h:19,
                    from include/linux/ethtool.h:18,
                    from include/linux/netdevice.h:37,
                    from include/net/sock.h:46,
                    from fs/io_uring.c:64:
   include/linux/pipe_fs_i.h:266:25: note: declared here
    struct pipe_inode_info *get_pipe_info(struct file *file, bool for_splice);
                            ^~~~~~~~~~~~~

vim +/get_pipe_info +2364 fs/io_uring.c

2361
2362 static bool io_splice_punt(struct file *file)
2363 {

> 2364		if (get_pipe_info(file))

2365 return false;
2366 if (!io_file_supports_async(file))
2367 return true;
2368 return !(file->f_mode & O_NONBLOCK);
2369 }
2370

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org Intel Corporation

kbuild test robot

build bot

2020-01-25 18:28 UTC #12 lore raw

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

In reply to Pavel Begunkov

Thank you for the patch! Perhaps something to improve:

Hi Pavel,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on next-20200121]
[cannot apply to linus/master v5.5-rc7 v5.5-rc6 v5.5-rc5 v5.5-rc7] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Pavel-Begunkov/splice-2-support-for-io_uring/20200124-114107
base:    bc80e6ad8ee12b0ee6c7d05faf1ebd3f2fb8f1e5
config: x86_64-randconfig-a002-20200125 (attached as .config)
compiler: gcc-7 (Debian 7.5.0-3) 7.5.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64

If you fix the issue, kindly add following tag

Reported-by kbuild test robot <[email protected]>

All warnings (new ones prefixed by >>):

In file included from include/linux/export.h:43:0,

                    from include/linux/linkage.h:7,
                    from include/linux/kernel.h:8,
                    from fs/io_uring.c:42:
   fs/io_uring.c: In function 'io_splice_punt':
   fs/io_uring.c:2364:6: error: too few arguments to function 'get_pipe_info'
     if (get_pipe_info(file))
         ^
   include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var'
    #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                                       ^~~~
>> fs/io_uring.c:2364:2: note: in expansion of macro 'if'
     if (get_pipe_info(file))
     ^~
   In file included from include/linux/splice.h:12:0,
                    from include/linux/skbuff.h:36,
                    from include/linux/if_ether.h:19,
                    from include/uapi/linux/ethtool.h:19,
                    from include/linux/ethtool.h:18,
                    from include/linux/netdevice.h:37,
                    from include/net/sock.h:46,
                    from fs/io_uring.c:64:
   include/linux/pipe_fs_i.h:266:25: note: declared here
    struct pipe_inode_info *get_pipe_info(struct file *file, bool for_splice);
                            ^~~~~~~~~~~~~
   In file included from include/linux/export.h:43:0,
                    from include/linux/linkage.h:7,
                    from include/linux/kernel.h:8,
                    from fs/io_uring.c:42:
   fs/io_uring.c:2364:6: error: too few arguments to function 'get_pipe_info'
     if (get_pipe_info(file))
         ^
   include/linux/compiler.h:58:61: note: in definition of macro '__trace_if_var'
    #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
                                                                ^~~~
>> fs/io_uring.c:2364:2: note: in expansion of macro 'if'
     if (get_pipe_info(file))
     ^~
   In file included from include/linux/splice.h:12:0,
                    from include/linux/skbuff.h:36,
                    from include/linux/if_ether.h:19,
                    from include/uapi/linux/ethtool.h:19,
                    from include/linux/ethtool.h:18,
                    from include/linux/netdevice.h:37,
                    from include/net/sock.h:46,
                    from fs/io_uring.c:64:
   include/linux/pipe_fs_i.h:266:25: note: declared here
    struct pipe_inode_info *get_pipe_info(struct file *file, bool for_splice);
                            ^~~~~~~~~~~~~
   In file included from include/linux/export.h:43:0,
                    from include/linux/linkage.h:7,
                    from include/linux/kernel.h:8,
                    from fs/io_uring.c:42:
   fs/io_uring.c:2364:6: error: too few arguments to function 'get_pipe_info'
     if (get_pipe_info(file))
         ^
   include/linux/compiler.h:69:3: note: in definition of macro '__trace_if_value'
     (cond) ?     \
      ^~~~
   include/linux/compiler.h:56:28: note: in expansion of macro '__trace_if_var'
    #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
                               ^~~~~~~~~~~~~~
>> fs/io_uring.c:2364:2: note: in expansion of macro 'if'
     if (get_pipe_info(file))
     ^~
   In file included from include/linux/splice.h:12:0,
                    from include/linux/skbuff.h:36,
                    from include/linux/if_ether.h:19,
                    from include/uapi/linux/ethtool.h:19,
                    from include/linux/ethtool.h:18,
                    from include/linux/netdevice.h:37,
                    from include/net/sock.h:46,
                    from fs/io_uring.c:64:
   include/linux/pipe_fs_i.h:266:25: note: declared here
    struct pipe_inode_info *get_pipe_info(struct file *file, bool for_splice);
                            ^~~~~~~~~~~~~

vim +/if +2364 fs/io_uring.c

2361
2362 static bool io_splice_punt(struct file *file)
2363 {

> 2364		if (get_pipe_info(file))

2365 return false;
2366 if (!io_file_supports_async(file))
2367 return true;
2368 return !(file->f_mode & O_NONBLOCK);
2369 }
2370

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org Intel Corporation

Jens Axboe

maintainer

2020-01-22 01:56 UTC #13 lore raw

In reply to Pavel Begunkov

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

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.

Signature · 1 line

Jens Axboe

Pavel Begunkov

author

2020-01-22 03:12 UTC #14 lore raw

Quoted 9 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.

Quoted 7 lines

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.

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 11 lines

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?

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?

Signature · 1 line

Pavel Begunkov

Jens Axboe

maintainer

2020-01-22 03:30 UTC #15 lore raw

Quoted 13 lines · Pavel Begunkov

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 8 lines

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.

I don't see this changing, ever, basically. And probably not a big deal, if you want to do more than 2GB worth of IO, you simply splice them over multiple commands. At those sizes, the overhead there is negligible.

Quoted 20 lines

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.

Quoted 5 lines

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?

It's not always trivial to do in libraries, or programming languages even. That's why it exists. I would not expect anyone to use it outside of that.

Signature · 1 line

Jens Axboe