* [PATCH 1/8] io_uring: deduplicate core cancellations sequence
2021-02-02 0:21 [PATCH 0/8] a second pack of 5.12 cleanups Pavel Begunkov
@ 2021-02-02 0:21 ` Pavel Begunkov
2021-02-02 0:21 ` [PATCH 2/8] io_uring: refactor scheduling in io_cqring_wait Pavel Begunkov
` (7 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Pavel Begunkov @ 2021-02-02 0:21 UTC (permalink / raw)
To: Jens Axboe, io-uring
Files and task cancellations go over same steps trying to cancel
requests in io-wq, poll, etc. Deduplicate it with a helper.
note: new io_uring_try_cancel_requests() is former
__io_uring_cancel_task_requests() with files passed as an agrument and
flushing overflowed requests.
Signed-off-by: Pavel Begunkov <[email protected]>
---
fs/io_uring.c | 85 ++++++++++++++++++++++++---------------------------
1 file changed, 40 insertions(+), 45 deletions(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 24ad36d71289..a750c504366d 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1003,9 +1003,9 @@ enum io_mem_account {
ACCT_PINNED,
};
-static void __io_uring_cancel_task_requests(struct io_ring_ctx *ctx,
- struct task_struct *task);
-
+static void io_uring_try_cancel_requests(struct io_ring_ctx *ctx,
+ struct task_struct *task,
+ struct files_struct *files);
static void destroy_fixed_rsrc_ref_node(struct fixed_rsrc_ref_node *ref_node);
static struct fixed_rsrc_ref_node *alloc_fixed_rsrc_ref_node(
struct io_ring_ctx *ctx);
@@ -8817,7 +8817,7 @@ static void io_ring_exit_work(struct work_struct *work)
* as nobody else will be looking for them.
*/
do {
- __io_uring_cancel_task_requests(ctx, NULL);
+ io_uring_try_cancel_requests(ctx, NULL, NULL);
} while (!wait_for_completion_timeout(&ctx->ref_comp, HZ/20));
io_ring_ctx_free(ctx);
}
@@ -8931,6 +8931,40 @@ static void io_cancel_defer_files(struct io_ring_ctx *ctx,
}
}
+static void io_uring_try_cancel_requests(struct io_ring_ctx *ctx,
+ struct task_struct *task,
+ struct files_struct *files)
+{
+ struct io_task_cancel cancel = { .task = task, .files = files, };
+
+ while (1) {
+ enum io_wq_cancel cret;
+ bool ret = false;
+
+ if (ctx->io_wq) {
+ cret = io_wq_cancel_cb(ctx->io_wq, io_cancel_task_cb,
+ &cancel, true);
+ ret |= (cret != IO_WQ_CANCEL_NOTFOUND);
+ }
+
+ /* SQPOLL thread does its own polling */
+ if (!(ctx->flags & IORING_SETUP_SQPOLL) && !files) {
+ while (!list_empty_careful(&ctx->iopoll_list)) {
+ io_iopoll_try_reap_events(ctx);
+ ret = true;
+ }
+ }
+
+ ret |= io_poll_remove_all(ctx, task, files);
+ ret |= io_kill_timeouts(ctx, task, files);
+ ret |= io_run_task_work();
+ io_cqring_overflow_flush(ctx, true, task, files);
+ if (!ret)
+ break;
+ cond_resched();
+ }
+}
+
static int io_uring_count_inflight(struct io_ring_ctx *ctx,
struct task_struct *task,
struct files_struct *files)
@@ -8950,7 +8984,6 @@ static void io_uring_cancel_files(struct io_ring_ctx *ctx,
struct files_struct *files)
{
while (!list_empty_careful(&ctx->inflight_list)) {
- struct io_task_cancel cancel = { .task = task, .files = files };
DEFINE_WAIT(wait);
int inflight;
@@ -8958,13 +8991,7 @@ static void io_uring_cancel_files(struct io_ring_ctx *ctx,
if (!inflight)
break;
- io_wq_cancel_cb(ctx->io_wq, io_cancel_task_cb, &cancel, true);
- io_poll_remove_all(ctx, task, files);
- io_kill_timeouts(ctx, task, files);
- io_cqring_overflow_flush(ctx, true, task, files);
- /* cancellations _may_ trigger task work */
- io_run_task_work();
-
+ io_uring_try_cancel_requests(ctx, task, files);
prepare_to_wait(&task->io_uring->wait, &wait,
TASK_UNINTERRUPTIBLE);
if (inflight == io_uring_count_inflight(ctx, task, files))
@@ -8973,37 +9000,6 @@ static void io_uring_cancel_files(struct io_ring_ctx *ctx,
}
}
-static void __io_uring_cancel_task_requests(struct io_ring_ctx *ctx,
- struct task_struct *task)
-{
- while (1) {
- struct io_task_cancel cancel = { .task = task, .files = NULL, };
- enum io_wq_cancel cret;
- bool ret = false;
-
- if (ctx->io_wq) {
- cret = io_wq_cancel_cb(ctx->io_wq, io_cancel_task_cb,
- &cancel, true);
- ret |= (cret != IO_WQ_CANCEL_NOTFOUND);
- }
-
- /* SQPOLL thread does its own polling */
- if (!(ctx->flags & IORING_SETUP_SQPOLL)) {
- while (!list_empty_careful(&ctx->iopoll_list)) {
- io_iopoll_try_reap_events(ctx);
- ret = true;
- }
- }
-
- ret |= io_poll_remove_all(ctx, task, NULL);
- ret |= io_kill_timeouts(ctx, task, NULL);
- ret |= io_run_task_work();
- if (!ret)
- break;
- cond_resched();
- }
-}
-
static void io_disable_sqo_submit(struct io_ring_ctx *ctx)
{
mutex_lock(&ctx->uring_lock);
@@ -9033,11 +9029,10 @@ static void io_uring_cancel_task_requests(struct io_ring_ctx *ctx,
}
io_cancel_defer_files(ctx, task, files);
- io_cqring_overflow_flush(ctx, true, task, files);
io_uring_cancel_files(ctx, task, files);
if (!files)
- __io_uring_cancel_task_requests(ctx, task);
+ io_uring_try_cancel_requests(ctx, task, NULL);
if ((ctx->flags & IORING_SETUP_SQPOLL) && ctx->sq_data) {
atomic_dec(&task->io_uring->in_idle);
--
2.24.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/8] io_uring: refactor scheduling in io_cqring_wait
2021-02-02 0:21 [PATCH 0/8] a second pack of 5.12 cleanups Pavel Begunkov
2021-02-02 0:21 ` [PATCH 1/8] io_uring: deduplicate core cancellations sequence Pavel Begunkov
@ 2021-02-02 0:21 ` Pavel Begunkov
2021-02-02 0:21 ` [PATCH 3/8] io_uring: refactor io_cqring_wait Pavel Begunkov
` (6 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Pavel Begunkov @ 2021-02-02 0:21 UTC (permalink / raw)
To: Jens Axboe, io-uring
schedule_timeout() with timeout=MAX_SCHEDULE_TIMEOUT is guaranteed to
work just as schedule(), so instead of hand-coding it based on arguments
always use the timeout version and simplify code.
Signed-off-by: Pavel Begunkov <[email protected]>
---
fs/io_uring.c | 19 ++++++++-----------
1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index a750c504366d..5b735635b8f0 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -7213,9 +7213,8 @@ static int io_cqring_wait(struct io_ring_ctx *ctx, int min_events,
.to_wait = min_events,
};
struct io_rings *rings = ctx->rings;
- struct timespec64 ts;
- signed long timeout = 0;
- int ret = 0;
+ signed long timeout = MAX_SCHEDULE_TIMEOUT;
+ int ret;
do {
io_cqring_overflow_flush(ctx, false, NULL, NULL);
@@ -7239,6 +7238,8 @@ static int io_cqring_wait(struct io_ring_ctx *ctx, int min_events,
}
if (uts) {
+ struct timespec64 ts;
+
if (get_timespec64(&ts, uts))
return -EFAULT;
timeout = timespec64_to_jiffies(&ts);
@@ -7264,14 +7265,10 @@ static int io_cqring_wait(struct io_ring_ctx *ctx, int min_events,
finish_wait(&ctx->wait, &iowq.wq);
continue;
}
- if (uts) {
- timeout = schedule_timeout(timeout);
- if (timeout == 0) {
- ret = -ETIME;
- break;
- }
- } else {
- schedule();
+ timeout = schedule_timeout(timeout);
+ if (timeout == 0) {
+ ret = -ETIME;
+ break;
}
} while (1);
finish_wait(&ctx->wait, &iowq.wq);
--
2.24.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/8] io_uring: refactor io_cqring_wait
2021-02-02 0:21 [PATCH 0/8] a second pack of 5.12 cleanups Pavel Begunkov
2021-02-02 0:21 ` [PATCH 1/8] io_uring: deduplicate core cancellations sequence Pavel Begunkov
2021-02-02 0:21 ` [PATCH 2/8] io_uring: refactor scheduling in io_cqring_wait Pavel Begunkov
@ 2021-02-02 0:21 ` Pavel Begunkov
2021-02-02 0:21 ` [PATCH 4/8] io_uring: refactor io_read for unsupported nowait Pavel Begunkov
` (5 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Pavel Begunkov @ 2021-02-02 0:21 UTC (permalink / raw)
To: Jens Axboe, io-uring
It's easy to make a mistake in io_cqring_wait() because for all
break/continue clauses we need to watch for prepare/finish_wait to be
used correctly. Extract all those into a new helper
io_cqring_wait_schedule(), and transforming the loop into simple series
of func calls: prepare(); check_and_schedule(); finish();
Signed-off-by: Pavel Begunkov <[email protected]>
---
fs/io_uring.c | 43 ++++++++++++++++++++++---------------------
1 file changed, 22 insertions(+), 21 deletions(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 5b735635b8f0..dcb9e937daa3 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -7195,6 +7195,25 @@ static int io_run_task_work_sig(void)
return -EINTR;
}
+/* when returns >0, the caller should retry */
+static inline int io_cqring_wait_schedule(struct io_ring_ctx *ctx,
+ struct io_wait_queue *iowq,
+ signed long *timeout)
+{
+ int ret;
+
+ /* make sure we run task_work before checking for signals */
+ ret = io_run_task_work_sig();
+ if (ret || io_should_wake(iowq))
+ return ret;
+ /* let the caller flush overflows, retry */
+ if (test_bit(0, &ctx->cq_check_overflow))
+ return 1;
+
+ *timeout = schedule_timeout(*timeout);
+ return !*timeout ? -ETIME : 1;
+}
+
/*
* Wait until events become available, if we don't already have some. The
* application must reap them itself, as they reside on the shared cq ring.
@@ -7251,27 +7270,9 @@ static int io_cqring_wait(struct io_ring_ctx *ctx, int min_events,
io_cqring_overflow_flush(ctx, false, NULL, NULL);
prepare_to_wait_exclusive(&ctx->wait, &iowq.wq,
TASK_INTERRUPTIBLE);
- /* make sure we run task_work before checking for signals */
- ret = io_run_task_work_sig();
- if (ret > 0) {
- finish_wait(&ctx->wait, &iowq.wq);
- continue;
- }
- else if (ret < 0)
- break;
- if (io_should_wake(&iowq))
- break;
- if (test_bit(0, &ctx->cq_check_overflow)) {
- finish_wait(&ctx->wait, &iowq.wq);
- continue;
- }
- timeout = schedule_timeout(timeout);
- if (timeout == 0) {
- ret = -ETIME;
- break;
- }
- } while (1);
- finish_wait(&ctx->wait, &iowq.wq);
+ ret = io_cqring_wait_schedule(ctx, &iowq, &timeout);
+ finish_wait(&ctx->wait, &iowq.wq);
+ } while (ret > 0);
restore_saved_sigmask_unless(ret == -EINTR);
--
2.24.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 4/8] io_uring: refactor io_read for unsupported nowait
2021-02-02 0:21 [PATCH 0/8] a second pack of 5.12 cleanups Pavel Begunkov
` (2 preceding siblings ...)
2021-02-02 0:21 ` [PATCH 3/8] io_uring: refactor io_cqring_wait Pavel Begunkov
@ 2021-02-02 0:21 ` Pavel Begunkov
2021-02-02 0:21 ` [PATCH 5/8] io_uring: further simplify do_read error parsing Pavel Begunkov
` (4 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Pavel Begunkov @ 2021-02-02 0:21 UTC (permalink / raw)
To: Jens Axboe, io-uring
!io_file_supports_async() case of io_read() is hard to read, it jumps
somewhere in the middle of the function just to do async setup and fail
on a similar check. Call io_setup_async_rw() directly for this case,
it's much easier to follow.
Signed-off-by: Pavel Begunkov <[email protected]>
---
fs/io_uring.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index dcb9e937daa3..866e0ea83dbe 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -3506,7 +3506,6 @@ static int io_read(struct io_kiocb *req, bool force_nonblock,
struct iov_iter __iter, *iter = &__iter;
struct io_async_rw *rw = req->async_data;
ssize_t io_size, ret, ret2;
- bool no_async;
if (rw) {
iter = &rw->iter;
@@ -3527,9 +3526,12 @@ static int io_read(struct io_kiocb *req, bool force_nonblock,
kiocb->ki_flags |= IOCB_NOWAIT;
/* If the file doesn't support async, just async punt */
- no_async = force_nonblock && !io_file_supports_async(req->file, READ);
- if (no_async)
- goto copy_iov;
+ if (force_nonblock && !io_file_supports_async(req->file, READ)) {
+ ret = io_setup_async_rw(req, iovec, inline_vecs, iter, true);
+ if (!ret)
+ return -EAGAIN;
+ goto out_free;
+ }
ret = rw_verify_area(READ, req->file, io_kiocb_ppos(kiocb), io_size);
if (unlikely(ret))
@@ -3568,8 +3570,6 @@ static int io_read(struct io_kiocb *req, bool force_nonblock,
ret = ret2;
goto out_free;
}
- if (no_async)
- return -EAGAIN;
rw = req->async_data;
/* it's copied and will be cleaned with ->io */
iovec = NULL;
--
2.24.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 5/8] io_uring: further simplify do_read error parsing
2021-02-02 0:21 [PATCH 0/8] a second pack of 5.12 cleanups Pavel Begunkov
` (3 preceding siblings ...)
2021-02-02 0:21 ` [PATCH 4/8] io_uring: refactor io_read for unsupported nowait Pavel Begunkov
@ 2021-02-02 0:21 ` Pavel Begunkov
2021-02-02 0:21 ` [PATCH 6/8] io_uring: let io_setup_async_rw take care of iovec Pavel Begunkov
` (3 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Pavel Begunkov @ 2021-02-02 0:21 UTC (permalink / raw)
To: Jens Axboe, io-uring
First, instead of checking iov_iter_count(iter) for 0 to find out that
all needed bytes were read, just compare returned code against io_size.
It's more reliable and arguably cleaner.
Also, place the half-read case into an else branch and delete an extra
label.
Signed-off-by: Pavel Begunkov <[email protected]>
---
fs/io_uring.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 866e0ea83dbe..1d1fa1f77332 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -3552,19 +3552,18 @@ static int io_read(struct io_kiocb *req, bool force_nonblock,
/* some cases will consume bytes even on error returns */
iov_iter_revert(iter, io_size - iov_iter_count(iter));
ret = 0;
- goto copy_iov;
- } else if (ret <= 0) {
+ } else if (ret <= 0 || ret == io_size) {
/* make sure -ERESTARTSYS -> -EINTR is done */
goto done;
- }
+ } else {
+ /* we did blocking attempt. no retry. */
+ if (!force_nonblock || (req->file->f_flags & O_NONBLOCK) ||
+ !(req->flags & REQ_F_ISREG))
+ goto done;
- /* read it all, or we did blocking attempt. no retry. */
- if (!iov_iter_count(iter) || !force_nonblock ||
- (req->file->f_flags & O_NONBLOCK) || !(req->flags & REQ_F_ISREG))
- goto done;
+ io_size -= ret;
+ }
- io_size -= ret;
-copy_iov:
ret2 = io_setup_async_rw(req, iovec, inline_vecs, iter, true);
if (ret2) {
ret = ret2;
--
2.24.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 6/8] io_uring: let io_setup_async_rw take care of iovec
2021-02-02 0:21 [PATCH 0/8] a second pack of 5.12 cleanups Pavel Begunkov
` (4 preceding siblings ...)
2021-02-02 0:21 ` [PATCH 5/8] io_uring: further simplify do_read error parsing Pavel Begunkov
@ 2021-02-02 0:21 ` Pavel Begunkov
2021-02-02 0:21 ` [PATCH 7/8] io_uring: don't forget to adjust io_size Pavel Begunkov
` (2 subsequent siblings)
8 siblings, 0 replies; 10+ messages in thread
From: Pavel Begunkov @ 2021-02-02 0:21 UTC (permalink / raw)
To: Jens Axboe, io-uring
Now we give out ownership of iovec into io_setup_async_rw(), so it
either sets request's context right or frees the iovec on error itself.
Makes our life a bit easier at call sites.
Signed-off-by: Pavel Begunkov <[email protected]>
---
fs/io_uring.c | 24 +++++++++---------------
1 file changed, 9 insertions(+), 15 deletions(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 1d1fa1f77332..f8492d62b6a1 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -2721,11 +2721,7 @@ static bool io_resubmit_prep(struct io_kiocb *req)
ret = io_import_iovec(rw, req, &iovec, &iter, false);
if (ret < 0)
return false;
- ret = io_setup_async_rw(req, iovec, inline_vecs, &iter, false);
- if (!ret)
- return true;
- kfree(iovec);
- return false;
+ return !io_setup_async_rw(req, iovec, inline_vecs, &iter, false);
}
#endif
@@ -3366,8 +3362,10 @@ static int io_setup_async_rw(struct io_kiocb *req, const struct iovec *iovec,
if (!force && !io_op_defs[req->opcode].needs_async_data)
return 0;
if (!req->async_data) {
- if (__io_alloc_async_data(req))
+ if (__io_alloc_async_data(req)) {
+ kfree(iovec);
return -ENOMEM;
+ }
io_req_map_rw(req, iovec, fast_iov, iter);
}
@@ -3528,9 +3526,7 @@ static int io_read(struct io_kiocb *req, bool force_nonblock,
/* If the file doesn't support async, just async punt */
if (force_nonblock && !io_file_supports_async(req->file, READ)) {
ret = io_setup_async_rw(req, iovec, inline_vecs, iter, true);
- if (!ret)
- return -EAGAIN;
- goto out_free;
+ return ret ?: -EAGAIN;
}
ret = rw_verify_area(READ, req->file, io_kiocb_ppos(kiocb), io_size);
@@ -3565,10 +3561,9 @@ static int io_read(struct io_kiocb *req, bool force_nonblock,
}
ret2 = io_setup_async_rw(req, iovec, inline_vecs, iter, true);
- if (ret2) {
- ret = ret2;
- goto out_free;
- }
+ if (ret2)
+ return ret2;
+
rw = req->async_data;
/* it's copied and will be cleaned with ->io */
iovec = NULL;
@@ -3703,8 +3698,7 @@ static int io_write(struct io_kiocb *req, bool force_nonblock,
/* some cases will consume bytes even on error returns */
iov_iter_revert(iter, io_size - iov_iter_count(iter));
ret = io_setup_async_rw(req, iovec, inline_vecs, iter, false);
- if (!ret)
- return -EAGAIN;
+ return ret ?: -EAGAIN;
}
out_free:
/* it's reportedly faster than delegating the null check to kfree() */
--
2.24.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 7/8] io_uring: don't forget to adjust io_size
2021-02-02 0:21 [PATCH 0/8] a second pack of 5.12 cleanups Pavel Begunkov
` (5 preceding siblings ...)
2021-02-02 0:21 ` [PATCH 6/8] io_uring: let io_setup_async_rw take care of iovec Pavel Begunkov
@ 2021-02-02 0:21 ` Pavel Begunkov
2021-02-02 0:21 ` [PATCH 8/8] io_uring: inline io_read()'s iovec freeing Pavel Begunkov
2021-02-04 13:52 ` [PATCH 0/8] a second pack of 5.12 cleanups Pavel Begunkov
8 siblings, 0 replies; 10+ messages in thread
From: Pavel Begunkov @ 2021-02-02 0:21 UTC (permalink / raw)
To: Jens Axboe, io-uring
We have invariant in io_read() of how much we're trying to read spilled
into an iter and io_size variable. The last one controls decision making
about whether to do read-retries. However, io_size is modified only
after the first read attempt, so if we happen to go for a third retry in
a single call to io_read(), we will get io_size greater than in the
iterator, so may lead to various side effects up to live-locking.
Modify io_size each time.
Signed-off-by: Pavel Begunkov <[email protected]>
---
fs/io_uring.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index f8492d62b6a1..3e648c0e6b8d 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -3551,13 +3551,10 @@ static int io_read(struct io_kiocb *req, bool force_nonblock,
} else if (ret <= 0 || ret == io_size) {
/* make sure -ERESTARTSYS -> -EINTR is done */
goto done;
- } else {
+ } else if (!force_nonblock || (req->file->f_flags & O_NONBLOCK) ||
+ !(req->flags & REQ_F_ISREG)) {
/* we did blocking attempt. no retry. */
- if (!force_nonblock || (req->file->f_flags & O_NONBLOCK) ||
- !(req->flags & REQ_F_ISREG))
- goto done;
-
- io_size -= ret;
+ goto done;
}
ret2 = io_setup_async_rw(req, iovec, inline_vecs, iter, true);
@@ -3570,6 +3567,7 @@ static int io_read(struct io_kiocb *req, bool force_nonblock,
/* now use our persistent iterator, if we aren't already */
iter = &rw->iter;
retry:
+ io_size -= ret;
rw->bytes_done += ret;
/* if we can retry, do so with the callbacks armed */
if (!io_rw_should_retry(req)) {
--
2.24.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 8/8] io_uring: inline io_read()'s iovec freeing
2021-02-02 0:21 [PATCH 0/8] a second pack of 5.12 cleanups Pavel Begunkov
` (6 preceding siblings ...)
2021-02-02 0:21 ` [PATCH 7/8] io_uring: don't forget to adjust io_size Pavel Begunkov
@ 2021-02-02 0:21 ` Pavel Begunkov
2021-02-04 13:52 ` [PATCH 0/8] a second pack of 5.12 cleanups Pavel Begunkov
8 siblings, 0 replies; 10+ messages in thread
From: Pavel Begunkov @ 2021-02-02 0:21 UTC (permalink / raw)
To: Jens Axboe, io-uring
io_read() has not the simpliest control flow with a lot of jumps and
it's hard to read. One of those is a out_free: label, which frees iovec.
However, from the middle of io_read() iovec is NULL'ed and so
kfree(iovec) is no-op, it leaves us with two place where we can inline
it and further clean up the code.
Signed-off-by: Pavel Begunkov <[email protected]>
---
fs/io_uring.c | 31 +++++++++++++------------------
1 file changed, 13 insertions(+), 18 deletions(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 3e648c0e6b8d..17a3736661c6 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -3530,14 +3530,18 @@ static int io_read(struct io_kiocb *req, bool force_nonblock,
}
ret = rw_verify_area(READ, req->file, io_kiocb_ppos(kiocb), io_size);
- if (unlikely(ret))
- goto out_free;
+ if (unlikely(ret)) {
+ kfree(iovec);
+ return ret;
+ }
ret = io_iter_do_read(req, iter);
if (ret == -EIOCBQUEUED) {
- ret = 0;
- goto out_free;
+ /* it's faster to check here then delegate to kfree */
+ if (iovec)
+ kfree(iovec);
+ return 0;
} else if (ret == -EAGAIN) {
/* IOPOLL retry should happen for io-wq threads */
if (!force_nonblock && !(req->ctx->flags & IORING_SETUP_IOPOLL))
@@ -3562,8 +3566,6 @@ static int io_read(struct io_kiocb *req, bool force_nonblock,
return ret2;
rw = req->async_data;
- /* it's copied and will be cleaned with ->io */
- iovec = NULL;
/* now use our persistent iterator, if we aren't already */
iter = &rw->iter;
retry:
@@ -3582,21 +3584,14 @@ static int io_read(struct io_kiocb *req, bool force_nonblock,
* do, then just retry at the new offset.
*/
ret = io_iter_do_read(req, iter);
- if (ret == -EIOCBQUEUED) {
- ret = 0;
- goto out_free;
- } else if (ret > 0 && ret < io_size) {
- /* we got some bytes, but not all. retry. */
+ if (ret == -EIOCBQUEUED)
+ return 0;
+ /* we got some bytes, but not all. retry. */
+ if (ret > 0 && ret < io_size)
goto retry;
- }
done:
kiocb_done(kiocb, ret, cs);
- ret = 0;
-out_free:
- /* it's reportedly faster than delegating the null check to kfree() */
- if (iovec)
- kfree(iovec);
- return ret;
+ return 0;
}
static int io_write_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
--
2.24.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 0/8] a second pack of 5.12 cleanups
2021-02-02 0:21 [PATCH 0/8] a second pack of 5.12 cleanups Pavel Begunkov
` (7 preceding siblings ...)
2021-02-02 0:21 ` [PATCH 8/8] io_uring: inline io_read()'s iovec freeing Pavel Begunkov
@ 2021-02-04 13:52 ` Pavel Begunkov
8 siblings, 0 replies; 10+ messages in thread
From: Pavel Begunkov @ 2021-02-04 13:52 UTC (permalink / raw)
To: Jens Axboe, io-uring
On 02/02/2021 00:21, Pavel Begunkov wrote:
> Those are a bit harder to look through.
>
> 4-8 are trying to simplify io_read(). 7/8 looks like a real problem, but
> not sure if even can happen with this IOCB_WAITQ set.
extended v2 has been sent
>
> Pavel Begunkov (8):
> io_uring: deduplicate core cancellations sequence
> io_uring: refactor scheduling in io_cqring_wait
> io_uring: refactor io_cqring_wait
> io_uring: refactor io_read for unsupported nowait
> io_uring: further simplify do_read error parsing
> io_uring: let io_setup_async_rw take care of iovec
> io_uring: don't forget to adjust io_size
> io_uring: inline io_read()'s iovec freeing
>
> fs/io_uring.c | 215 +++++++++++++++++++++++---------------------------
> 1 file changed, 97 insertions(+), 118 deletions(-)
>
--
Pavel Begunkov
^ permalink raw reply [flat|nested] 10+ messages in thread