* [PATCH 13/18] io_uring: add file set registration
From: Jens Axboe @ 2019-01-28 21:35 UTC (permalink / raw)
To: linux-aio, linux-block, linux-man, linux-api; +Cc: hch, jmoyer, avi, Jens Axboe
In-Reply-To: <20190128213538.13486-1-axboe@kernel.dk>
We normally have to fget/fput for each IO we do on a file. Even with
the batching we do, the cost of the atomic inc/dec of the file usage
count adds up.
This adds IORING_REGISTER_FILES, and IORING_UNREGISTER_FILES opcodes
for the io_uring_register(2) system call. The arguments passed in must
be an array of __s32 holding file descriptors, and nr_args should hold
the number of file descriptors the application wishes to pin for the
duration of the io_uring context (or until IORING_UNREGISTER_FILES is
called).
When used, the application must set IOSQE_FIXED_FILE in the sqe->flags
member. Then, instead of setting sqe->fd to the real fd, it sets sqe->fd
to the index in the array passed in to IORING_REGISTER_FILES.
Files are automatically unregistered when the io_uring context is
torn down. An application need only unregister if it wishes to
register a few set of fds.
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
fs/io_uring.c | 125 +++++++++++++++++++++++++++++-----
include/uapi/linux/io_uring.h | 9 ++-
2 files changed, 116 insertions(+), 18 deletions(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 682714d6f217..77993972879b 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -98,6 +98,10 @@ struct io_ring_ctx {
struct fasync_struct *cq_fasync;
} ____cacheline_aligned_in_smp;
+ /* if used, fixed file set */
+ struct file **user_files;
+ unsigned nr_user_files;
+
/* if used, fixed mapped user buffers */
unsigned nr_user_bufs;
struct io_mapped_ubuf *user_bufs;
@@ -135,6 +139,7 @@ struct io_kiocb {
#define REQ_F_FORCE_NONBLOCK 1 /* inline submission attempt */
#define REQ_F_IOPOLL_COMPLETED 2 /* polled IO has completed */
#define REQ_F_IOPOLL_EAGAIN 4 /* submission got EAGAIN */
+#define REQ_F_FIXED_FILE 8 /* ctx owns file */
u64 user_data;
u64 res;
@@ -357,15 +362,17 @@ static void io_iopoll_complete(struct io_ring_ctx *ctx, unsigned int *nr_events,
* Batched puts of the same file, to avoid dirtying the
* file usage count multiple times, if avoidable.
*/
- if (!file) {
- file = req->rw.ki_filp;
- file_count = 1;
- } else if (file == req->rw.ki_filp) {
- file_count++;
- } else {
- fput_many(file, file_count);
- file = req->rw.ki_filp;
- file_count = 1;
+ if (!(req->flags & REQ_F_FIXED_FILE)) {
+ if (!file) {
+ file = req->rw.ki_filp;
+ file_count = 1;
+ } else if (file == req->rw.ki_filp) {
+ file_count++;
+ } else {
+ fput_many(file, file_count);
+ file = req->rw.ki_filp;
+ file_count = 1;
+ }
}
if (to_free == ARRAY_SIZE(reqs))
@@ -502,13 +509,19 @@ static void kiocb_end_write(struct kiocb *kiocb)
}
}
+static void io_fput(struct io_kiocb *req)
+{
+ if (!(req->flags & REQ_F_FIXED_FILE))
+ fput(req->rw.ki_filp);
+}
+
static void io_complete_rw(struct kiocb *kiocb, long res, long res2)
{
struct io_kiocb *req = container_of(kiocb, struct io_kiocb, rw);
kiocb_end_write(kiocb);
- fput(kiocb->ki_filp);
+ io_fput(req);
io_cqring_add_event(req->ctx, req->user_data, res, 0);
io_free_req(req);
}
@@ -612,7 +625,14 @@ static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe,
struct kiocb *kiocb = &req->rw;
int ret;
- kiocb->ki_filp = io_file_get(state, sqe->fd);
+ if (sqe->flags & IOSQE_FIXED_FILE) {
+ if (unlikely(!ctx->user_files || sqe->fd >= ctx->nr_user_files))
+ return -EBADF;
+ kiocb->ki_filp = ctx->user_files[sqe->fd];
+ req->flags |= REQ_F_FIXED_FILE;
+ } else {
+ kiocb->ki_filp = io_file_get(state, sqe->fd);
+ }
if (unlikely(!kiocb->ki_filp))
return -EBADF;
kiocb->ki_pos = sqe->off;
@@ -651,7 +671,8 @@ static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe,
}
return 0;
out_fput:
- io_file_put(state, kiocb->ki_filp);
+ if (!(sqe->flags & IOSQE_FIXED_FILE))
+ io_file_put(state, kiocb->ki_filp);
return ret;
}
@@ -769,7 +790,7 @@ static ssize_t io_read(struct io_kiocb *req, const struct io_uring_sqe *sqe,
kfree(iovec);
out_fput:
if (unlikely(ret))
- fput(file);
+ io_fput(req);
return ret;
}
@@ -824,7 +845,7 @@ static ssize_t io_write(struct io_kiocb *req, const struct io_uring_sqe *sqe,
kfree(iovec);
out_fput:
if (unlikely(ret))
- fput(file);
+ io_fput(req);
return ret;
}
@@ -862,14 +883,23 @@ static int io_fsync(struct io_kiocb *req, const struct io_uring_sqe *sqe,
if (unlikely(sqe->fsync_flags & ~IORING_FSYNC_DATASYNC))
return -EINVAL;
- file = fget(sqe->fd);
+ if (sqe->flags & IOSQE_FIXED_FILE) {
+ if (unlikely(!ctx->user_files || sqe->fd >= ctx->nr_user_files))
+ return -EBADF;
+ file = ctx->user_files[sqe->fd];
+ } else {
+ file = fget(sqe->fd);
+ }
+
if (unlikely(!file))
return -EBADF;
ret = vfs_fsync_range(file, sqe->off, end > 0 ? end : LLONG_MAX,
sqe->fsync_flags & IORING_FSYNC_DATASYNC);
- fput(file);
+ if (!(sqe->flags & IOSQE_FIXED_FILE))
+ fput(file);
+
io_cqring_add_event(ctx, sqe->user_data, ret, 0);
io_free_req(req);
return 0;
@@ -987,7 +1017,7 @@ static int io_submit_sqe(struct io_ring_ctx *ctx, struct sqe_submit *s,
ssize_t ret;
/* enforce forwards compatibility on users */
- if (unlikely(s->sqe->flags))
+ if (unlikely(s->sqe->flags & ~IOSQE_FIXED_FILE))
return -EINVAL;
req = io_get_req(ctx, state);
@@ -1195,6 +1225,57 @@ static int __io_uring_enter(struct io_ring_ctx *ctx, unsigned to_submit,
return submitted ? submitted : ret;
}
+static int io_sqe_files_unregister(struct io_ring_ctx *ctx)
+{
+ int i;
+
+ if (!ctx->user_files)
+ return -ENXIO;
+
+ for (i = 0; i < ctx->nr_user_files; i++)
+ fput(ctx->user_files[i]);
+
+ kfree(ctx->user_files);
+ ctx->user_files = NULL;
+ ctx->nr_user_files = 0;
+ return 0;
+}
+
+static int io_sqe_files_register(struct io_ring_ctx *ctx, void __user *arg,
+ unsigned nr_args)
+{
+ __s32 __user *fds = (__s32 __user *) arg;
+ int fd, i, ret = 0;
+
+ if (ctx->user_files)
+ return -EBUSY;
+ if (!nr_args)
+ return -EINVAL;
+
+ ctx->user_files = kcalloc(nr_args, sizeof(struct file *), GFP_KERNEL);
+ if (!ctx->user_files)
+ return -ENOMEM;
+
+ for (i = 0; i < nr_args; i++) {
+ ret = -EFAULT;
+ if (copy_from_user(&fd, &fds[i], sizeof(fd)))
+ break;
+
+ ctx->user_files[i] = fget(fd);
+
+ ret = -EBADF;
+ if (!ctx->user_files[i])
+ break;
+ ctx->nr_user_files++;
+ ret = 0;
+ }
+
+ if (ret)
+ io_sqe_files_unregister(ctx);
+
+ return ret;
+}
+
static int io_sq_offload_start(struct io_ring_ctx *ctx)
{
int ret;
@@ -1482,6 +1563,7 @@ static void io_ring_ctx_free(struct io_ring_ctx *ctx)
destroy_workqueue(ctx->sqo_wq);
io_iopoll_reap_events(ctx);
io_sqe_buffer_unregister(ctx);
+ io_sqe_files_unregister(ctx);
io_mem_free(ctx->sq_ring);
io_mem_free(ctx->sq_sqes);
@@ -1777,6 +1859,15 @@ static int __io_uring_register(struct io_ring_ctx *ctx, unsigned opcode,
break;
ret = io_sqe_buffer_unregister(ctx);
break;
+ case IORING_REGISTER_FILES:
+ ret = io_sqe_files_register(ctx, arg, nr_args);
+ break;
+ case IORING_UNREGISTER_FILES:
+ ret = -EINVAL;
+ if (arg || nr_args)
+ break;
+ ret = io_sqe_files_unregister(ctx);
+ break;
default:
ret = -EINVAL;
break;
diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
index 03ce7133c3b2..8323320077ec 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -18,7 +18,7 @@
*/
struct io_uring_sqe {
__u8 opcode; /* type of operation for this sqe */
- __u8 flags; /* as of now unused */
+ __u8 flags; /* IOSQE_ flags */
__u16 ioprio; /* ioprio for the request */
__s32 fd; /* file descriptor to do IO on */
__u64 off; /* offset into file */
@@ -35,6 +35,11 @@ struct io_uring_sqe {
};
};
+/*
+ * sqe->flags
+ */
+#define IOSQE_FIXED_FILE (1 << 0) /* use fixed fileset */
+
/*
* io_uring_setup() flags
*/
@@ -114,5 +119,7 @@ struct io_uring_params {
*/
#define IORING_REGISTER_BUFFERS 0
#define IORING_UNREGISTER_BUFFERS 1
+#define IORING_REGISTER_FILES 2
+#define IORING_UNREGISTER_FILES 3
#endif
--
2.17.1
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply related
* [PATCH 14/18] io_uring: add submission polling
From: Jens Axboe @ 2019-01-28 21:35 UTC (permalink / raw)
To: linux-aio, linux-block, linux-man, linux-api; +Cc: hch, jmoyer, avi, Jens Axboe
In-Reply-To: <20190128213538.13486-1-axboe@kernel.dk>
This enables an application to do IO, without ever entering the kernel.
By using the SQ ring to fill in new sqes and watching for completions
on the CQ ring, we can submit and reap IOs without doing a single system
call. The kernel side thread will poll for new submissions, and in case
of HIPRI/polled IO, it'll also poll for completions.
By default, we allow 1 second of active spinning. This can by changed
by passing in a different grace period at io_uring_register(2) time.
If the thread exceeds this idle time without having any work to do, it
will set:
sq_ring->flags |= IORING_SQ_NEED_WAKEUP.
The application will have to call io_uring_enter() to start things back
up again. If IO is kept busy, that will never be needed. Basically an
application that has this feature enabled will guard it's
io_uring_enter(2) call with:
read_barrier();
if (*sq_ring->flags & IORING_SQ_NEED_WAKEUP)
io_uring_enter(fd, 0, 0, IORING_ENTER_SQ_WAKEUP);
instead of calling it unconditionally.
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
fs/io_uring.c | 232 +++++++++++++++++++++++++++++++++-
include/uapi/linux/io_uring.h | 12 +-
2 files changed, 237 insertions(+), 7 deletions(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 77993972879b..34e786eb2e2d 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -24,6 +24,7 @@
#include <linux/percpu.h>
#include <linux/slab.h>
#include <linux/workqueue.h>
+#include <linux/kthread.h>
#include <linux/blkdev.h>
#include <linux/bvec.h>
#include <linux/anon_inodes.h>
@@ -79,14 +80,16 @@ struct io_ring_ctx {
unsigned cached_sq_head;
unsigned sq_entries;
unsigned sq_mask;
- unsigned sq_thread_cpu;
+ unsigned sq_thread_idle;
struct io_uring_sqe *sq_sqes;
} ____cacheline_aligned_in_smp;
/* IO offload */
struct workqueue_struct *sqo_wq;
+ struct task_struct *sqo_thread; /* if using sq thread polling */
struct mm_struct *sqo_mm;
struct files_struct *sqo_files;
+ wait_queue_head_t sqo_wait;
struct {
/* CQ ring */
@@ -262,6 +265,8 @@ static void __io_cqring_add_event(struct io_ring_ctx *ctx, u64 ki_user_data,
if (waitqueue_active(&ctx->wait))
wake_up(&ctx->wait);
+ if (waitqueue_active(&ctx->sqo_wait))
+ wake_up(&ctx->sqo_wait);
}
static void io_cqring_add_event(struct io_ring_ctx *ctx, u64 ki_user_data,
@@ -1113,6 +1118,168 @@ static bool io_get_sqring(struct io_ring_ctx *ctx, struct sqe_submit *s)
return false;
}
+static int io_submit_sqes(struct io_ring_ctx *ctx, struct sqe_submit *sqes,
+ unsigned int nr, bool mm_fault)
+{
+ struct io_submit_state state, *statep = NULL;
+ int ret, i, submitted = 0;
+
+ if (nr > IO_PLUG_THRESHOLD) {
+ io_submit_state_start(&state, ctx, nr);
+ statep = &state;
+ }
+
+ for (i = 0; i < nr; i++) {
+ if (unlikely(mm_fault))
+ ret = -EFAULT;
+ else
+ ret = io_submit_sqe(ctx, &sqes[i], statep);
+ if (!ret) {
+ submitted++;
+ continue;
+ }
+
+ io_cqring_add_event(ctx, sqes[i].sqe->user_data, ret, 0);
+ }
+
+ if (statep)
+ io_submit_state_end(&state);
+
+ return submitted;
+}
+
+static int io_sq_thread(void *data)
+{
+ struct sqe_submit sqes[IO_IOPOLL_BATCH];
+ struct io_ring_ctx *ctx = data;
+ struct mm_struct *cur_mm = NULL;
+ struct files_struct *old_files;
+ mm_segment_t old_fs;
+ DEFINE_WAIT(wait);
+ unsigned inflight;
+ unsigned long timeout;
+
+ old_files = current->files;
+ current->files = ctx->sqo_files;
+
+ old_fs = get_fs();
+ set_fs(USER_DS);
+
+ timeout = inflight = 0;
+ while (!kthread_should_stop()) {
+ bool all_fixed, mm_fault = false;
+ int i;
+
+ if (inflight) {
+ unsigned nr_events = 0;
+
+ if (ctx->flags & IORING_SETUP_IOPOLL) {
+ /*
+ * We disallow the app entering submit/complete
+ * with polling, so no need to lock the ring.
+ */
+ io_iopoll_check(ctx, &nr_events, 0);
+ } else {
+ /*
+ * Normal IO, just pretend everything completed.
+ * We don't have to poll completions for that.
+ */
+ nr_events = inflight;
+ }
+
+ inflight -= nr_events;
+ if (!inflight)
+ timeout = jiffies + ctx->sq_thread_idle;
+ }
+
+ if (!io_get_sqring(ctx, &sqes[0])) {
+ /*
+ * We're polling. If we're within the defined idle
+ * period, then let us spin without work before going
+ * to sleep.
+ */
+ if (inflight || !time_after(jiffies, timeout)) {
+ cpu_relax();
+ continue;
+ }
+
+ /*
+ * Drop cur_mm before scheduling, we can't hold it for
+ * long periods (or over schedule()). Do this before
+ * adding ourselves to the waitqueue, as the unuse/drop
+ * may sleep.
+ */
+ if (cur_mm) {
+ unuse_mm(cur_mm);
+ mmput(cur_mm);
+ cur_mm = NULL;
+ }
+
+ prepare_to_wait(&ctx->sqo_wait, &wait,
+ TASK_INTERRUPTIBLE);
+
+ /* Tell userspace we may need a wakeup call */
+ ctx->sq_ring->flags |= IORING_SQ_NEED_WAKEUP;
+ smp_wmb();
+
+ if (!io_get_sqring(ctx, &sqes[0])) {
+ if (kthread_should_park())
+ kthread_parkme();
+ if (kthread_should_stop()) {
+ finish_wait(&ctx->sqo_wait, &wait);
+ break;
+ }
+ if (signal_pending(current))
+ flush_signals(current);
+ schedule();
+ finish_wait(&ctx->sqo_wait, &wait);
+
+ ctx->sq_ring->flags &= ~IORING_SQ_NEED_WAKEUP;
+ smp_wmb();
+ continue;
+ }
+ finish_wait(&ctx->sqo_wait, &wait);
+
+ ctx->sq_ring->flags &= ~IORING_SQ_NEED_WAKEUP;
+ smp_wmb();
+ }
+
+ i = 0;
+ all_fixed = true;
+ do {
+ if (all_fixed && io_sqe_needs_user(sqes[i].sqe))
+ all_fixed = false;
+
+ i++;
+ if (i == ARRAY_SIZE(sqes))
+ break;
+ } while (io_get_sqring(ctx, &sqes[i]));
+
+ io_commit_sqring(ctx);
+
+ /* Unless all new commands are FIXED regions, grab mm */
+ if (!all_fixed && !cur_mm) {
+ mm_fault = !mmget_not_zero(ctx->sqo_mm);
+ if (!mm_fault) {
+ use_mm(ctx->sqo_mm);
+ cur_mm = ctx->sqo_mm;
+ }
+ }
+
+ inflight += io_submit_sqes(ctx, sqes, i, mm_fault);
+ }
+
+ io_iopoll_reap_events(ctx);
+
+ current->files = old_files;
+ set_fs(old_fs);
+ if (cur_mm) {
+ unuse_mm(cur_mm);
+ mmput(cur_mm);
+ }
+ return 0;
+}
+
static int io_ring_submit(struct io_ring_ctx *ctx, unsigned int to_submit)
{
struct io_submit_state state, *statep = NULL;
@@ -1198,6 +1365,17 @@ static int __io_uring_enter(struct io_ring_ctx *ctx, unsigned to_submit,
{
int submitted, ret;
+ /*
+ * For SQ polling, the thread will do all submissions and completions.
+ * Just return the requested submit count, and wake the thread if
+ * we were asked to.
+ */
+ if (ctx->flags & IORING_SETUP_SQPOLL) {
+ if (flags & IORING_ENTER_SQ_WAKEUP)
+ wake_up(&ctx->sqo_wait);
+ return to_submit;
+ }
+
submitted = ret = 0;
if (to_submit) {
submitted = io_ring_submit(ctx, to_submit);
@@ -1276,10 +1454,12 @@ static int io_sqe_files_register(struct io_ring_ctx *ctx, void __user *arg,
return ret;
}
-static int io_sq_offload_start(struct io_ring_ctx *ctx)
+static int io_sq_offload_start(struct io_ring_ctx *ctx,
+ struct io_uring_params *p)
{
int ret;
+ init_waitqueue_head(&ctx->sqo_wait);
ctx->sqo_mm = current->mm;
/*
@@ -1292,6 +1472,34 @@ static int io_sq_offload_start(struct io_ring_ctx *ctx)
if (!ctx->sqo_files)
goto err;
+ ctx->sq_thread_idle = msecs_to_jiffies(p->sq_thread_idle);
+ if (!ctx->sq_thread_idle)
+ ctx->sq_thread_idle = HZ;
+
+ if (ctx->flags & IORING_SETUP_SQPOLL) {
+ if (p->flags & IORING_SETUP_SQ_AFF) {
+ int cpu;
+
+ cpu = array_index_nospec(p->sq_thread_cpu, NR_CPUS);
+ ctx->sqo_thread = kthread_create_on_cpu(io_sq_thread,
+ ctx, cpu,
+ "io_uring-sq");
+ } else {
+ ctx->sqo_thread = kthread_create(io_sq_thread, ctx,
+ "io_uring-sq");
+ }
+ if (IS_ERR(ctx->sqo_thread)) {
+ ret = PTR_ERR(ctx->sqo_thread);
+ ctx->sqo_thread = NULL;
+ goto err;
+ }
+ wake_up_process(ctx->sqo_thread);
+ } else if (p->flags & IORING_SETUP_SQ_AFF) {
+ /* Can't have SQ_AFF without SQPOLL */
+ ret = -EINVAL;
+ goto err;
+ }
+
/* Do QD, or 2 * CPUS, whatever is smallest */
ctx->sqo_wq = alloc_workqueue("io_ring-wq", WQ_UNBOUND | WQ_FREEZABLE,
min(ctx->sq_entries - 1, 2 * num_online_cpus()));
@@ -1302,6 +1510,11 @@ static int io_sq_offload_start(struct io_ring_ctx *ctx)
return 0;
err:
+ if (ctx->sqo_thread) {
+ kthread_park(ctx->sqo_thread);
+ kthread_stop(ctx->sqo_thread);
+ ctx->sqo_thread = NULL;
+ }
if (ctx->sqo_files)
ctx->sqo_files = NULL;
ctx->sqo_mm = NULL;
@@ -1560,8 +1773,13 @@ static int io_sqe_buffer_register(struct io_ring_ctx *ctx, void __user *arg,
static void io_ring_ctx_free(struct io_ring_ctx *ctx)
{
+ if (ctx->sqo_thread) {
+ kthread_park(ctx->sqo_thread);
+ kthread_stop(ctx->sqo_thread);
+ }
destroy_workqueue(ctx->sqo_wq);
- io_iopoll_reap_events(ctx);
+ if (!(ctx->flags & IORING_SETUP_SQPOLL))
+ io_iopoll_reap_events(ctx);
io_sqe_buffer_unregister(ctx);
io_sqe_files_unregister(ctx);
@@ -1602,7 +1820,8 @@ static void io_ring_ctx_wait_and_kill(struct io_ring_ctx *ctx)
percpu_ref_kill(&ctx->refs);
mutex_unlock(&ctx->uring_lock);
- io_iopoll_reap_events(ctx);
+ if (!(ctx->flags & IORING_SETUP_SQPOLL))
+ io_iopoll_reap_events(ctx);
wait_for_completion(&ctx->ctx_done);
io_ring_ctx_free(ctx);
}
@@ -1771,7 +1990,7 @@ static int io_uring_create(unsigned entries, struct io_uring_params *p)
if (ret)
goto err;
- ret = io_sq_offload_start(ctx);
+ ret = io_sq_offload_start(ctx, p);
if (ret)
goto err;
@@ -1820,7 +2039,8 @@ static long io_uring_setup(u32 entries, struct io_uring_params __user *params)
return -EINVAL;
}
- if (p.flags & ~IORING_SETUP_IOPOLL)
+ if (p.flags & ~(IORING_SETUP_IOPOLL | IORING_SETUP_SQPOLL |
+ IORING_SETUP_SQ_AFF))
return -EINVAL;
ret = io_uring_create(entries, &p);
diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
index 8323320077ec..cb3592d618fe 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -44,6 +44,8 @@ struct io_uring_sqe {
* io_uring_setup() flags
*/
#define IORING_SETUP_IOPOLL (1 << 0) /* io_context is polled */
+#define IORING_SETUP_SQPOLL (1 << 1) /* SQ poll thread */
+#define IORING_SETUP_SQ_AFF (1 << 2) /* sq_thread_cpu is valid */
#define IORING_OP_NOP 0
#define IORING_OP_READV 1
@@ -87,6 +89,11 @@ struct io_sqring_offsets {
__u32 resv[3];
};
+/*
+ * sq_ring->flags
+ */
+#define IORING_SQ_NEED_WAKEUP (1 << 0) /* needs io_uring_enter wakeup */
+
struct io_cqring_offsets {
__u32 head;
__u32 tail;
@@ -101,6 +108,7 @@ struct io_cqring_offsets {
* io_uring_enter(2) flags
*/
#define IORING_ENTER_GETEVENTS (1 << 0)
+#define IORING_ENTER_SQ_WAKEUP (1 << 1)
/*
* Passed in for io_uring_setup(2). Copied back with updated info on success
@@ -109,7 +117,9 @@ struct io_uring_params {
__u32 sq_entries;
__u32 cq_entries;
__u32 flags;
- __u16 resv[10];
+ __u16 sq_thread_cpu;
+ __u16 sq_thread_idle;
+ __u16 resv[8];
struct io_sqring_offsets sq_off;
struct io_cqring_offsets cq_off;
};
--
2.17.1
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply related
* [PATCH 15/18] io_uring: add io_kiocb ref count
From: Jens Axboe @ 2019-01-28 21:35 UTC (permalink / raw)
To: linux-aio, linux-block, linux-man, linux-api; +Cc: hch, jmoyer, avi, Jens Axboe
In-Reply-To: <20190128213538.13486-1-axboe@kernel.dk>
We'll use this for the POLL implementation. Regular requests will
NOT be using references, so initialize it to 0. Any real use of
the io_kiocb ref will initialize it to at least 2.
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
fs/io_uring.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 34e786eb2e2d..71bf890e25f2 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -139,6 +139,7 @@ struct io_kiocb {
struct io_ring_ctx *ctx;
struct list_head list;
unsigned int flags;
+ refcount_t refs;
#define REQ_F_FORCE_NONBLOCK 1 /* inline submission attempt */
#define REQ_F_IOPOLL_COMPLETED 2 /* polled IO has completed */
#define REQ_F_IOPOLL_EAGAIN 4 /* submission got EAGAIN */
@@ -319,6 +320,7 @@ static struct io_kiocb *io_get_req(struct io_ring_ctx *ctx,
if (req) {
req->ctx = ctx;
req->flags = 0;
+ refcount_set(&req->refs, 0);
return req;
}
@@ -338,8 +340,10 @@ static void io_free_req_many(struct io_ring_ctx *ctx, void **reqs, int *nr)
static void io_free_req(struct io_kiocb *req)
{
- io_ring_drop_ctx_refs(req->ctx, 1);
- kmem_cache_free(req_cachep, req);
+ if (!refcount_read(&req->refs) || refcount_dec_and_test(&req->refs)) {
+ io_ring_drop_ctx_refs(req->ctx, 1);
+ kmem_cache_free(req_cachep, req);
+ }
}
/*
--
2.17.1
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply related
* [PATCH 16/18] io_uring: add support for IORING_OP_POLL
From: Jens Axboe @ 2019-01-28 21:35 UTC (permalink / raw)
To: linux-aio, linux-block, linux-man, linux-api; +Cc: hch, jmoyer, avi, Jens Axboe
In-Reply-To: <20190128213538.13486-1-axboe@kernel.dk>
This is basically a direct port of bfe4037e722e, which implements a
one-shot poll command through aio. Description below is based on that
commit as well. However, instead of adding a POLL command and relying
on io_cancel(2) to remove it, we mimic the epoll(2) interface of
having a command to add a poll notification, IORING_OP_POLL_ADD,
and one to remove it again, IORING_OP_POLL_REMOVE.
To poll for a file descriptor the application should submit an sqe of
type IORING_OP_POLL. It will poll the fd for the events specified in the
poll_events field.
Unlike poll or epoll without EPOLLONESHOT this interface always works in
one shot mode, that is once the sqe is completed, it will have to be
resubmitted.
Based-on-code-from: Christoph Hellwig <hch@lst.de>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
fs/io_uring.c | 245 ++++++++++++++++++++++++++++++++++
include/uapi/linux/io_uring.h | 3 +
2 files changed, 248 insertions(+)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 71bf890e25f2..13e9bb0ce44e 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -122,6 +122,7 @@ struct io_ring_ctx {
spinlock_t completion_lock;
bool poll_multi_file;
struct list_head poll_list;
+ struct list_head cancel_list;
} ____cacheline_aligned_in_smp;
};
@@ -130,9 +131,19 @@ struct sqe_submit {
unsigned index;
};
+struct io_poll_iocb {
+ struct file *file;
+ struct wait_queue_head *head;
+ __poll_t events;
+ bool woken;
+ bool canceled;
+ struct wait_queue_entry wait;
+};
+
struct io_kiocb {
union {
struct kiocb rw;
+ struct io_poll_iocb poll;
struct sqe_submit submit;
};
@@ -204,6 +215,7 @@ static struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
init_waitqueue_head(&ctx->wait);
spin_lock_init(&ctx->completion_lock);
INIT_LIST_HEAD(&ctx->poll_list);
+ INIT_LIST_HEAD(&ctx->cancel_list);
return ctx;
}
@@ -914,6 +926,232 @@ static int io_fsync(struct io_kiocb *req, const struct io_uring_sqe *sqe,
return 0;
}
+static void io_poll_remove_one(struct io_kiocb *req)
+{
+ struct io_poll_iocb *poll = &req->poll;
+
+ spin_lock(&poll->head->lock);
+ WRITE_ONCE(poll->canceled, true);
+ if (!list_empty(&poll->wait.entry)) {
+ list_del_init(&poll->wait.entry);
+ queue_work(req->ctx->sqo_wq, &req->work);
+ }
+ spin_unlock(&poll->head->lock);
+
+ list_del_init(&req->list);
+}
+
+static void io_poll_remove_all(struct io_ring_ctx *ctx)
+{
+ struct io_kiocb *req;
+
+ spin_lock_irq(&ctx->completion_lock);
+ while (!list_empty(&ctx->cancel_list)) {
+ req = list_first_entry(&ctx->cancel_list, struct io_kiocb,list);
+ io_poll_remove_one(req);
+ }
+ spin_unlock_irq(&ctx->completion_lock);
+}
+
+/*
+ * Find a running poll command that matches one specified in sqe->addr,
+ * and remove it if found.
+ */
+static int io_poll_remove(struct io_kiocb *req, const struct io_uring_sqe *sqe)
+{
+ struct io_ring_ctx *ctx = req->ctx;
+ struct io_kiocb *poll_req, *next;
+ int ret = -ENOENT;
+
+ if (unlikely(req->ctx->flags & IORING_SETUP_IOPOLL))
+ return -EINVAL;
+ if (sqe->ioprio || sqe->off || sqe->len || sqe->buf_index ||
+ sqe->poll_events)
+ return -EINVAL;
+
+ spin_lock_irq(&ctx->completion_lock);
+ list_for_each_entry_safe(poll_req, next, &ctx->cancel_list, list) {
+ if (sqe->addr == poll_req->user_data) {
+ io_poll_remove_one(poll_req);
+ ret = 0;
+ break;
+ }
+ }
+ spin_unlock_irq(&ctx->completion_lock);
+
+ io_cqring_add_event(req->ctx, sqe->user_data, ret, 0);
+ io_free_req(req);
+ return 0;
+}
+
+static void io_poll_complete(struct io_kiocb *req, __poll_t mask)
+{
+ io_cqring_add_event(req->ctx, req->user_data, mangle_poll(mask), 0);
+ io_fput(req);
+ io_free_req(req);
+}
+
+static void io_poll_complete_work(struct work_struct *work)
+{
+ struct io_kiocb *req = container_of(work, struct io_kiocb, work);
+ struct io_poll_iocb *poll = &req->poll;
+ struct poll_table_struct pt = { ._key = poll->events };
+ struct io_ring_ctx *ctx = req->ctx;
+ __poll_t mask = 0;
+
+ if (!READ_ONCE(poll->canceled))
+ mask = vfs_poll(poll->file, &pt) & poll->events;
+
+ /*
+ * Note that ->ki_cancel callers also delete iocb from active_reqs after
+ * calling ->ki_cancel. We need the ctx_lock roundtrip here to
+ * synchronize with them. In the cancellation case the list_del_init
+ * itself is not actually needed, but harmless so we keep it in to
+ * avoid further branches in the fast path.
+ */
+ spin_lock_irq(&ctx->completion_lock);
+ if (!mask && !READ_ONCE(poll->canceled)) {
+ add_wait_queue(poll->head, &poll->wait);
+ spin_unlock_irq(&ctx->completion_lock);
+ return;
+ }
+ list_del_init(&req->list);
+ spin_unlock_irq(&ctx->completion_lock);
+
+ io_poll_complete(req, mask);
+}
+
+static int io_poll_wake(struct wait_queue_entry *wait, unsigned mode, int sync,
+ void *key)
+{
+ struct io_poll_iocb *poll = container_of(wait, struct io_poll_iocb,
+ wait);
+ struct io_kiocb *req = container_of(poll, struct io_kiocb, poll);
+ struct io_ring_ctx *ctx = req->ctx;
+ __poll_t mask = key_to_poll(key);
+
+ poll->woken = true;
+
+ /* for instances that support it check for an event match first: */
+ if (mask) {
+ if (!(mask & poll->events))
+ return 0;
+
+ /* try to complete the iocb inline if we can: */
+ if (spin_trylock(&ctx->completion_lock)) {
+ list_del(&req->list);
+ spin_unlock(&ctx->completion_lock);
+
+ list_del_init(&poll->wait.entry);
+ io_poll_complete(req, mask);
+ return 1;
+ }
+ }
+
+ list_del_init(&poll->wait.entry);
+ queue_work(ctx->sqo_wq, &req->work);
+ return 1;
+}
+
+struct io_poll_table {
+ struct poll_table_struct pt;
+ struct io_kiocb *req;
+ int error;
+};
+
+static void io_poll_queue_proc(struct file *file, struct wait_queue_head *head,
+ struct poll_table_struct *p)
+{
+ struct io_poll_table *pt = container_of(p, struct io_poll_table, pt);
+
+ if (unlikely(pt->req->poll.head)) {
+ pt->error = -EINVAL;
+ return;
+ }
+
+ pt->error = 0;
+ pt->req->poll.head = head;
+ add_wait_queue(head, &pt->req->poll.wait);
+}
+
+static int io_poll_add(struct io_kiocb *req, const struct io_uring_sqe *sqe)
+{
+ struct io_poll_iocb *poll = &req->poll;
+ struct io_ring_ctx *ctx = req->ctx;
+ struct io_poll_table ipt;
+ __poll_t mask;
+
+ if (unlikely(req->ctx->flags & IORING_SETUP_IOPOLL))
+ return -EINVAL;
+ if (sqe->addr || sqe->ioprio || sqe->off || sqe->len || sqe->buf_index)
+ return -EINVAL;
+
+ INIT_WORK(&req->work, io_poll_complete_work);
+ poll->events = demangle_poll(sqe->poll_events) | EPOLLERR | EPOLLHUP;
+
+ if (sqe->flags & IOSQE_FIXED_FILE) {
+ if (unlikely(!ctx->user_files || sqe->fd >= ctx->nr_user_files))
+ return -EBADF;
+ poll->file = ctx->user_files[sqe->fd];
+ req->flags |= REQ_F_FIXED_FILE;
+ } else {
+ poll->file = fget(sqe->fd);
+ }
+ if (unlikely(!poll->file))
+ return -EBADF;
+
+ poll->head = NULL;
+ poll->woken = false;
+ poll->canceled = false;
+
+ ipt.pt._qproc = io_poll_queue_proc;
+ ipt.pt._key = poll->events;
+ ipt.req = req;
+ ipt.error = -EINVAL; /* same as no support for IOCB_CMD_POLL */
+
+ /* initialized the list so that we can do list_empty checks */
+ INIT_LIST_HEAD(&poll->wait.entry);
+ init_waitqueue_func_entry(&poll->wait, io_poll_wake);
+
+ /* one for removal from waitqueue, one for this function */
+ refcount_set(&req->refs, 2);
+
+ mask = vfs_poll(poll->file, &ipt.pt) & poll->events;
+ if (unlikely(!poll->head)) {
+ /* we did not manage to set up a waitqueue, done */
+ goto out;
+ }
+
+ spin_lock_irq(&ctx->completion_lock);
+ spin_lock(&poll->head->lock);
+ if (poll->woken) {
+ /* wake_up context handles the rest */
+ mask = 0;
+ ipt.error = 0;
+ } else if (mask || ipt.error) {
+ /* if we get an error or a mask we are done */
+ WARN_ON_ONCE(list_empty(&poll->wait.entry));
+ list_del_init(&poll->wait.entry);
+ } else {
+ /* actually waiting for an event */
+ list_add_tail(&req->list, &ctx->cancel_list);
+ }
+ spin_unlock(&poll->head->lock);
+ spin_unlock_irq(&ctx->completion_lock);
+
+out:
+ if (unlikely(ipt.error)) {
+ if (!(sqe->flags & IOSQE_FIXED_FILE))
+ fput(poll->file);
+ return ipt.error;
+ }
+
+ if (mask)
+ io_poll_complete(req, mask);
+ io_free_req(req);
+ return 0;
+}
+
static int __io_submit_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
struct sqe_submit *s, bool force_nonblock,
struct io_submit_state *state)
@@ -949,6 +1187,12 @@ static int __io_submit_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
case IORING_OP_FSYNC:
ret = io_fsync(req, sqe, force_nonblock);
break;
+ case IORING_OP_POLL_ADD:
+ ret = io_poll_add(req, sqe);
+ break;
+ case IORING_OP_POLL_REMOVE:
+ ret = io_poll_remove(req, sqe);
+ break;
default:
ret = -EINVAL;
break;
@@ -1824,6 +2068,7 @@ static void io_ring_ctx_wait_and_kill(struct io_ring_ctx *ctx)
percpu_ref_kill(&ctx->refs);
mutex_unlock(&ctx->uring_lock);
+ io_poll_remove_all(ctx);
if (!(ctx->flags & IORING_SETUP_SQPOLL))
io_iopoll_reap_events(ctx);
wait_for_completion(&ctx->ctx_done);
diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
index cb3592d618fe..39d3d3336dce 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -27,6 +27,7 @@ struct io_uring_sqe {
union {
__kernel_rwf_t rw_flags;
__u32 fsync_flags;
+ __u16 poll_events;
};
__u64 user_data; /* data to be passed back at completion time */
union {
@@ -53,6 +54,8 @@ struct io_uring_sqe {
#define IORING_OP_FSYNC 3
#define IORING_OP_READ_FIXED 4
#define IORING_OP_WRITE_FIXED 5
+#define IORING_OP_POLL_ADD 6
+#define IORING_OP_POLL_REMOVE 7
/*
* sqe->fsync_flags
--
2.17.1
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply related
* [PATCH 17/18] io_uring: allow workqueue item to handle multiple buffered requests
From: Jens Axboe @ 2019-01-28 21:35 UTC (permalink / raw)
To: linux-aio, linux-block, linux-man, linux-api; +Cc: hch, jmoyer, avi, Jens Axboe
In-Reply-To: <20190128213538.13486-1-axboe@kernel.dk>
Right now we punt any buffered request that ends up triggering an
-EAGAIN to an async workqueue. This works fine in terms of providing
async execution of them, but it also can create quite a lot of work
queue items. For sequentially buffered IO, it's advantageous to
serialize the issue of them. For reads, the first one will trigger a
read-ahead, and subsequent request merely end up waiting on later pages
to complete. For writes, devices usually respond better to streamed
sequential writes.
Add state to track the last buffered request we punted to a work queue,
and if the next one is sequential to the previous, attempt to get the
previous work item to handle it. We limit the number of sequential
add-ons to the a multiple (8) of the max read-ahead size of the file.
This should be a good number for both reads and wries, as it defines the
max IO size the device can do directly.
This drastically cuts down on the number of context switches we need to
handle buffered sequential IO, and a basic test case of copying a big
file with io_uring sees a 5x speedup.
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
fs/io_uring.c | 231 ++++++++++++++++++++++++++++++++++++++++++--------
1 file changed, 194 insertions(+), 37 deletions(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 13e9bb0ce44e..0240e0a04f6a 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -67,6 +67,16 @@ struct io_mapped_ubuf {
unsigned int nr_bvecs;
};
+struct async_list {
+ spinlock_t lock;
+ atomic_t cnt;
+ struct list_head list;
+
+ struct file *file;
+ off_t io_end;
+ size_t io_pages;
+};
+
struct io_ring_ctx {
struct {
struct percpu_ref refs;
@@ -124,6 +134,8 @@ struct io_ring_ctx {
struct list_head poll_list;
struct list_head cancel_list;
} ____cacheline_aligned_in_smp;
+
+ struct async_list pending_async[2];
};
struct sqe_submit {
@@ -155,6 +167,7 @@ struct io_kiocb {
#define REQ_F_IOPOLL_COMPLETED 2 /* polled IO has completed */
#define REQ_F_IOPOLL_EAGAIN 4 /* submission got EAGAIN */
#define REQ_F_FIXED_FILE 8 /* ctx owns file */
+#define REQ_F_SEQ_PREV 16 /* sequential with previous */
u64 user_data;
u64 res;
@@ -198,6 +211,7 @@ static void io_ring_ctx_ref_free(struct percpu_ref *ref)
static struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
{
struct io_ring_ctx *ctx;
+ int i;
ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
if (!ctx)
@@ -213,6 +227,11 @@ static struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
init_completion(&ctx->ctx_done);
mutex_init(&ctx->uring_lock);
init_waitqueue_head(&ctx->wait);
+ for (i = 0; i < ARRAY_SIZE(ctx->pending_async); i++) {
+ spin_lock_init(&ctx->pending_async[i].lock);
+ INIT_LIST_HEAD(&ctx->pending_async[i].list);
+ atomic_set(&ctx->pending_async[i].cnt, 0);
+ }
spin_lock_init(&ctx->completion_lock);
INIT_LIST_HEAD(&ctx->poll_list);
INIT_LIST_HEAD(&ctx->cancel_list);
@@ -772,6 +791,39 @@ static int io_import_iovec(struct io_ring_ctx *ctx, int rw,
return import_iovec(rw, buf, sqe->len, UIO_FASTIOV, iovec, iter);
}
+static void io_async_list_note(int rw, struct io_kiocb *req, size_t len)
+{
+ struct async_list *async_list = &req->ctx->pending_async[rw];
+ struct kiocb *kiocb = &req->rw;
+ struct file *filp = kiocb->ki_filp;
+ off_t io_end = kiocb->ki_pos + len;
+
+ if (filp == async_list->file && kiocb->ki_pos == async_list->io_end) {
+ unsigned long max_pages;
+
+ /* Use 8x RA size as a decent limiter for both reads/writes */
+ max_pages = filp->f_ra.ra_pages;
+ if (!max_pages)
+ max_pages = VM_MAX_READAHEAD >> (PAGE_SHIFT - 10);
+ max_pages *= 8;
+
+ len >>= PAGE_SHIFT;
+ if (async_list->io_pages + len <= max_pages) {
+ req->flags |= REQ_F_SEQ_PREV;
+ async_list->io_pages += len;
+ } else {
+ io_end = 0;
+ async_list->io_pages = 0;
+ }
+ }
+
+ if (async_list->file != filp) {
+ async_list->io_pages = 0;
+ async_list->file = filp;
+ }
+ async_list->io_end = io_end;
+}
+
static ssize_t io_read(struct io_kiocb *req, const struct io_uring_sqe *sqe,
bool force_nonblock, struct io_submit_state *state)
{
@@ -779,6 +831,7 @@ static ssize_t io_read(struct io_kiocb *req, const struct io_uring_sqe *sqe,
struct kiocb *kiocb = &req->rw;
struct iov_iter iter;
struct file *file;
+ size_t iov_count;
ssize_t ret;
ret = io_prep_rw(req, sqe, force_nonblock, state);
@@ -797,16 +850,19 @@ static ssize_t io_read(struct io_kiocb *req, const struct io_uring_sqe *sqe,
if (ret)
goto out_fput;
- ret = rw_verify_area(READ, file, &kiocb->ki_pos, iov_iter_count(&iter));
+ iov_count = iov_iter_count(&iter);
+ ret = rw_verify_area(READ, file, &kiocb->ki_pos, iov_count);
if (!ret) {
ssize_t ret2;
/* Catch -EAGAIN return for forced non-blocking submission */
ret2 = call_read_iter(file, kiocb, &iter);
- if (!force_nonblock || ret2 != -EAGAIN)
+ if (!force_nonblock || ret2 != -EAGAIN) {
io_rw_done(kiocb, ret2);
- else
+ } else {
+ io_async_list_note(READ, req, iov_count);
ret = -EAGAIN;
+ }
}
kfree(iovec);
out_fput:
@@ -822,6 +878,7 @@ static ssize_t io_write(struct io_kiocb *req, const struct io_uring_sqe *sqe,
struct kiocb *kiocb = &req->rw;
struct iov_iter iter;
struct file *file;
+ size_t iov_count;
ssize_t ret;
ret = io_prep_rw(req, sqe, force_nonblock, state);
@@ -829,10 +886,6 @@ static ssize_t io_write(struct io_kiocb *req, const struct io_uring_sqe *sqe,
return ret;
file = kiocb->ki_filp;
- ret = -EAGAIN;
- if (force_nonblock && !(kiocb->ki_flags & IOCB_DIRECT))
- goto out_fput;
-
ret = -EBADF;
if (unlikely(!(file->f_mode & FMODE_WRITE)))
goto out_fput;
@@ -844,8 +897,15 @@ static ssize_t io_write(struct io_kiocb *req, const struct io_uring_sqe *sqe,
if (ret)
goto out_fput;
- ret = rw_verify_area(WRITE, file, &kiocb->ki_pos,
- iov_iter_count(&iter));
+ iov_count = iov_iter_count(&iter);
+
+ ret = -EAGAIN;
+ if (force_nonblock && !(kiocb->ki_flags & IOCB_DIRECT)) {
+ io_async_list_note(WRITE, req, iov_count);
+ goto out_free;
+ }
+
+ ret = rw_verify_area(WRITE, file, &kiocb->ki_pos, iov_count);
if (!ret) {
/*
* Open-code file_start_write here to grab freeze protection,
@@ -863,6 +923,7 @@ static ssize_t io_write(struct io_kiocb *req, const struct io_uring_sqe *sqe,
kiocb->ki_flags |= IOCB_WRITE;
io_rw_done(kiocb, call_write_iter(file, kiocb, &iter));
}
+out_free:
kfree(iovec);
out_fput:
if (unlikely(ret))
@@ -1210,6 +1271,21 @@ static int __io_submit_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
return 0;
}
+static struct async_list *io_async_list_from_sqe(struct io_ring_ctx *ctx,
+ const struct io_uring_sqe *sqe)
+{
+ switch (sqe->opcode) {
+ case IORING_OP_READV:
+ case IORING_OP_READ_FIXED:
+ return &ctx->pending_async[READ];
+ case IORING_OP_WRITEV:
+ case IORING_OP_WRITE_FIXED:
+ return &ctx->pending_async[WRITE];
+ default:
+ return NULL;
+ }
+}
+
static inline bool io_sqe_needs_user(const struct io_uring_sqe *sqe)
{
return !(sqe->opcode == IORING_OP_READ_FIXED ||
@@ -1219,50 +1295,124 @@ static inline bool io_sqe_needs_user(const struct io_uring_sqe *sqe)
static void io_sq_wq_submit_work(struct work_struct *work)
{
struct io_kiocb *req = container_of(work, struct io_kiocb, work);
- struct sqe_submit *s = &req->submit;
- u64 user_data = s->sqe->user_data;
struct io_ring_ctx *ctx = req->ctx;
+ struct mm_struct *cur_mm = NULL;
struct files_struct *old_files;
+ struct async_list *async_list;
+ LIST_HEAD(req_list);
mm_segment_t old_fs;
- bool needs_user;
int ret;
- /* Ensure we clear previously set forced non-block flag */
- req->flags &= ~REQ_F_FORCE_NONBLOCK;
-
old_files = current->files;
current->files = ctx->sqo_files;
+ async_list = io_async_list_from_sqe(ctx, req->submit.sqe);
+restart:
+ do {
+ struct sqe_submit *s = &req->submit;
+ u64 user_data = s->sqe->user_data;
+
+ /* Ensure we clear previously set forced non-block flag */
+ req->flags &= ~REQ_F_FORCE_NONBLOCK;
+
+ ret = 0;
+ if (io_sqe_needs_user(s->sqe) && !cur_mm) {
+ if (!mmget_not_zero(ctx->sqo_mm)) {
+ ret = -EFAULT;
+ } else {
+ cur_mm = ctx->sqo_mm;;
+ use_mm(ctx->sqo_mm);
+ old_fs = get_fs();
+ set_fs(USER_DS);
+ }
+ }
+
+ if (!ret)
+ ret = __io_submit_sqe(ctx, req, s, false, NULL);
+ if (ret) {
+ io_cqring_add_event(ctx, user_data, ret, 0);
+ io_free_req(req);
+ }
+ if (!async_list)
+ break;
+ if (!list_empty(&req_list)) {
+ req = list_first_entry(&req_list, struct io_kiocb,
+ list);
+ list_del(&req->list);
+ continue;
+ }
+ if (list_empty(&async_list->list))
+ break;
+
+ req = NULL;
+ spin_lock(&async_list->lock);
+ if (list_empty(&async_list->list)) {
+ spin_unlock(&async_list->lock);
+ break;
+ }
+ list_splice_init(&async_list->list, &req_list);
+ spin_unlock(&async_list->lock);
+
+ req = list_first_entry(&req_list, struct io_kiocb, list);
+ list_del(&req->list);
+ } while (req);
+
/*
- * If we're doing IO to fixed buffers, we don't need to get/set
- * user context
+ * Rare case of racing with a submitter. If we find the count has
+ * dropped to zero AND we have pending work items, then restart
+ * the processing. This is a tiny race window.
*/
- needs_user = io_sqe_needs_user(s->sqe);
- if (needs_user) {
- if (!mmget_not_zero(ctx->sqo_mm)) {
- ret = -EFAULT;
- goto err;
+ ret = atomic_dec_return(&async_list->cnt);
+ while (!ret && !list_empty(&async_list->list)) {
+ spin_lock(&async_list->lock);
+ atomic_inc(&async_list->cnt);
+ list_splice_init(&async_list->list, &req_list);
+ spin_unlock(&async_list->lock);
+
+ if (!list_empty(&req_list)) {
+ req = list_first_entry(&req_list, struct io_kiocb,
+ list);
+ list_del(&req->list);
+ goto restart;
}
- use_mm(ctx->sqo_mm);
- old_fs = get_fs();
- set_fs(USER_DS);
+ ret = atomic_dec_return(&async_list->cnt);
}
- ret = __io_submit_sqe(ctx, req, s, false, NULL);
-
- if (needs_user) {
+ if (cur_mm) {
set_fs(old_fs);
- unuse_mm(ctx->sqo_mm);
- mmput(ctx->sqo_mm);
- }
-err:
- if (ret) {
- io_cqring_add_event(ctx, user_data, ret, 0);
- io_free_req(req);
+ unuse_mm(cur_mm);
+ mmput(cur_mm);
}
current->files = old_files;
}
+/*
+ * See if we can piggy back onto previously submitted work, that is still
+ * running. We currently only allow this if the new request is sequential
+ * to the previous one we punted.
+ */
+static bool io_add_to_prev_work(struct async_list *list, struct io_kiocb *req)
+{
+ bool ret = false;
+
+ if (!list)
+ return false;
+ if (!(req->flags & REQ_F_SEQ_PREV))
+ return false;
+ if (!atomic_read(&list->cnt))
+ return false;
+
+ ret = true;
+ spin_lock(&list->lock);
+ list_add_tail(&req->list, &list->list);
+ if (!atomic_read(&list->cnt)) {
+ list_del_init(&req->list);
+ ret = false;
+ }
+ spin_unlock(&list->lock);
+ return ret;
+}
+
static int io_submit_sqe(struct io_ring_ctx *ctx, struct sqe_submit *s,
struct io_submit_state *state)
{
@@ -1279,9 +1429,16 @@ static int io_submit_sqe(struct io_ring_ctx *ctx, struct sqe_submit *s,
ret = __io_submit_sqe(ctx, req, s, true, state);
if (ret == -EAGAIN) {
+ struct async_list *list;
+
+ list = io_async_list_from_sqe(ctx, s->sqe);
memcpy(&req->submit, s, sizeof(*s));
- INIT_WORK(&req->work, io_sq_wq_submit_work);
- queue_work(ctx->sqo_wq, &req->work);
+ if (!io_add_to_prev_work(list, req)) {
+ if (list)
+ atomic_inc(&list->cnt);
+ INIT_WORK(&req->work, io_sq_wq_submit_work);
+ queue_work(ctx->sqo_wq, &req->work);
+ }
ret = 0;
}
if (ret)
--
2.17.1
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply related
* [PATCH 18/18] io_uring: add io_uring_event cache hit information
From: Jens Axboe @ 2019-01-28 21:35 UTC (permalink / raw)
To: linux-aio, linux-block, linux-man, linux-api; +Cc: hch, jmoyer, avi, Jens Axboe
In-Reply-To: <20190128213538.13486-1-axboe@kernel.dk>
Add hint on whether a read was served out of the page cache, or if it
hit media. This is useful for buffered async IO, O_DIRECT reads would
never have this set (for obvious reasons).
If the read hit page cache, cqe->flags will have IOCQE_FLAG_CACHEHIT
set.
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
fs/io_uring.c | 7 ++++++-
include/uapi/linux/io_uring.h | 5 +++++
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 0240e0a04f6a..9b4b4f9d06a5 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -558,11 +558,16 @@ static void io_fput(struct io_kiocb *req)
static void io_complete_rw(struct kiocb *kiocb, long res, long res2)
{
struct io_kiocb *req = container_of(kiocb, struct io_kiocb, rw);
+ unsigned ev_flags = 0;
kiocb_end_write(kiocb);
io_fput(req);
- io_cqring_add_event(req->ctx, req->user_data, res, 0);
+
+ if (res > 0 && (req->flags & REQ_F_FORCE_NONBLOCK))
+ ev_flags = IOCQE_FLAG_CACHEHIT;
+
+ io_cqring_add_event(req->ctx, req->user_data, res, ev_flags);
io_free_req(req);
}
diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
index 39d3d3336dce..589b6402081d 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -71,6 +71,11 @@ struct io_uring_cqe {
__u32 flags;
};
+/*
+ * io_uring_event->flags
+ */
+#define IOCQE_FLAG_CACHEHIT (1 << 0) /* IO did not hit media */
+
/*
* Magic offsets for the application to mmap the data it needs
*/
--
2.17.1
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply related
* Re: [PATCH 05/18] Add io_uring IO interface
From: Jeff Moyer @ 2019-01-28 21:53 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-aio, linux-block, linux-man, linux-api, hch, avi
In-Reply-To: <20190128213538.13486-6-axboe@kernel.dk>
Jens Axboe <axboe@kernel.dk> writes:
> +static int __io_uring_enter(struct io_ring_ctx *ctx, unsigned to_submit,
> + unsigned min_complete, unsigned flags,
> + const sigset_t __user *sig, size_t sigsz)
> +{
> + int submitted, ret;
> +
> + submitted = ret = 0;
> + if (to_submit) {
> + submitted = io_ring_submit(ctx, to_submit);
> + if (submitted < 0)
> + return submitted;
> + }
> + if (flags & IORING_ENTER_GETEVENTS) {
Do we want to disallow any unsupported flags?
-Jeff
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Jens Axboe @ 2019-01-28 21:56 UTC (permalink / raw)
To: Jeff Moyer; +Cc: linux-aio, linux-block, linux-man, linux-api, hch, avi
In-Reply-To: <x498sz4wisd.fsf@segfault.boston.devel.redhat.com>
On 1/28/19 2:53 PM, Jeff Moyer wrote:
> Jens Axboe <axboe@kernel.dk> writes:
>
>> +static int __io_uring_enter(struct io_ring_ctx *ctx, unsigned to_submit,
>> + unsigned min_complete, unsigned flags,
>> + const sigset_t __user *sig, size_t sigsz)
>> +{
>> + int submitted, ret;
>> +
>> + submitted = ret = 0;
>> + if (to_submit) {
>> + submitted = io_ring_submit(ctx, to_submit);
>> + if (submitted < 0)
>> + return submitted;
>> + }
>> + if (flags & IORING_ENTER_GETEVENTS) {
>
> Do we want to disallow any unsupported flags?
Yes good point, we do that in other spots. Fixed.
--
Jens Axboe
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 09/18] io_uring: use fget/fput_many() for file references
From: Jann Horn @ 2019-01-28 21:56 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-aio, linux-block, linux-man, Linux API, hch, jmoyer,
Avi Kivity
In-Reply-To: <20190128213538.13486-10-axboe@kernel.dk>
On Mon, Jan 28, 2019 at 10:36 PM Jens Axboe <axboe@kernel.dk> wrote:
> Add a separate io_submit_state structure, to cache some of the things
> we need for IO submission.
>
> One such example is file reference batching. io_submit_state. We get as
> many references as the number of sqes we are submitting, and drop
> unused ones if we end up switching files. The assumption here is that
> we're usually only dealing with one fd, and if there are multiple,
> hopefuly they are at least somewhat ordered. Could trivially be extended
> to cover multiple fds, if needed.
>
> On the completion side we do the same thing, except this is trivially
> done just locally in io_iopoll_reap().
>
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
[...]
> +/*
> + * Get as many references to a file as we have IOs left in this submission,
> + * assuming most submissions are for one file, or at least that each file
> + * has more than one submission.
> + */
> +static struct file *io_file_get(struct io_submit_state *state, int fd)
> +{
> + if (!state)
> + return fget(fd);
> +
> + if (state->file) {
> + if (state->fd == fd) {
> + state->used_refs++;
> + state->ios_left--;
> + return state->file;
> + }
> + io_file_put(state, NULL);
> + }
> + state->file = fget_many(fd, state->ios_left);
> + if (!state->file)
> + return NULL;
This looks wrong.
Looking at "[PATCH 05/18] Add io_uring IO interface", as far as I can
tell, io_ring_submit() is called via __io_uring_enter() <-
sys_io_uring_enter() with an unchecked argument "unsigned int
to_submit" that is then, in this patch, stored in state->ios_left and
then used here. On a 32-bit platform, file->f_count is only 32 bits
wide, so I think you can then trivially overflow the reference count,
leading to use-after-free. Am I missing something?
> + state->fd = fd;
> + state->has_refs = state->ios_left;
> + state->used_refs = 1;
> + state->ios_left--;
> + return state->file;
> +}
[...]
> +static void io_submit_state_start(struct io_submit_state *state,
> + struct io_ring_ctx *ctx, unsigned max_ios)
> +{
> + blk_start_plug(&state->plug);
> + state->file = NULL;
> + state->ios_left = max_ios;
> +}
> +
> static void io_commit_sqring(struct io_ring_ctx *ctx)
> {
> struct io_sq_ring *ring = ctx->sq_ring;
> @@ -879,11 +974,13 @@ static bool io_get_sqring(struct io_ring_ctx *ctx, struct sqe_submit *s)
>
> static int io_ring_submit(struct io_ring_ctx *ctx, unsigned int to_submit)
> {
> + struct io_submit_state state, *statep = NULL;
> int i, ret = 0, submit = 0;
> - struct blk_plug plug;
>
> - if (to_submit > IO_PLUG_THRESHOLD)
> - blk_start_plug(&plug);
> + if (to_submit > IO_PLUG_THRESHOLD) {
> + io_submit_state_start(&state, ctx, to_submit);
> + statep = &state;
> + }
[...]
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 09/18] io_uring: use fget/fput_many() for file references
From: Jens Axboe @ 2019-01-28 22:03 UTC (permalink / raw)
To: Jann Horn
Cc: linux-aio, linux-block, linux-man, Linux API, hch, jmoyer,
Avi Kivity
In-Reply-To: <CAG48ez0WzBEG5w5faFZ_DjU_62aTT=iBEPHEdYZuyq-LGRJtPw@mail.gmail.com>
On 1/28/19 2:56 PM, Jann Horn wrote:
> On Mon, Jan 28, 2019 at 10:36 PM Jens Axboe <axboe@kernel.dk> wrote:
>> Add a separate io_submit_state structure, to cache some of the things
>> we need for IO submission.
>>
>> One such example is file reference batching. io_submit_state. We get as
>> many references as the number of sqes we are submitting, and drop
>> unused ones if we end up switching files. The assumption here is that
>> we're usually only dealing with one fd, and if there are multiple,
>> hopefuly they are at least somewhat ordered. Could trivially be extended
>> to cover multiple fds, if needed.
>>
>> On the completion side we do the same thing, except this is trivially
>> done just locally in io_iopoll_reap().
>>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>> ---
> [...]
>> +/*
>> + * Get as many references to a file as we have IOs left in this submission,
>> + * assuming most submissions are for one file, or at least that each file
>> + * has more than one submission.
>> + */
>> +static struct file *io_file_get(struct io_submit_state *state, int fd)
>> +{
>> + if (!state)
>> + return fget(fd);
>> +
>> + if (state->file) {
>> + if (state->fd == fd) {
>> + state->used_refs++;
>> + state->ios_left--;
>> + return state->file;
>> + }
>> + io_file_put(state, NULL);
>> + }
>> + state->file = fget_many(fd, state->ios_left);
>> + if (!state->file)
>> + return NULL;
>
> This looks wrong.
>
> Looking at "[PATCH 05/18] Add io_uring IO interface", as far as I can
> tell, io_ring_submit() is called via __io_uring_enter() <-
> sys_io_uring_enter() with an unchecked argument "unsigned int
> to_submit" that is then, in this patch, stored in state->ios_left and
> then used here. On a 32-bit platform, file->f_count is only 32 bits
> wide, so I think you can then trivially overflow the reference count,
> leading to use-after-free. Am I missing something?
No, that is possible. Since we cap the SQ ring entries at 4k, I think
we should just validate to_submit/min_complete against those numbers.
That would also solve this overflow.
--
Jens Axboe
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* [PATCH] Fix: membarrier: racy access to p->mm in membarrier_global_expedited()
From: Mathieu Desnoyers @ 2019-01-28 22:07 UTC (permalink / raw)
To: Ingo Molnar, Peter Zijlstra
Cc: linux-kernel, linux-api, Mathieu Desnoyers, Jann Horn,
Thomas Gleixner, Andrea Parri, Andy Lutomirski, Avi Kivity,
Benjamin Herrenschmidt, Boqun Feng, Dave Watson, David Sehr,
H . Peter Anvin, Linus Torvalds, Maged Michael, Michael Ellerman,
Paul E . McKenney, Paul Mackerras, Russell King, Will Deacon,
stable@
Jann Horn identified a racy access to p->mm in the global expedited
command of the membarrier system call.
The suggested fix is to hold the task_lock() around the accesses to
p->mm and to the mm_struct membarrier_state field to guarantee the
existence of the mm_struct.
Link: https://lore.kernel.org/lkml/CAG48ez2G8ctF8dHS42TF37pThfr3y0RNOOYTmxvACm4u8Yu3cw@mail.gmail.com
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Tested-by: Jann Horn <jannh@google.com>
CC: Jann Horn <jannh@google.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Peter Zijlstra (Intel) <peterz@infradead.org>
CC: Ingo Molnar <mingo@kernel.org>
CC: Andrea Parri <parri.andrea@gmail.com>
CC: Andy Lutomirski <luto@kernel.org>
CC: Avi Kivity <avi@scylladb.com>
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CC: Boqun Feng <boqun.feng@gmail.com>
CC: Dave Watson <davejwatson@fb.com>
CC: David Sehr <sehr@google.com>
CC: H. Peter Anvin <hpa@zytor.com>
CC: Linus Torvalds <torvalds@linux-foundation.org>
CC: Maged Michael <maged.michael@gmail.com>
CC: Michael Ellerman <mpe@ellerman.id.au>
CC: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
CC: Paul Mackerras <paulus@samba.org>
CC: Russell King <linux@armlinux.org.uk>
CC: Will Deacon <will.deacon@arm.com>
CC: stable@vger.kernel.org # v4.16+
CC: linux-api@vger.kernel.org
---
kernel/sched/membarrier.c | 27 +++++++++++++++++++++------
1 file changed, 21 insertions(+), 6 deletions(-)
diff --git a/kernel/sched/membarrier.c b/kernel/sched/membarrier.c
index 76e0eaf4654e..305fdcc4c5f7 100644
--- a/kernel/sched/membarrier.c
+++ b/kernel/sched/membarrier.c
@@ -81,12 +81,27 @@ static int membarrier_global_expedited(void)
rcu_read_lock();
p = task_rcu_dereference(&cpu_rq(cpu)->curr);
- if (p && p->mm && (atomic_read(&p->mm->membarrier_state) &
- MEMBARRIER_STATE_GLOBAL_EXPEDITED)) {
- if (!fallback)
- __cpumask_set_cpu(cpu, tmpmask);
- else
- smp_call_function_single(cpu, ipi_mb, NULL, 1);
+ /*
+ * Skip this CPU if the runqueue's current task is NULL or if
+ * it is a kernel thread.
+ */
+ if (p && READ_ONCE(p->mm)) {
+ bool mm_match;
+
+ /*
+ * Read p->mm and access membarrier_state while holding
+ * the task lock to ensure existence of mm.
+ */
+ task_lock(p);
+ mm_match = p->mm && (atomic_read(&p->mm->membarrier_state) &
+ MEMBARRIER_STATE_GLOBAL_EXPEDITED);
+ task_unlock(p);
+ if (mm_match) {
+ if (!fallback)
+ __cpumask_set_cpu(cpu, tmpmask);
+ else
+ smp_call_function_single(cpu, ipi_mb, NULL, 1);
+ }
}
rcu_read_unlock();
}
--
2.17.1
^ permalink raw reply related
* Re: [PATCH 05/18] Add io_uring IO interface
From: Jann Horn @ 2019-01-28 22:32 UTC (permalink / raw)
To: Jens Axboe, Al Viro
Cc: linux-aio, linux-block, linux-man, Linux API, hch, jmoyer,
Avi Kivity
In-Reply-To: <20190128213538.13486-6-axboe@kernel.dk>
On Mon, Jan 28, 2019 at 10:35 PM Jens Axboe <axboe@kernel.dk> wrote:
> The submission queue (SQ) and completion queue (CQ) rings are shared
> between the application and the kernel. This eliminates the need to
> copy data back and forth to submit and complete IO.
>
> IO submissions use the io_uring_sqe data structure, and completions
> are generated in the form of io_uring_sqe data structures. The SQ
> ring is an index into the io_uring_sqe array, which makes it possible
> to submit a batch of IOs without them being contiguous in the ring.
> The CQ ring is always contiguous, as completion events are inherently
> unordered, and hence any io_uring_cqe entry can point back to an
> arbitrary submission.
>
> Two new system calls are added for this:
>
> io_uring_setup(entries, params)
> Sets up a context for doing async IO. On success, returns a file
> descriptor that the application can mmap to gain access to the
> SQ ring, CQ ring, and io_uring_sqes.
>
> io_uring_enter(fd, to_submit, min_complete, flags, sigset, sigsetsize)
> Initiates IO against the rings mapped to this fd, or waits for
> them to complete, or both. The behavior is controlled by the
> parameters passed in. If 'to_submit' is non-zero, then we'll
> try and submit new IO. If IORING_ENTER_GETEVENTS is set, the
> kernel will wait for 'min_complete' events, if they aren't
> already available. It's valid to set IORING_ENTER_GETEVENTS
> and 'min_complete' == 0 at the same time, this allows the
> kernel to return already completed events without waiting
> for them. This is useful only for polling, as for IRQ
> driven IO, the application can just check the CQ ring
> without entering the kernel.
>
> With this setup, it's possible to do async IO with a single system
> call. Future developments will enable polled IO with this interface,
> and polled submission as well. The latter will enable an application
> to do IO without doing ANY system calls at all.
>
> For IRQ driven IO, an application only needs to enter the kernel for
> completions if it wants to wait for them to occur.
>
> Each io_uring is backed by a workqueue, to support buffered async IO
> as well. We will only punt to an async context if the command would
> need to wait for IO on the device side. Any data that can be accessed
> directly in the page cache is done inline. This avoids the slowness
> issue of usual threadpools, since cached data is accessed as quickly
> as a sync interface.
>
> Sample application: http://git.kernel.dk/cgit/fio/plain/t/io_uring.c
[...]
> +static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe,
> + bool force_nonblock)
> +{
> + struct kiocb *kiocb = &req->rw;
> + int ret;
> +
> + kiocb->ki_filp = fget(sqe->fd);
> + if (unlikely(!kiocb->ki_filp))
> + return -EBADF;
> + kiocb->ki_pos = sqe->off;
> + kiocb->ki_flags = iocb_flags(kiocb->ki_filp);
> + kiocb->ki_hint = ki_hint_validate(file_write_hint(kiocb->ki_filp));
> + if (sqe->ioprio) {
> + ret = ioprio_check_cap(sqe->ioprio);
> + if (ret)
> + goto out_fput;
> +
> + kiocb->ki_ioprio = sqe->ioprio;
> + } else
> + kiocb->ki_ioprio = get_current_ioprio();
> +
> + ret = kiocb_set_rw_flags(kiocb, sqe->rw_flags);
> + if (unlikely(ret))
> + goto out_fput;
> + if (force_nonblock) {
> + kiocb->ki_flags |= IOCB_NOWAIT;
> + req->flags |= REQ_F_FORCE_NONBLOCK;
> + }
> + if (kiocb->ki_flags & IOCB_HIPRI) {
> + ret = -EINVAL;
> + goto out_fput;
> + }
> +
> + kiocb->ki_complete = io_complete_rw;
> + return 0;
> +out_fput:
> + fput(kiocb->ki_filp);
> + return ret;
> +}
[...]
> +static ssize_t io_read(struct io_kiocb *req, const struct io_uring_sqe *sqe,
> + bool force_nonblock)
> +{
> + struct iovec inline_vecs[UIO_FASTIOV], *iovec = inline_vecs;
> + struct kiocb *kiocb = &req->rw;
> + struct iov_iter iter;
> + struct file *file;
> + ssize_t ret;
> +
> + ret = io_prep_rw(req, sqe, force_nonblock);
> + if (ret)
> + return ret;
> + file = kiocb->ki_filp;
> +
> + ret = -EBADF;
> + if (unlikely(!(file->f_mode & FMODE_READ)))
> + goto out_fput;
> + ret = -EINVAL;
> + if (unlikely(!file->f_op->read_iter))
> + goto out_fput;
> +
> + ret = io_import_iovec(req->ctx, READ, sqe, &iovec, &iter);
> + if (ret)
> + goto out_fput;
> +
> + ret = rw_verify_area(READ, file, &kiocb->ki_pos, iov_iter_count(&iter));
> + if (!ret) {
> + ssize_t ret2;
> +
> + /* Catch -EAGAIN return for forced non-blocking submission */
> + ret2 = call_read_iter(file, kiocb, &iter);
> + if (!force_nonblock || ret2 != -EAGAIN)
> + io_rw_done(kiocb, ret2);
> + else
> + ret = -EAGAIN;
> + }
> + kfree(iovec);
> +out_fput:
> + if (unlikely(ret))
> + fput(file);
> + return ret;
> +}
[...]
> +static int __io_submit_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
> + struct sqe_submit *s, bool force_nonblock)
> +{
> + const struct io_uring_sqe *sqe = s->sqe;
> + ssize_t ret;
> +
> + if (unlikely(s->index >= ctx->sq_entries))
> + return -EINVAL;
> + req->user_data = sqe->user_data;
> +
> + ret = -EINVAL;
> + switch (sqe->opcode) {
> + case IORING_OP_NOP:
> + ret = io_nop(req, sqe);
> + break;
> + case IORING_OP_READV:
> + ret = io_read(req, sqe, force_nonblock);
> + break;
> + case IORING_OP_WRITEV:
> + ret = io_write(req, sqe, force_nonblock);
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> + return ret;
> +}
> +
> +static void io_sq_wq_submit_work(struct work_struct *work)
> +{
> + struct io_kiocb *req = container_of(work, struct io_kiocb, work);
> + struct sqe_submit *s = &req->submit;
> + u64 user_data = s->sqe->user_data;
> + struct io_ring_ctx *ctx = req->ctx;
> + mm_segment_t old_fs = get_fs();
> + struct files_struct *old_files;
> + int ret;
> +
> + /* Ensure we clear previously set forced non-block flag */
> + req->flags &= ~REQ_F_FORCE_NONBLOCK;
> +
> + old_files = current->files;
> + current->files = ctx->sqo_files;
I think you're not supposed to twiddle with current->files without
holding task_lock(current).
> + if (!mmget_not_zero(ctx->sqo_mm)) {
> + ret = -EFAULT;
> + goto err;
> + }
> +
> + use_mm(ctx->sqo_mm);
> + set_fs(USER_DS);
> +
> + ret = __io_submit_sqe(ctx, req, s, false);
> +
> + set_fs(old_fs);
> + unuse_mm(ctx->sqo_mm);
> + mmput(ctx->sqo_mm);
> +err:
> + if (ret) {
> + io_cqring_add_event(ctx, user_data, ret, 0);
> + io_free_req(req);
> + }
> + current->files = old_files;
> +}
[...]
> +static int io_sq_offload_start(struct io_ring_ctx *ctx)
> +{
> + int ret;
> +
> + ctx->sqo_mm = current->mm;
What keeps this thing alive?
> + /*
> + * This is safe since 'current' has the fd installed, and if that gets
> + * closed on exit, then fops->release() is invoked which waits for the
> + * async contexts to flush and exit before exiting.
> + */
> + ret = -EBADF;
> + ctx->sqo_files = current->files;
> + if (!ctx->sqo_files)
> + goto err;
That's gnarly. Adding Al Viro to the thread.
I think you misunderstand the semantics of f_op->release. The ->flush
handler is invoked whenever a file descriptor is closed through
filp_close() (via deletion of the files_struct, sys_close(),
sys_dup2(), ...), so if you had used that one, _maybe_ this would
work. But the ->release handler only runs when the _last_ reference to
a struct file has been dropped - so you can, for example, fork() a
child, then exit() in the parent, and the ->release handler isn't
invoked. So I don't see how this can work.
But even if you had abused ->flush for this instead: close_files()
currently has a comment in it that claims that "this is the last
reference to the files structure"; this change would make that claim
untrue.
> + /* Do QD, or 2 * CPUS, whatever is smallest */
> + ctx->sqo_wq = alloc_workqueue("io_ring-wq", WQ_UNBOUND | WQ_FREEZABLE,
> + min(ctx->sq_entries - 1, 2 * num_online_cpus()));
> + if (!ctx->sqo_wq) {
> + ret = -ENOMEM;
> + goto err;
> + }
> +
> + return 0;
> +err:
> + if (ctx->sqo_files)
> + ctx->sqo_files = NULL;
> + ctx->sqo_mm = NULL;
> + return ret;
> +}
[...]
> +static const struct file_operations io_uring_fops = {
> + .release = io_uring_release,
> + .mmap = io_uring_mmap,
> + .poll = io_uring_poll,
> + .fasync = io_uring_fasync,
> +};
[...]
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH] Fix: membarrier: racy access to p->mm in membarrier_global_expedited()
From: Paul E. McKenney @ 2019-01-28 22:39 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Ingo Molnar, Peter Zijlstra, linux-kernel, linux-api, Jann Horn,
Thomas Gleixner, Andrea Parri, Andy Lutomirski, Avi Kivity,
Benjamin Herrenschmidt, Boqun Feng, Dave Watson, David Sehr,
H . Peter Anvin, Linus Torvalds, Maged Michael, Michael Ellerman,
Paul Mackerras, Russell King, Will Deacon, stable
In-Reply-To: <20190128220707.30774-1-mathieu.desnoyers@efficios.com>
On Mon, Jan 28, 2019 at 05:07:07PM -0500, Mathieu Desnoyers wrote:
> Jann Horn identified a racy access to p->mm in the global expedited
> command of the membarrier system call.
>
> The suggested fix is to hold the task_lock() around the accesses to
> p->mm and to the mm_struct membarrier_state field to guarantee the
> existence of the mm_struct.
>
> Link: https://lore.kernel.org/lkml/CAG48ez2G8ctF8dHS42TF37pThfr3y0RNOOYTmxvACm4u8Yu3cw@mail.gmail.com
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> Tested-by: Jann Horn <jannh@google.com>
> CC: Jann Horn <jannh@google.com>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Peter Zijlstra (Intel) <peterz@infradead.org>
> CC: Ingo Molnar <mingo@kernel.org>
> CC: Andrea Parri <parri.andrea@gmail.com>
> CC: Andy Lutomirski <luto@kernel.org>
> CC: Avi Kivity <avi@scylladb.com>
> CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> CC: Boqun Feng <boqun.feng@gmail.com>
> CC: Dave Watson <davejwatson@fb.com>
> CC: David Sehr <sehr@google.com>
> CC: H. Peter Anvin <hpa@zytor.com>
> CC: Linus Torvalds <torvalds@linux-foundation.org>
> CC: Maged Michael <maged.michael@gmail.com>
> CC: Michael Ellerman <mpe@ellerman.id.au>
> CC: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> CC: Paul Mackerras <paulus@samba.org>
> CC: Russell King <linux@armlinux.org.uk>
> CC: Will Deacon <will.deacon@arm.com>
> CC: stable@vger.kernel.org # v4.16+
> CC: linux-api@vger.kernel.org
> ---
> kernel/sched/membarrier.c | 27 +++++++++++++++++++++------
> 1 file changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/sched/membarrier.c b/kernel/sched/membarrier.c
> index 76e0eaf4654e..305fdcc4c5f7 100644
> --- a/kernel/sched/membarrier.c
> +++ b/kernel/sched/membarrier.c
> @@ -81,12 +81,27 @@ static int membarrier_global_expedited(void)
>
> rcu_read_lock();
> p = task_rcu_dereference(&cpu_rq(cpu)->curr);
> - if (p && p->mm && (atomic_read(&p->mm->membarrier_state) &
> - MEMBARRIER_STATE_GLOBAL_EXPEDITED)) {
> - if (!fallback)
> - __cpumask_set_cpu(cpu, tmpmask);
> - else
> - smp_call_function_single(cpu, ipi_mb, NULL, 1);
> + /*
> + * Skip this CPU if the runqueue's current task is NULL or if
> + * it is a kernel thread.
> + */
> + if (p && READ_ONCE(p->mm)) {
> + bool mm_match;
> +
> + /*
> + * Read p->mm and access membarrier_state while holding
> + * the task lock to ensure existence of mm.
> + */
> + task_lock(p);
> + mm_match = p->mm && (atomic_read(&p->mm->membarrier_state) &
Are we guaranteed that this p->mm will be the same as the one loaded via
READ_ONCE() above? Either way, wouldn't it be better to READ_ONCE() it a
single time and use the same value everywhere?
Thanx, Paul
> + MEMBARRIER_STATE_GLOBAL_EXPEDITED);
> + task_unlock(p);
> + if (mm_match) {
> + if (!fallback)
> + __cpumask_set_cpu(cpu, tmpmask);
> + else
> + smp_call_function_single(cpu, ipi_mb, NULL, 1);
> + }
> }
> rcu_read_unlock();
> }
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH] Fix: membarrier: racy access to p->mm in membarrier_global_expedited()
From: Jann Horn @ 2019-01-28 22:45 UTC (permalink / raw)
To: Paul E. McKenney
Cc: Mathieu Desnoyers, Ingo Molnar, Peter Zijlstra, kernel list,
Linux API, Thomas Gleixner, Andrea Parri, Andy Lutomirski,
Avi Kivity, Benjamin Herrenschmidt, Boqun Feng, Dave Watson,
David Sehr, H . Peter Anvin, Linus Torvalds, Maged Michael,
Michael Ellerman, Paul Mackerras, Russell King,
Will Deacon <will>
In-Reply-To: <20190128223948.GD4240@linux.ibm.com>
On Mon, Jan 28, 2019 at 11:39 PM Paul E. McKenney <paulmck@linux.ibm.com> wrote:
> On Mon, Jan 28, 2019 at 05:07:07PM -0500, Mathieu Desnoyers wrote:
> > Jann Horn identified a racy access to p->mm in the global expedited
> > command of the membarrier system call.
> >
> > The suggested fix is to hold the task_lock() around the accesses to
> > p->mm and to the mm_struct membarrier_state field to guarantee the
> > existence of the mm_struct.
> >
> > Link: https://lore.kernel.org/lkml/CAG48ez2G8ctF8dHS42TF37pThfr3y0RNOOYTmxvACm4u8Yu3cw@mail.gmail.com
> > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
[...]
> > --- a/kernel/sched/membarrier.c
> > +++ b/kernel/sched/membarrier.c
> > @@ -81,12 +81,27 @@ static int membarrier_global_expedited(void)
> >
> > rcu_read_lock();
> > p = task_rcu_dereference(&cpu_rq(cpu)->curr);
> > - if (p && p->mm && (atomic_read(&p->mm->membarrier_state) &
> > - MEMBARRIER_STATE_GLOBAL_EXPEDITED)) {
> > - if (!fallback)
> > - __cpumask_set_cpu(cpu, tmpmask);
> > - else
> > - smp_call_function_single(cpu, ipi_mb, NULL, 1);
> > + /*
> > + * Skip this CPU if the runqueue's current task is NULL or if
> > + * it is a kernel thread.
> > + */
> > + if (p && READ_ONCE(p->mm)) {
> > + bool mm_match;
> > +
> > + /*
> > + * Read p->mm and access membarrier_state while holding
> > + * the task lock to ensure existence of mm.
> > + */
> > + task_lock(p);
> > + mm_match = p->mm && (atomic_read(&p->mm->membarrier_state) &
>
> Are we guaranteed that this p->mm will be the same as the one loaded via
> READ_ONCE() above?
No; the way I read it, that's just an optimization and has no effect
on correctness.
> Either way, wouldn't it be better to READ_ONCE() it a
> single time and use the same value everywhere?
No; the first READ_ONCE() returns a pointer that you can't access
because it wasn't read under a lock. You can only use it for a NULL
check.
^ permalink raw reply
* Re: [PATCH] Fix: membarrier: racy access to p->mm in membarrier_global_expedited()
From: Mathieu Desnoyers @ 2019-01-28 22:46 UTC (permalink / raw)
To: paulmck
Cc: Ingo Molnar, Peter Zijlstra, linux-kernel, linux-api, Jann Horn,
Thomas Gleixner, Andrea Parri, Andy Lutomirski, Avi Kivity,
Benjamin Herrenschmidt, Boqun Feng, Dave Watson, David Sehr,
H. Peter Anvin, Linus Torvalds, maged michael, Michael Ellerman,
Paul Mackerras, Russell King, ARM Linux, Will Deacon
In-Reply-To: <20190128223948.GD4240@linux.ibm.com>
----- On Jan 28, 2019, at 5:39 PM, paulmck paulmck@linux.ibm.com wrote:
> On Mon, Jan 28, 2019 at 05:07:07PM -0500, Mathieu Desnoyers wrote:
>> Jann Horn identified a racy access to p->mm in the global expedited
>> command of the membarrier system call.
>>
>> The suggested fix is to hold the task_lock() around the accesses to
>> p->mm and to the mm_struct membarrier_state field to guarantee the
>> existence of the mm_struct.
>>
>> Link:
>> https://lore.kernel.org/lkml/CAG48ez2G8ctF8dHS42TF37pThfr3y0RNOOYTmxvACm4u8Yu3cw@mail.gmail.com
>> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>> Tested-by: Jann Horn <jannh@google.com>
>> CC: Jann Horn <jannh@google.com>
>> CC: Thomas Gleixner <tglx@linutronix.de>
>> CC: Peter Zijlstra (Intel) <peterz@infradead.org>
>> CC: Ingo Molnar <mingo@kernel.org>
>> CC: Andrea Parri <parri.andrea@gmail.com>
>> CC: Andy Lutomirski <luto@kernel.org>
>> CC: Avi Kivity <avi@scylladb.com>
>> CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> CC: Boqun Feng <boqun.feng@gmail.com>
>> CC: Dave Watson <davejwatson@fb.com>
>> CC: David Sehr <sehr@google.com>
>> CC: H. Peter Anvin <hpa@zytor.com>
>> CC: Linus Torvalds <torvalds@linux-foundation.org>
>> CC: Maged Michael <maged.michael@gmail.com>
>> CC: Michael Ellerman <mpe@ellerman.id.au>
>> CC: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
>> CC: Paul Mackerras <paulus@samba.org>
>> CC: Russell King <linux@armlinux.org.uk>
>> CC: Will Deacon <will.deacon@arm.com>
>> CC: stable@vger.kernel.org # v4.16+
>> CC: linux-api@vger.kernel.org
>> ---
>> kernel/sched/membarrier.c | 27 +++++++++++++++++++++------
>> 1 file changed, 21 insertions(+), 6 deletions(-)
>>
>> diff --git a/kernel/sched/membarrier.c b/kernel/sched/membarrier.c
>> index 76e0eaf4654e..305fdcc4c5f7 100644
>> --- a/kernel/sched/membarrier.c
>> +++ b/kernel/sched/membarrier.c
>> @@ -81,12 +81,27 @@ static int membarrier_global_expedited(void)
>>
>> rcu_read_lock();
>> p = task_rcu_dereference(&cpu_rq(cpu)->curr);
>> - if (p && p->mm && (atomic_read(&p->mm->membarrier_state) &
>> - MEMBARRIER_STATE_GLOBAL_EXPEDITED)) {
>> - if (!fallback)
>> - __cpumask_set_cpu(cpu, tmpmask);
>> - else
>> - smp_call_function_single(cpu, ipi_mb, NULL, 1);
>> + /*
>> + * Skip this CPU if the runqueue's current task is NULL or if
>> + * it is a kernel thread.
>> + */
>> + if (p && READ_ONCE(p->mm)) {
>> + bool mm_match;
>> +
>> + /*
>> + * Read p->mm and access membarrier_state while holding
>> + * the task lock to ensure existence of mm.
>> + */
>> + task_lock(p);
>> + mm_match = p->mm && (atomic_read(&p->mm->membarrier_state) &
>
> Are we guaranteed that this p->mm will be the same as the one loaded via
> READ_ONCE() above? Either way, wouldn't it be better to READ_ONCE() it a
> single time and use the same value everywhere?
The first "READ_ONCE()" above is _outside_ of the task_lock() critical section.
Those two accesses _can_ load two different pointers, and this is why we
need to re-read the p->mm pointer within the task_lock() critical section to
ensure existence of the mm_struct that we use.
If we move the READ_ONCE() into the task_lock(), we need to uselessly
take a lock before we can skip kernel threads.
If we lead the READ_ONCE() outside the task_lock(), then p->mm can be updated
between the READ_ONCE() and reference to the mm_struct content within the
task_lock(), which is racy and does not guarantee its existence.
Or am I missing your point ?
Thanks,
Mathieu
>
> Thanx, Paul
>
>> + MEMBARRIER_STATE_GLOBAL_EXPEDITED);
>> + task_unlock(p);
>> + if (mm_match) {
>> + if (!fallback)
>> + __cpumask_set_cpu(cpu, tmpmask);
>> + else
>> + smp_call_function_single(cpu, ipi_mb, NULL, 1);
>> + }
>> }
>> rcu_read_unlock();
>> }
>> --
>> 2.17.1
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [PATCH] Fix: membarrier: racy access to p->mm in membarrier_global_expedited()
From: Paul E. McKenney @ 2019-01-28 23:22 UTC (permalink / raw)
To: Jann Horn
Cc: Mathieu Desnoyers, Ingo Molnar, Peter Zijlstra, kernel list,
Linux API, Thomas Gleixner, Andrea Parri, Andy Lutomirski,
Avi Kivity, Benjamin Herrenschmidt, Boqun Feng, Dave Watson,
David Sehr, H . Peter Anvin, Linus Torvalds, Maged Michael,
Michael Ellerman, Paul Mackerras, Russell King,
Will Deacon <will>
In-Reply-To: <CAG48ez1+Vx7=ZqeLJ0waceSneCQhEcoS=bv9ggb3Dbveb+W6AQ@mail.gmail.com>
On Mon, Jan 28, 2019 at 11:45:32PM +0100, Jann Horn wrote:
> On Mon, Jan 28, 2019 at 11:39 PM Paul E. McKenney <paulmck@linux.ibm.com> wrote:
> > On Mon, Jan 28, 2019 at 05:07:07PM -0500, Mathieu Desnoyers wrote:
> > > Jann Horn identified a racy access to p->mm in the global expedited
> > > command of the membarrier system call.
> > >
> > > The suggested fix is to hold the task_lock() around the accesses to
> > > p->mm and to the mm_struct membarrier_state field to guarantee the
> > > existence of the mm_struct.
> > >
> > > Link: https://lore.kernel.org/lkml/CAG48ez2G8ctF8dHS42TF37pThfr3y0RNOOYTmxvACm4u8Yu3cw@mail.gmail.com
> > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> [...]
> > > --- a/kernel/sched/membarrier.c
> > > +++ b/kernel/sched/membarrier.c
> > > @@ -81,12 +81,27 @@ static int membarrier_global_expedited(void)
> > >
> > > rcu_read_lock();
> > > p = task_rcu_dereference(&cpu_rq(cpu)->curr);
> > > - if (p && p->mm && (atomic_read(&p->mm->membarrier_state) &
> > > - MEMBARRIER_STATE_GLOBAL_EXPEDITED)) {
> > > - if (!fallback)
> > > - __cpumask_set_cpu(cpu, tmpmask);
> > > - else
> > > - smp_call_function_single(cpu, ipi_mb, NULL, 1);
> > > + /*
> > > + * Skip this CPU if the runqueue's current task is NULL or if
> > > + * it is a kernel thread.
> > > + */
> > > + if (p && READ_ONCE(p->mm)) {
> > > + bool mm_match;
> > > +
> > > + /*
> > > + * Read p->mm and access membarrier_state while holding
> > > + * the task lock to ensure existence of mm.
> > > + */
> > > + task_lock(p);
> > > + mm_match = p->mm && (atomic_read(&p->mm->membarrier_state) &
> >
> > Are we guaranteed that this p->mm will be the same as the one loaded via
> > READ_ONCE() above?
>
> No; the way I read it, that's just an optimization and has no effect
> on correctness.
>
> > Either way, wouldn't it be better to READ_ONCE() it a
> > single time and use the same value everywhere?
>
> No; the first READ_ONCE() returns a pointer that you can't access
> because it wasn't read under a lock. You can only use it for a NULL
> check.
Ah, of course! Thank you both!
Thanx, Paul
^ permalink raw reply
* Re: [PATCH 12/18] io_uring: add support for pre-mapped user IO buffers
From: Jann Horn @ 2019-01-28 23:35 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-aio, linux-block, linux-man, Linux API, hch, jmoyer,
Avi Kivity
In-Reply-To: <20190128213538.13486-13-axboe@kernel.dk>
On Mon, Jan 28, 2019 at 10:36 PM Jens Axboe <axboe@kernel.dk> wrote:
> If we have fixed user buffers, we can map them into the kernel when we
> setup the io_context. That avoids the need to do get_user_pages() for
> each and every IO.
[...]
> +static int __io_uring_register(struct io_ring_ctx *ctx, unsigned opcode,
> + void __user *arg, unsigned nr_args)
> +{
> + int ret;
> +
> + /* Drop our initial ref and wait for the ctx to be fully idle */
> + percpu_ref_put(&ctx->refs);
The line above drops a reference that you just got in the caller...
> + percpu_ref_kill(&ctx->refs);
> + wait_for_completion(&ctx->ctx_done);
> +
> + switch (opcode) {
> + case IORING_REGISTER_BUFFERS:
> + ret = io_sqe_buffer_register(ctx, arg, nr_args);
> + break;
> + case IORING_UNREGISTER_BUFFERS:
> + ret = -EINVAL;
> + if (arg || nr_args)
> + break;
> + ret = io_sqe_buffer_unregister(ctx);
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> +
> + /* bring the ctx back to life */
> + reinit_completion(&ctx->ctx_done);
> + percpu_ref_resurrect(&ctx->refs);
> + percpu_ref_get(&ctx->refs);
And then this line takes a reference that the caller will immediately
drop again? Why?
> + return ret;
> +}
> +
> +SYSCALL_DEFINE4(io_uring_register, unsigned int, fd, unsigned int, opcode,
> + void __user *, arg, unsigned int, nr_args)
> +{
> + struct io_ring_ctx *ctx;
> + long ret = -EBADF;
> + struct fd f;
> +
> + f = fdget(fd);
> + if (!f.file)
> + return -EBADF;
> +
> + ret = -EOPNOTSUPP;
> + if (f.file->f_op != &io_uring_fops)
> + goto out_fput;
> +
> + ret = -ENXIO;
> + ctx = f.file->private_data;
> + if (!percpu_ref_tryget(&ctx->refs))
> + goto out_fput;
If you are holding the uring_lock of a ctx that can be accessed
through a file descriptor (which you do just after this point), you
know that the percpu_ref isn't zero, right? Why are you doing the
tryget here?
> + ret = -EBUSY;
> + if (mutex_trylock(&ctx->uring_lock)) {
> + ret = __io_uring_register(ctx, opcode, arg, nr_args);
> + mutex_unlock(&ctx->uring_lock);
> + }
> + io_ring_drop_ctx_refs(ctx, 1);
> +out_fput:
> + fdput(f);
> + return ret;
> +}
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Jens Axboe @ 2019-01-28 23:46 UTC (permalink / raw)
To: Jann Horn, Al Viro
Cc: linux-aio, linux-block, linux-man, Linux API, hch, jmoyer,
Avi Kivity
In-Reply-To: <CAG48ez0vDqDH4ks7q4L3F+xt-4kVQrN1yw34QwFAmwQyy27FTw@mail.gmail.com>
On 1/28/19 3:32 PM, Jann Horn wrote:
> On Mon, Jan 28, 2019 at 10:35 PM Jens Axboe <axboe@kernel.dk> wrote:
>> The submission queue (SQ) and completion queue (CQ) rings are shared
>> between the application and the kernel. This eliminates the need to
>> copy data back and forth to submit and complete IO.
>>
>> IO submissions use the io_uring_sqe data structure, and completions
>> are generated in the form of io_uring_sqe data structures. The SQ
>> ring is an index into the io_uring_sqe array, which makes it possible
>> to submit a batch of IOs without them being contiguous in the ring.
>> The CQ ring is always contiguous, as completion events are inherently
>> unordered, and hence any io_uring_cqe entry can point back to an
>> arbitrary submission.
>>
>> Two new system calls are added for this:
>>
>> io_uring_setup(entries, params)
>> Sets up a context for doing async IO. On success, returns a file
>> descriptor that the application can mmap to gain access to the
>> SQ ring, CQ ring, and io_uring_sqes.
>>
>> io_uring_enter(fd, to_submit, min_complete, flags, sigset, sigsetsize)
>> Initiates IO against the rings mapped to this fd, or waits for
>> them to complete, or both. The behavior is controlled by the
>> parameters passed in. If 'to_submit' is non-zero, then we'll
>> try and submit new IO. If IORING_ENTER_GETEVENTS is set, the
>> kernel will wait for 'min_complete' events, if they aren't
>> already available. It's valid to set IORING_ENTER_GETEVENTS
>> and 'min_complete' == 0 at the same time, this allows the
>> kernel to return already completed events without waiting
>> for them. This is useful only for polling, as for IRQ
>> driven IO, the application can just check the CQ ring
>> without entering the kernel.
>>
>> With this setup, it's possible to do async IO with a single system
>> call. Future developments will enable polled IO with this interface,
>> and polled submission as well. The latter will enable an application
>> to do IO without doing ANY system calls at all.
>>
>> For IRQ driven IO, an application only needs to enter the kernel for
>> completions if it wants to wait for them to occur.
>>
>> Each io_uring is backed by a workqueue, to support buffered async IO
>> as well. We will only punt to an async context if the command would
>> need to wait for IO on the device side. Any data that can be accessed
>> directly in the page cache is done inline. This avoids the slowness
>> issue of usual threadpools, since cached data is accessed as quickly
>> as a sync interface.
>>
>> Sample application: http://git.kernel.dk/cgit/fio/plain/t/io_uring.c
> [...]
>> +static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe,
>> + bool force_nonblock)
>> +{
>> + struct kiocb *kiocb = &req->rw;
>> + int ret;
>> +
>> + kiocb->ki_filp = fget(sqe->fd);
>> + if (unlikely(!kiocb->ki_filp))
>> + return -EBADF;
>> + kiocb->ki_pos = sqe->off;
>> + kiocb->ki_flags = iocb_flags(kiocb->ki_filp);
>> + kiocb->ki_hint = ki_hint_validate(file_write_hint(kiocb->ki_filp));
>> + if (sqe->ioprio) {
>> + ret = ioprio_check_cap(sqe->ioprio);
>> + if (ret)
>> + goto out_fput;
>> +
>> + kiocb->ki_ioprio = sqe->ioprio;
>> + } else
>> + kiocb->ki_ioprio = get_current_ioprio();
>> +
>> + ret = kiocb_set_rw_flags(kiocb, sqe->rw_flags);
>> + if (unlikely(ret))
>> + goto out_fput;
>> + if (force_nonblock) {
>> + kiocb->ki_flags |= IOCB_NOWAIT;
>> + req->flags |= REQ_F_FORCE_NONBLOCK;
>> + }
>> + if (kiocb->ki_flags & IOCB_HIPRI) {
>> + ret = -EINVAL;
>> + goto out_fput;
>> + }
>> +
>> + kiocb->ki_complete = io_complete_rw;
>> + return 0;
>> +out_fput:
>> + fput(kiocb->ki_filp);
>> + return ret;
>> +}
> [...]
>> +static ssize_t io_read(struct io_kiocb *req, const struct io_uring_sqe *sqe,
>> + bool force_nonblock)
>> +{
>> + struct iovec inline_vecs[UIO_FASTIOV], *iovec = inline_vecs;
>> + struct kiocb *kiocb = &req->rw;
>> + struct iov_iter iter;
>> + struct file *file;
>> + ssize_t ret;
>> +
>> + ret = io_prep_rw(req, sqe, force_nonblock);
>> + if (ret)
>> + return ret;
>> + file = kiocb->ki_filp;
>> +
>> + ret = -EBADF;
>> + if (unlikely(!(file->f_mode & FMODE_READ)))
>> + goto out_fput;
>> + ret = -EINVAL;
>> + if (unlikely(!file->f_op->read_iter))
>> + goto out_fput;
>> +
>> + ret = io_import_iovec(req->ctx, READ, sqe, &iovec, &iter);
>> + if (ret)
>> + goto out_fput;
>> +
>> + ret = rw_verify_area(READ, file, &kiocb->ki_pos, iov_iter_count(&iter));
>> + if (!ret) {
>> + ssize_t ret2;
>> +
>> + /* Catch -EAGAIN return for forced non-blocking submission */
>> + ret2 = call_read_iter(file, kiocb, &iter);
>> + if (!force_nonblock || ret2 != -EAGAIN)
>> + io_rw_done(kiocb, ret2);
>> + else
>> + ret = -EAGAIN;
>> + }
>> + kfree(iovec);
>> +out_fput:
>> + if (unlikely(ret))
>> + fput(file);
>> + return ret;
>> +}
> [...]
>> +static int __io_submit_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
>> + struct sqe_submit *s, bool force_nonblock)
>> +{
>> + const struct io_uring_sqe *sqe = s->sqe;
>> + ssize_t ret;
>> +
>> + if (unlikely(s->index >= ctx->sq_entries))
>> + return -EINVAL;
>> + req->user_data = sqe->user_data;
>> +
>> + ret = -EINVAL;
>> + switch (sqe->opcode) {
>> + case IORING_OP_NOP:
>> + ret = io_nop(req, sqe);
>> + break;
>> + case IORING_OP_READV:
>> + ret = io_read(req, sqe, force_nonblock);
>> + break;
>> + case IORING_OP_WRITEV:
>> + ret = io_write(req, sqe, force_nonblock);
>> + break;
>> + default:
>> + ret = -EINVAL;
>> + break;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static void io_sq_wq_submit_work(struct work_struct *work)
>> +{
>> + struct io_kiocb *req = container_of(work, struct io_kiocb, work);
>> + struct sqe_submit *s = &req->submit;
>> + u64 user_data = s->sqe->user_data;
>> + struct io_ring_ctx *ctx = req->ctx;
>> + mm_segment_t old_fs = get_fs();
>> + struct files_struct *old_files;
>> + int ret;
>> +
>> + /* Ensure we clear previously set forced non-block flag */
>> + req->flags &= ~REQ_F_FORCE_NONBLOCK;
>> +
>> + old_files = current->files;
>> + current->files = ctx->sqo_files;
>
> I think you're not supposed to twiddle with current->files without
> holding task_lock(current).
'current' is the work queue item in this case, do we need to protect
against anything else? I can add the locking around the assignments
(both places).
>> + if (!mmget_not_zero(ctx->sqo_mm)) {
>> + ret = -EFAULT;
>> + goto err;
>> + }
>> +
>> + use_mm(ctx->sqo_mm);
>> + set_fs(USER_DS);
>> +
>> + ret = __io_submit_sqe(ctx, req, s, false);
>> +
>> + set_fs(old_fs);
>> + unuse_mm(ctx->sqo_mm);
>> + mmput(ctx->sqo_mm);
>> +err:
>> + if (ret) {
>> + io_cqring_add_event(ctx, user_data, ret, 0);
>> + io_free_req(req);
>> + }
>> + current->files = old_files;
>> +}
> [...]
>> +static int io_sq_offload_start(struct io_ring_ctx *ctx)
>> +{
>> + int ret;
>> +
>> + ctx->sqo_mm = current->mm;
>
> What keeps this thing alive?
I think we're deadling with the same thing as the files below, I'll
defer to that.
>> + /*
>> + * This is safe since 'current' has the fd installed, and if that gets
>> + * closed on exit, then fops->release() is invoked which waits for the
>> + * async contexts to flush and exit before exiting.
>> + */
>> + ret = -EBADF;
>> + ctx->sqo_files = current->files;
>> + if (!ctx->sqo_files)
>> + goto err;
>
> That's gnarly. Adding Al Viro to the thread.
>
> I think you misunderstand the semantics of f_op->release. The ->flush
> handler is invoked whenever a file descriptor is closed through
> filp_close() (via deletion of the files_struct, sys_close(),
> sys_dup2(), ...), so if you had used that one, _maybe_ this would
> work. But the ->release handler only runs when the _last_ reference to
> a struct file has been dropped - so you can, for example, fork() a
> child, then exit() in the parent, and the ->release handler isn't
> invoked. So I don't see how this can work.
The anonfd is CLOEXEC. The idea is exactly that it only runs when the
last reference to the file has been dropped. Not sure why you think I
need ->flush() here?
> But even if you had abused ->flush for this instead: close_files()
> currently has a comment in it that claims that "this is the last
> reference to the files structure"; this change would make that claim
> untrue.
Let me see if I can explain my intent better than that comment... We
know the parent who set up the io_uring instance will be around for as
long as io_uring instance persists. When we are tearing down the
io_uring, then we wait for any async contexts (like the one above) to
exit. Once they are exited, it's safe to proceed with the exit and
teardown ->files[].
That's the idea... Not trying to be clever, some of this dates back to
the aio weirdness where it was impossible to have cross references like
this, since it would lead to teardown deadlocks with how exit_aio() is
used. I can probably grab a struct files reference above, but currently
I don't see why it's needed.
>> + /* Do QD, or 2 * CPUS, whatever is smallest */
>> + ctx->sqo_wq = alloc_workqueue("io_ring-wq", WQ_UNBOUND | WQ_FREEZABLE,
>> + min(ctx->sq_entries - 1, 2 * num_online_cpus()));
>> + if (!ctx->sqo_wq) {
>> + ret = -ENOMEM;
>> + goto err;
>> + }
>> +
>> + return 0;
>> +err:
>> + if (ctx->sqo_files)
>> + ctx->sqo_files = NULL;
>> + ctx->sqo_mm = NULL;
>> + return ret;
>> +}
> [...]
>> +static const struct file_operations io_uring_fops = {
>> + .release = io_uring_release,
>> + .mmap = io_uring_mmap,
>> + .poll = io_uring_poll,
>> + .fasync = io_uring_fasync,
>> +};
> [...]
>
--
Jens Axboe
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 12/18] io_uring: add support for pre-mapped user IO buffers
From: Jens Axboe @ 2019-01-28 23:50 UTC (permalink / raw)
To: Jann Horn
Cc: linux-aio, linux-block, linux-man, Linux API, hch, jmoyer,
Avi Kivity
In-Reply-To: <CAG48ez3cJS=RUqyP630du3pNw_wBdM82KOtbX=YY6U4LxfMBew@mail.gmail.com>
On 1/28/19 4:35 PM, Jann Horn wrote:
> On Mon, Jan 28, 2019 at 10:36 PM Jens Axboe <axboe@kernel.dk> wrote:
>> If we have fixed user buffers, we can map them into the kernel when we
>> setup the io_context. That avoids the need to do get_user_pages() for
>> each and every IO.
> [...]
>> +static int __io_uring_register(struct io_ring_ctx *ctx, unsigned opcode,
>> + void __user *arg, unsigned nr_args)
>> +{
>> + int ret;
>> +
>> + /* Drop our initial ref and wait for the ctx to be fully idle */
>> + percpu_ref_put(&ctx->refs);
>
> The line above drops a reference that you just got in the caller...
Right
>> + percpu_ref_kill(&ctx->refs);
>> + wait_for_completion(&ctx->ctx_done);
>> +
>> + switch (opcode) {
>> + case IORING_REGISTER_BUFFERS:
>> + ret = io_sqe_buffer_register(ctx, arg, nr_args);
>> + break;
>> + case IORING_UNREGISTER_BUFFERS:
>> + ret = -EINVAL;
>> + if (arg || nr_args)
>> + break;
>> + ret = io_sqe_buffer_unregister(ctx);
>> + break;
>> + default:
>> + ret = -EINVAL;
>> + break;
>> + }
>> +
>> + /* bring the ctx back to life */
>> + reinit_completion(&ctx->ctx_done);
>> + percpu_ref_resurrect(&ctx->refs);
>> + percpu_ref_get(&ctx->refs);
>
> And then this line takes a reference that the caller will immediately
> drop again? Why?
Just want to keep it symmetric and avoid having weird "this function drops
a reference" use cases.
>
>> + return ret;
>> +}
>> +
>> +SYSCALL_DEFINE4(io_uring_register, unsigned int, fd, unsigned int, opcode,
>> + void __user *, arg, unsigned int, nr_args)
>> +{
>> + struct io_ring_ctx *ctx;
>> + long ret = -EBADF;
>> + struct fd f;
>> +
>> + f = fdget(fd);
>> + if (!f.file)
>> + return -EBADF;
>> +
>> + ret = -EOPNOTSUPP;
>> + if (f.file->f_op != &io_uring_fops)
>> + goto out_fput;
>> +
>> + ret = -ENXIO;
>> + ctx = f.file->private_data;
>> + if (!percpu_ref_tryget(&ctx->refs))
>> + goto out_fput;
>
> If you are holding the uring_lock of a ctx that can be accessed
> through a file descriptor (which you do just after this point), you
> know that the percpu_ref isn't zero, right? Why are you doing the
> tryget here?
Not sure I follow... We don't hold the lock at this point. I guess your
point is that since the descriptor is open (or we'd fail the above
check), then there's no point doing the tryget variant here? That's
strictly true, that could just be a get().
--
Jens Axboe
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Jann Horn @ 2019-01-28 23:59 UTC (permalink / raw)
To: Jens Axboe
Cc: Al Viro, linux-aio, linux-block, linux-man, Linux API, hch,
jmoyer, Avi Kivity
In-Reply-To: <e9326a77-54c5-e2b8-d9e5-663261462597@kernel.dk>
On Tue, Jan 29, 2019 at 12:47 AM Jens Axboe <axboe@kernel.dk> wrote:
> On 1/28/19 3:32 PM, Jann Horn wrote:
> > On Mon, Jan 28, 2019 at 10:35 PM Jens Axboe <axboe@kernel.dk> wrote:
> >> The submission queue (SQ) and completion queue (CQ) rings are shared
> >> between the application and the kernel. This eliminates the need to
> >> copy data back and forth to submit and complete IO.
> >>
> >> IO submissions use the io_uring_sqe data structure, and completions
> >> are generated in the form of io_uring_sqe data structures. The SQ
> >> ring is an index into the io_uring_sqe array, which makes it possible
> >> to submit a batch of IOs without them being contiguous in the ring.
> >> The CQ ring is always contiguous, as completion events are inherently
> >> unordered, and hence any io_uring_cqe entry can point back to an
> >> arbitrary submission.
> >>
> >> Two new system calls are added for this:
> >>
> >> io_uring_setup(entries, params)
> >> Sets up a context for doing async IO. On success, returns a file
> >> descriptor that the application can mmap to gain access to the
> >> SQ ring, CQ ring, and io_uring_sqes.
> >>
> >> io_uring_enter(fd, to_submit, min_complete, flags, sigset, sigsetsize)
> >> Initiates IO against the rings mapped to this fd, or waits for
> >> them to complete, or both. The behavior is controlled by the
> >> parameters passed in. If 'to_submit' is non-zero, then we'll
> >> try and submit new IO. If IORING_ENTER_GETEVENTS is set, the
> >> kernel will wait for 'min_complete' events, if they aren't
> >> already available. It's valid to set IORING_ENTER_GETEVENTS
> >> and 'min_complete' == 0 at the same time, this allows the
> >> kernel to return already completed events without waiting
> >> for them. This is useful only for polling, as for IRQ
> >> driven IO, the application can just check the CQ ring
> >> without entering the kernel.
> >>
> >> With this setup, it's possible to do async IO with a single system
> >> call. Future developments will enable polled IO with this interface,
> >> and polled submission as well. The latter will enable an application
> >> to do IO without doing ANY system calls at all.
> >>
> >> For IRQ driven IO, an application only needs to enter the kernel for
> >> completions if it wants to wait for them to occur.
> >>
> >> Each io_uring is backed by a workqueue, to support buffered async IO
> >> as well. We will only punt to an async context if the command would
> >> need to wait for IO on the device side. Any data that can be accessed
> >> directly in the page cache is done inline. This avoids the slowness
> >> issue of usual threadpools, since cached data is accessed as quickly
> >> as a sync interface.
> >>
> >> Sample application: http://git.kernel.dk/cgit/fio/plain/t/io_uring.c
> > [...]
> >> +static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe,
> >> + bool force_nonblock)
> >> +{
> >> + struct kiocb *kiocb = &req->rw;
> >> + int ret;
> >> +
> >> + kiocb->ki_filp = fget(sqe->fd);
> >> + if (unlikely(!kiocb->ki_filp))
> >> + return -EBADF;
> >> + kiocb->ki_pos = sqe->off;
> >> + kiocb->ki_flags = iocb_flags(kiocb->ki_filp);
> >> + kiocb->ki_hint = ki_hint_validate(file_write_hint(kiocb->ki_filp));
> >> + if (sqe->ioprio) {
> >> + ret = ioprio_check_cap(sqe->ioprio);
> >> + if (ret)
> >> + goto out_fput;
> >> +
> >> + kiocb->ki_ioprio = sqe->ioprio;
> >> + } else
> >> + kiocb->ki_ioprio = get_current_ioprio();
> >> +
> >> + ret = kiocb_set_rw_flags(kiocb, sqe->rw_flags);
> >> + if (unlikely(ret))
> >> + goto out_fput;
> >> + if (force_nonblock) {
> >> + kiocb->ki_flags |= IOCB_NOWAIT;
> >> + req->flags |= REQ_F_FORCE_NONBLOCK;
> >> + }
> >> + if (kiocb->ki_flags & IOCB_HIPRI) {
> >> + ret = -EINVAL;
> >> + goto out_fput;
> >> + }
> >> +
> >> + kiocb->ki_complete = io_complete_rw;
> >> + return 0;
> >> +out_fput:
> >> + fput(kiocb->ki_filp);
> >> + return ret;
> >> +}
> > [...]
> >> +static ssize_t io_read(struct io_kiocb *req, const struct io_uring_sqe *sqe,
> >> + bool force_nonblock)
> >> +{
> >> + struct iovec inline_vecs[UIO_FASTIOV], *iovec = inline_vecs;
> >> + struct kiocb *kiocb = &req->rw;
> >> + struct iov_iter iter;
> >> + struct file *file;
> >> + ssize_t ret;
> >> +
> >> + ret = io_prep_rw(req, sqe, force_nonblock);
> >> + if (ret)
> >> + return ret;
> >> + file = kiocb->ki_filp;
> >> +
> >> + ret = -EBADF;
> >> + if (unlikely(!(file->f_mode & FMODE_READ)))
> >> + goto out_fput;
> >> + ret = -EINVAL;
> >> + if (unlikely(!file->f_op->read_iter))
> >> + goto out_fput;
> >> +
> >> + ret = io_import_iovec(req->ctx, READ, sqe, &iovec, &iter);
> >> + if (ret)
> >> + goto out_fput;
> >> +
> >> + ret = rw_verify_area(READ, file, &kiocb->ki_pos, iov_iter_count(&iter));
> >> + if (!ret) {
> >> + ssize_t ret2;
> >> +
> >> + /* Catch -EAGAIN return for forced non-blocking submission */
> >> + ret2 = call_read_iter(file, kiocb, &iter);
> >> + if (!force_nonblock || ret2 != -EAGAIN)
> >> + io_rw_done(kiocb, ret2);
> >> + else
> >> + ret = -EAGAIN;
> >> + }
> >> + kfree(iovec);
> >> +out_fput:
> >> + if (unlikely(ret))
> >> + fput(file);
> >> + return ret;
> >> +}
> > [...]
> >> +static int __io_submit_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
> >> + struct sqe_submit *s, bool force_nonblock)
> >> +{
> >> + const struct io_uring_sqe *sqe = s->sqe;
> >> + ssize_t ret;
> >> +
> >> + if (unlikely(s->index >= ctx->sq_entries))
> >> + return -EINVAL;
> >> + req->user_data = sqe->user_data;
> >> +
> >> + ret = -EINVAL;
> >> + switch (sqe->opcode) {
> >> + case IORING_OP_NOP:
> >> + ret = io_nop(req, sqe);
> >> + break;
> >> + case IORING_OP_READV:
> >> + ret = io_read(req, sqe, force_nonblock);
> >> + break;
> >> + case IORING_OP_WRITEV:
> >> + ret = io_write(req, sqe, force_nonblock);
> >> + break;
> >> + default:
> >> + ret = -EINVAL;
> >> + break;
> >> + }
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static void io_sq_wq_submit_work(struct work_struct *work)
> >> +{
> >> + struct io_kiocb *req = container_of(work, struct io_kiocb, work);
> >> + struct sqe_submit *s = &req->submit;
> >> + u64 user_data = s->sqe->user_data;
> >> + struct io_ring_ctx *ctx = req->ctx;
> >> + mm_segment_t old_fs = get_fs();
> >> + struct files_struct *old_files;
> >> + int ret;
> >> +
> >> + /* Ensure we clear previously set forced non-block flag */
> >> + req->flags &= ~REQ_F_FORCE_NONBLOCK;
> >> +
> >> + old_files = current->files;
> >> + current->files = ctx->sqo_files;
> >
> > I think you're not supposed to twiddle with current->files without
> > holding task_lock(current).
>
> 'current' is the work queue item in this case, do we need to protect
> against anything else? I can add the locking around the assignments
> (both places).
Stuff like proc_fd_link() uses get_files_struct(), which grabs a
reference to your current files_struct protected only by task_lock();
and it doesn't use anything like READ_ONCE(), so even if the object
lifetime is not a problem, get_files_struct() could potentially crash
due to a double-read (reading task->files twice and assuming that the
result will be the same). As far as I can tell, this procfs code also
works on kernel threads.
> >> + if (!mmget_not_zero(ctx->sqo_mm)) {
> >> + ret = -EFAULT;
> >> + goto err;
> >> + }
> >> +
> >> + use_mm(ctx->sqo_mm);
> >> + set_fs(USER_DS);
> >> +
> >> + ret = __io_submit_sqe(ctx, req, s, false);
> >> +
> >> + set_fs(old_fs);
> >> + unuse_mm(ctx->sqo_mm);
> >> + mmput(ctx->sqo_mm);
> >> +err:
> >> + if (ret) {
> >> + io_cqring_add_event(ctx, user_data, ret, 0);
> >> + io_free_req(req);
> >> + }
> >> + current->files = old_files;
> >> +}
> > [...]
> >> +static int io_sq_offload_start(struct io_ring_ctx *ctx)
> >> +{
> >> + int ret;
> >> +
> >> + ctx->sqo_mm = current->mm;
> >
> > What keeps this thing alive?
>
> I think we're deadling with the same thing as the files below, I'll
> defer to that.
>
> >> + /*
> >> + * This is safe since 'current' has the fd installed, and if that gets
> >> + * closed on exit, then fops->release() is invoked which waits for the
> >> + * async contexts to flush and exit before exiting.
> >> + */
> >> + ret = -EBADF;
> >> + ctx->sqo_files = current->files;
> >> + if (!ctx->sqo_files)
> >> + goto err;
> >
> > That's gnarly. Adding Al Viro to the thread.
> >
> > I think you misunderstand the semantics of f_op->release. The ->flush
> > handler is invoked whenever a file descriptor is closed through
> > filp_close() (via deletion of the files_struct, sys_close(),
> > sys_dup2(), ...), so if you had used that one, _maybe_ this would
> > work. But the ->release handler only runs when the _last_ reference to
> > a struct file has been dropped - so you can, for example, fork() a
> > child, then exit() in the parent, and the ->release handler isn't
> > invoked. So I don't see how this can work.
>
> The anonfd is CLOEXEC. The idea is exactly that it only runs when the
> last reference to the file has been dropped. Not sure why you think I
> need ->flush() here?
Can't I just use fcntl(fd, F_SETFD, fd, 0) to clear the CLOEXEC flag?
Or send the fd via SCM_RIGHTS?
> > But even if you had abused ->flush for this instead: close_files()
> > currently has a comment in it that claims that "this is the last
> > reference to the files structure"; this change would make that claim
> > untrue.
>
> Let me see if I can explain my intent better than that comment... We
> know the parent who set up the io_uring instance will be around for as
> long as io_uring instance persists.
That's the part that I think is wrong: As far as I can tell, the
parent can go away and you won't notice.
Also, note that "the parent" is different things for ->files and ->mm.
You can have a multithreaded process whose threads don't have the same
->files, or multiple process that share ->files without sharing ->mm,
...
> When we are tearing down the
> io_uring, then we wait for any async contexts (like the one above) to
> exit. Once they are exited, it's safe to proceed with the exit and
> teardown ->files[].
But you only do that teardown on ->release, right? And ->release
doesn't have much to do with the process lifetime.
> That's the idea... Not trying to be clever, some of this dates back to
> the aio weirdness where it was impossible to have cross references like
> this, since it would lead to teardown deadlocks with how exit_aio() is
> used. I can probably grab a struct files reference above, but currently
> I don't see why it's needed.
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Jens Axboe @ 2019-01-29 0:03 UTC (permalink / raw)
To: Jann Horn
Cc: Al Viro, linux-aio, linux-block, linux-man, Linux API, hch,
jmoyer, Avi Kivity
In-Reply-To: <CAG48ez17NW0GJVRC6dFcHZTgQifFz5og1XCUbXkHKhr6f=j74Q@mail.gmail.com>
On 1/28/19 4:59 PM, Jann Horn wrote:
> On Tue, Jan 29, 2019 at 12:47 AM Jens Axboe <axboe@kernel.dk> wrote:
>> On 1/28/19 3:32 PM, Jann Horn wrote:
>>> On Mon, Jan 28, 2019 at 10:35 PM Jens Axboe <axboe@kernel.dk> wrote:
>>>> The submission queue (SQ) and completion queue (CQ) rings are shared
>>>> between the application and the kernel. This eliminates the need to
>>>> copy data back and forth to submit and complete IO.
>>>>
>>>> IO submissions use the io_uring_sqe data structure, and completions
>>>> are generated in the form of io_uring_sqe data structures. The SQ
>>>> ring is an index into the io_uring_sqe array, which makes it possible
>>>> to submit a batch of IOs without them being contiguous in the ring.
>>>> The CQ ring is always contiguous, as completion events are inherently
>>>> unordered, and hence any io_uring_cqe entry can point back to an
>>>> arbitrary submission.
>>>>
>>>> Two new system calls are added for this:
>>>>
>>>> io_uring_setup(entries, params)
>>>> Sets up a context for doing async IO. On success, returns a file
>>>> descriptor that the application can mmap to gain access to the
>>>> SQ ring, CQ ring, and io_uring_sqes.
>>>>
>>>> io_uring_enter(fd, to_submit, min_complete, flags, sigset, sigsetsize)
>>>> Initiates IO against the rings mapped to this fd, or waits for
>>>> them to complete, or both. The behavior is controlled by the
>>>> parameters passed in. If 'to_submit' is non-zero, then we'll
>>>> try and submit new IO. If IORING_ENTER_GETEVENTS is set, the
>>>> kernel will wait for 'min_complete' events, if they aren't
>>>> already available. It's valid to set IORING_ENTER_GETEVENTS
>>>> and 'min_complete' == 0 at the same time, this allows the
>>>> kernel to return already completed events without waiting
>>>> for them. This is useful only for polling, as for IRQ
>>>> driven IO, the application can just check the CQ ring
>>>> without entering the kernel.
>>>>
>>>> With this setup, it's possible to do async IO with a single system
>>>> call. Future developments will enable polled IO with this interface,
>>>> and polled submission as well. The latter will enable an application
>>>> to do IO without doing ANY system calls at all.
>>>>
>>>> For IRQ driven IO, an application only needs to enter the kernel for
>>>> completions if it wants to wait for them to occur.
>>>>
>>>> Each io_uring is backed by a workqueue, to support buffered async IO
>>>> as well. We will only punt to an async context if the command would
>>>> need to wait for IO on the device side. Any data that can be accessed
>>>> directly in the page cache is done inline. This avoids the slowness
>>>> issue of usual threadpools, since cached data is accessed as quickly
>>>> as a sync interface.
>>>>
>>>> Sample application: http://git.kernel.dk/cgit/fio/plain/t/io_uring.c
>>> [...]
>>>> +static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe,
>>>> + bool force_nonblock)
>>>> +{
>>>> + struct kiocb *kiocb = &req->rw;
>>>> + int ret;
>>>> +
>>>> + kiocb->ki_filp = fget(sqe->fd);
>>>> + if (unlikely(!kiocb->ki_filp))
>>>> + return -EBADF;
>>>> + kiocb->ki_pos = sqe->off;
>>>> + kiocb->ki_flags = iocb_flags(kiocb->ki_filp);
>>>> + kiocb->ki_hint = ki_hint_validate(file_write_hint(kiocb->ki_filp));
>>>> + if (sqe->ioprio) {
>>>> + ret = ioprio_check_cap(sqe->ioprio);
>>>> + if (ret)
>>>> + goto out_fput;
>>>> +
>>>> + kiocb->ki_ioprio = sqe->ioprio;
>>>> + } else
>>>> + kiocb->ki_ioprio = get_current_ioprio();
>>>> +
>>>> + ret = kiocb_set_rw_flags(kiocb, sqe->rw_flags);
>>>> + if (unlikely(ret))
>>>> + goto out_fput;
>>>> + if (force_nonblock) {
>>>> + kiocb->ki_flags |= IOCB_NOWAIT;
>>>> + req->flags |= REQ_F_FORCE_NONBLOCK;
>>>> + }
>>>> + if (kiocb->ki_flags & IOCB_HIPRI) {
>>>> + ret = -EINVAL;
>>>> + goto out_fput;
>>>> + }
>>>> +
>>>> + kiocb->ki_complete = io_complete_rw;
>>>> + return 0;
>>>> +out_fput:
>>>> + fput(kiocb->ki_filp);
>>>> + return ret;
>>>> +}
>>> [...]
>>>> +static ssize_t io_read(struct io_kiocb *req, const struct io_uring_sqe *sqe,
>>>> + bool force_nonblock)
>>>> +{
>>>> + struct iovec inline_vecs[UIO_FASTIOV], *iovec = inline_vecs;
>>>> + struct kiocb *kiocb = &req->rw;
>>>> + struct iov_iter iter;
>>>> + struct file *file;
>>>> + ssize_t ret;
>>>> +
>>>> + ret = io_prep_rw(req, sqe, force_nonblock);
>>>> + if (ret)
>>>> + return ret;
>>>> + file = kiocb->ki_filp;
>>>> +
>>>> + ret = -EBADF;
>>>> + if (unlikely(!(file->f_mode & FMODE_READ)))
>>>> + goto out_fput;
>>>> + ret = -EINVAL;
>>>> + if (unlikely(!file->f_op->read_iter))
>>>> + goto out_fput;
>>>> +
>>>> + ret = io_import_iovec(req->ctx, READ, sqe, &iovec, &iter);
>>>> + if (ret)
>>>> + goto out_fput;
>>>> +
>>>> + ret = rw_verify_area(READ, file, &kiocb->ki_pos, iov_iter_count(&iter));
>>>> + if (!ret) {
>>>> + ssize_t ret2;
>>>> +
>>>> + /* Catch -EAGAIN return for forced non-blocking submission */
>>>> + ret2 = call_read_iter(file, kiocb, &iter);
>>>> + if (!force_nonblock || ret2 != -EAGAIN)
>>>> + io_rw_done(kiocb, ret2);
>>>> + else
>>>> + ret = -EAGAIN;
>>>> + }
>>>> + kfree(iovec);
>>>> +out_fput:
>>>> + if (unlikely(ret))
>>>> + fput(file);
>>>> + return ret;
>>>> +}
>>> [...]
>>>> +static int __io_submit_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
>>>> + struct sqe_submit *s, bool force_nonblock)
>>>> +{
>>>> + const struct io_uring_sqe *sqe = s->sqe;
>>>> + ssize_t ret;
>>>> +
>>>> + if (unlikely(s->index >= ctx->sq_entries))
>>>> + return -EINVAL;
>>>> + req->user_data = sqe->user_data;
>>>> +
>>>> + ret = -EINVAL;
>>>> + switch (sqe->opcode) {
>>>> + case IORING_OP_NOP:
>>>> + ret = io_nop(req, sqe);
>>>> + break;
>>>> + case IORING_OP_READV:
>>>> + ret = io_read(req, sqe, force_nonblock);
>>>> + break;
>>>> + case IORING_OP_WRITEV:
>>>> + ret = io_write(req, sqe, force_nonblock);
>>>> + break;
>>>> + default:
>>>> + ret = -EINVAL;
>>>> + break;
>>>> + }
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static void io_sq_wq_submit_work(struct work_struct *work)
>>>> +{
>>>> + struct io_kiocb *req = container_of(work, struct io_kiocb, work);
>>>> + struct sqe_submit *s = &req->submit;
>>>> + u64 user_data = s->sqe->user_data;
>>>> + struct io_ring_ctx *ctx = req->ctx;
>>>> + mm_segment_t old_fs = get_fs();
>>>> + struct files_struct *old_files;
>>>> + int ret;
>>>> +
>>>> + /* Ensure we clear previously set forced non-block flag */
>>>> + req->flags &= ~REQ_F_FORCE_NONBLOCK;
>>>> +
>>>> + old_files = current->files;
>>>> + current->files = ctx->sqo_files;
>>>
>>> I think you're not supposed to twiddle with current->files without
>>> holding task_lock(current).
>>
>> 'current' is the work queue item in this case, do we need to protect
>> against anything else? I can add the locking around the assignments
>> (both places).
>
> Stuff like proc_fd_link() uses get_files_struct(), which grabs a
> reference to your current files_struct protected only by task_lock();
> and it doesn't use anything like READ_ONCE(), so even if the object
> lifetime is not a problem, get_files_struct() could potentially crash
> due to a double-read (reading task->files twice and assuming that the
> result will be the same). As far as I can tell, this procfs code also
> works on kernel threads.
OK, that does make sense. I've added the locking.
>>>> + if (!mmget_not_zero(ctx->sqo_mm)) {
>>>> + ret = -EFAULT;
>>>> + goto err;
>>>> + }
>>>> +
>>>> + use_mm(ctx->sqo_mm);
>>>> + set_fs(USER_DS);
>>>> +
>>>> + ret = __io_submit_sqe(ctx, req, s, false);
>>>> +
>>>> + set_fs(old_fs);
>>>> + unuse_mm(ctx->sqo_mm);
>>>> + mmput(ctx->sqo_mm);
>>>> +err:
>>>> + if (ret) {
>>>> + io_cqring_add_event(ctx, user_data, ret, 0);
>>>> + io_free_req(req);
>>>> + }
>>>> + current->files = old_files;
>>>> +}
>>> [...]
>>>> +static int io_sq_offload_start(struct io_ring_ctx *ctx)
>>>> +{
>>>> + int ret;
>>>> +
>>>> + ctx->sqo_mm = current->mm;
>>>
>>> What keeps this thing alive?
>>
>> I think we're deadling with the same thing as the files below, I'll
>> defer to that.
>>
>>>> + /*
>>>> + * This is safe since 'current' has the fd installed, and if that gets
>>>> + * closed on exit, then fops->release() is invoked which waits for the
>>>> + * async contexts to flush and exit before exiting.
>>>> + */
>>>> + ret = -EBADF;
>>>> + ctx->sqo_files = current->files;
>>>> + if (!ctx->sqo_files)
>>>> + goto err;
>>>
>>> That's gnarly. Adding Al Viro to the thread.
>>>
>>> I think you misunderstand the semantics of f_op->release. The ->flush
>>> handler is invoked whenever a file descriptor is closed through
>>> filp_close() (via deletion of the files_struct, sys_close(),
>>> sys_dup2(), ...), so if you had used that one, _maybe_ this would
>>> work. But the ->release handler only runs when the _last_ reference to
>>> a struct file has been dropped - so you can, for example, fork() a
>>> child, then exit() in the parent, and the ->release handler isn't
>>> invoked. So I don't see how this can work.
>>
>> The anonfd is CLOEXEC. The idea is exactly that it only runs when the
>> last reference to the file has been dropped. Not sure why you think I
>> need ->flush() here?
>
> Can't I just use fcntl(fd, F_SETFD, fd, 0) to clear the CLOEXEC flag?
> Or send the fd via SCM_RIGHTS?
That would obviously be a problem...
>>> But even if you had abused ->flush for this instead: close_files()
>>> currently has a comment in it that claims that "this is the last
>>> reference to the files structure"; this change would make that claim
>>> untrue.
>>
>> Let me see if I can explain my intent better than that comment... We
>> know the parent who set up the io_uring instance will be around for as
>> long as io_uring instance persists.
>
> That's the part that I think is wrong: As far as I can tell, the
> parent can go away and you won't notice.
If that's the case, then the mm/files referencing needs to be looked
over for sure. It's currently relying on the fact that the parent stays
alive. If it can go away without ->release() being called, then we have
issues.
> Also, note that "the parent" is different things for ->files and ->mm.
> You can have a multithreaded process whose threads don't have the same
> ->files, or multiple process that share ->files without sharing ->mm,
> ...
Of course, I do realize that.
>> When we are tearing down the
>> io_uring, then we wait for any async contexts (like the one above) to
>> exit. Once they are exited, it's safe to proceed with the exit and
>> teardown ->files[].
>
> But you only do that teardown on ->release, right? And ->release
> doesn't have much to do with the process lifetime.
Yes, only on ->relase().
--
Jens Axboe
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Jens Axboe @ 2019-01-29 0:31 UTC (permalink / raw)
To: Jann Horn
Cc: Al Viro, linux-aio, linux-block, linux-man, Linux API, hch,
jmoyer, Avi Kivity
In-Reply-To: <c20a1250-9f3f-a286-9baf-5b71db0c128f@kernel.dk>
On 1/28/19 5:03 PM, Jens Axboe wrote:
>> But you only do that teardown on ->release, right? And ->release
>> doesn't have much to do with the process lifetime.
>
> Yes, only on ->relase().
OK, so I reworked the files struct to just grab it, then we ensure that
doesn't go away. For mm, it's a bit more tricky. I think the best
solution here is to add a fops->flush() and check for the process
exiting its files. If it does, we quiesce the async contexts and prevent
further use of that mm. We can't just keep holding a reference to the mm
like we do with the files.
That should solve both cases.
--
Jens Axboe
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 05/18] Add io_uring IO interface
From: Jann Horn @ 2019-01-29 0:34 UTC (permalink / raw)
To: Jens Axboe
Cc: Al Viro, linux-aio, linux-block, linux-man, Linux API, hch,
jmoyer, Avi Kivity
In-Reply-To: <51c759f4-7ce5-182b-98e8-2c0e99ca4f9a@kernel.dk>
On Tue, Jan 29, 2019 at 1:32 AM Jens Axboe <axboe@kernel.dk> wrote:
> On 1/28/19 5:03 PM, Jens Axboe wrote:
> >> But you only do that teardown on ->release, right? And ->release
> >> doesn't have much to do with the process lifetime.
> >
> > Yes, only on ->relase().
>
> OK, so I reworked the files struct to just grab it, then we ensure that
> doesn't go away. For mm, it's a bit more tricky. I think the best
> solution here is to add a fops->flush() and check for the process
> exiting its files. If it does, we quiesce the async contexts and prevent
> further use of that mm. We can't just keep holding a reference to the mm
> like we do with the files.
>
> That should solve both cases.
You still have to hold a reference on the mm though, I think (for
example, because two tasks might be sharing the fd table without
sharing the mm).
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 12/18] io_uring: add support for pre-mapped user IO buffers
From: Jann Horn @ 2019-01-29 0:36 UTC (permalink / raw)
To: Jens Axboe
Cc: linux-aio, linux-block, linux-man, Linux API, hch, jmoyer,
Avi Kivity
In-Reply-To: <cb243bfc-0e3a-d5a4-3087-90af1a1824a0@kernel.dk>
On Tue, Jan 29, 2019 at 12:50 AM Jens Axboe <axboe@kernel.dk> wrote:
> On 1/28/19 4:35 PM, Jann Horn wrote:
> > On Mon, Jan 28, 2019 at 10:36 PM Jens Axboe <axboe@kernel.dk> wrote:
> >> If we have fixed user buffers, we can map them into the kernel when we
> >> setup the io_context. That avoids the need to do get_user_pages() for
> >> each and every IO.
> > [...]
> >> +static int __io_uring_register(struct io_ring_ctx *ctx, unsigned opcode,
> >> + void __user *arg, unsigned nr_args)
> >> +{
> >> + int ret;
> >> +
> >> + /* Drop our initial ref and wait for the ctx to be fully idle */
> >> + percpu_ref_put(&ctx->refs);
> >
> > The line above drops a reference that you just got in the caller...
>
> Right
>
> >> + percpu_ref_kill(&ctx->refs);
> >> + wait_for_completion(&ctx->ctx_done);
> >> +
> >> + switch (opcode) {
> >> + case IORING_REGISTER_BUFFERS:
> >> + ret = io_sqe_buffer_register(ctx, arg, nr_args);
> >> + break;
> >> + case IORING_UNREGISTER_BUFFERS:
> >> + ret = -EINVAL;
> >> + if (arg || nr_args)
> >> + break;
> >> + ret = io_sqe_buffer_unregister(ctx);
> >> + break;
> >> + default:
> >> + ret = -EINVAL;
> >> + break;
> >> + }
> >> +
> >> + /* bring the ctx back to life */
> >> + reinit_completion(&ctx->ctx_done);
> >> + percpu_ref_resurrect(&ctx->refs);
> >> + percpu_ref_get(&ctx->refs);
> >
> > And then this line takes a reference that the caller will immediately
> > drop again? Why?
>
> Just want to keep it symmetric and avoid having weird "this function drops
> a reference" use cases.
>
> >
> >> + return ret;
> >> +}
> >> +
> >> +SYSCALL_DEFINE4(io_uring_register, unsigned int, fd, unsigned int, opcode,
> >> + void __user *, arg, unsigned int, nr_args)
> >> +{
> >> + struct io_ring_ctx *ctx;
> >> + long ret = -EBADF;
> >> + struct fd f;
> >> +
> >> + f = fdget(fd);
> >> + if (!f.file)
> >> + return -EBADF;
> >> +
> >> + ret = -EOPNOTSUPP;
> >> + if (f.file->f_op != &io_uring_fops)
> >> + goto out_fput;
> >> +
> >> + ret = -ENXIO;
> >> + ctx = f.file->private_data;
> >> + if (!percpu_ref_tryget(&ctx->refs))
> >> + goto out_fput;
> >
> > If you are holding the uring_lock of a ctx that can be accessed
> > through a file descriptor (which you do just after this point), you
> > know that the percpu_ref isn't zero, right? Why are you doing the
> > tryget here?
>
> Not sure I follow... We don't hold the lock at this point. I guess your
> point is that since the descriptor is open (or we'd fail the above
> check), then there's no point doing the tryget variant here? That's
> strictly true, that could just be a get().
As far as I can tell, you could do the following without breaking anything:
========================
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 6916dc3222cf..c2d82765eefe 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -2485,7 +2485,6 @@ static int __io_uring_register(struct
io_ring_ctx *ctx, unsigned opcode,
int ret;
/* Drop our initial ref and wait for the ctx to be fully idle */
- percpu_ref_put(&ctx->refs);
percpu_ref_kill(&ctx->refs);
wait_for_completion(&ctx->ctx_done);
@@ -2516,7 +2515,6 @@ static int __io_uring_register(struct
io_ring_ctx *ctx, unsigned opcode,
/* bring the ctx back to life */
reinit_completion(&ctx->ctx_done);
percpu_ref_resurrect(&ctx->refs);
- percpu_ref_get(&ctx->refs);
return ret;
}
@@ -2535,17 +2533,13 @@ SYSCALL_DEFINE4(io_uring_register, unsigned
int, fd, unsigned int, opcode,
if (f.file->f_op != &io_uring_fops)
goto out_fput;
- ret = -ENXIO;
ctx = f.file->private_data;
- if (!percpu_ref_tryget(&ctx->refs))
- goto out_fput;
ret = -EBUSY;
if (mutex_trylock(&ctx->uring_lock)) {
ret = __io_uring_register(ctx, opcode, arg, nr_args);
mutex_unlock(&ctx->uring_lock);
}
- io_ring_drop_ctx_refs(ctx, 1);
out_fput:
fdput(f);
return ret;
========================
The two functions that can drop the initial ref of the percpu refcount are:
1. io_ring_ctx_wait_and_kill(); this is only used on ->release() or on
setup failure, meaning that as long as you have a reference to the
file from fget()/fdget(), io_ring_ctx_wait_and_kill() can't have been
called on your context
2. __io_uring_register(); this temporarily kills the percpu refcount
and resurrects it, all under ctx->uring_lock, meaning that as long as
you're holding ctx->uring_lock, __io_uring_register() can't have
killed the percpu refcount
Therefore, I think that as long as you're in sys_io_uring_register and
holding the ctx->uring_lock, you know that the percpu refcount is
alive, and bumping and dropping non-initial references has no effect.
Perhaps this makes more sense when you view the percpu refcount as a
read/write lock - percpu_ref_tryget() takes a read lock, the
percpu_ref_kill() dance takes a write lock.
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply related
* Re: [PATCH 05/18] Add io_uring IO interface
From: Andy Lutomirski @ 2019-01-29 0:47 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jens Axboe, Linux FS Devel, linux-aio, linux-block, Jeff Moyer,
Avi Kivity, Linux API, linux-man
In-Reply-To: <20190128145700.GA9795@lst.de>
On Mon, Jan 28, 2019 at 6:57 AM Christoph Hellwig <hch@lst.de> wrote:
>
> [please make sure linux-api and linux-man are CCed on new syscalls
> so that we get API experts to review them]
> > +static int io_import_iovec(struct io_ring_ctx *ctx, int rw,
> > + const struct io_uring_sqe *sqe,
> > + struct iovec **iovec, struct iov_iter *iter)
> > +{
> > + void __user *buf = u64_to_user_ptr(sqe->addr);
> > +
> > +#ifdef CONFIG_COMPAT
> > + if (ctx->compat)
> > + return compat_import_iovec(rw, buf, sqe->len, UIO_FASTIOV,
> > + iovec, iter);
> > +#endif
>
> I think we can just check in_compat_syscall() here, which means we
> can kill the ->compat member, and the separate compat version of the
> setup syscall.
>
Since this whole API is new, I don't suppose you could introduce a
struct iovec64 or similar and just make the ABI be identical for
64-bit and 32-bit code?
--Andy
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox