None of the opcodes in io_uring_register use ring quiesce
anymore. Hence io_register_op_must_quiesce always returns
false and io_ctx_quiesce is never called.
Signed-off-by: Usama Arif <[email protected]>
---
fs/io_uring.c | 83 ---------------------------------------------------
1 file changed, 83 deletions(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index a2ce2601d4de..ad8f84376955 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1292,18 +1292,6 @@ static inline unsigned int io_put_kbuf(struct io_kiocb *req)
return __io_put_kbuf(req);
}
-static void io_refs_resurrect(struct percpu_ref *ref, struct completion *compl)
-{
- bool got = percpu_ref_tryget(ref);
-
- /* already at zero, wait for ->release() */
- if (!got)
- wait_for_completion(compl);
- percpu_ref_resurrect(ref);
- if (got)
- percpu_ref_put(ref);
-}
-
static bool io_match_task(struct io_kiocb *head, struct task_struct *task,
bool cancel_all)
__must_hold(&req->ctx->timeout_lock)
@@ -10993,66 +10981,6 @@ static __cold int io_register_iowq_max_workers(struct io_ring_ctx *ctx,
return ret;
}
-static bool io_register_op_must_quiesce(int op)
-{
- switch (op) {
- case IORING_REGISTER_BUFFERS:
- case IORING_UNREGISTER_BUFFERS:
- case IORING_REGISTER_FILES:
- case IORING_UNREGISTER_FILES:
- case IORING_REGISTER_FILES_UPDATE:
- case IORING_REGISTER_EVENTFD:
- case IORING_REGISTER_EVENTFD_ASYNC:
- case IORING_UNREGISTER_EVENTFD:
- case IORING_REGISTER_PROBE:
- case IORING_REGISTER_PERSONALITY:
- case IORING_UNREGISTER_PERSONALITY:
- case IORING_REGISTER_ENABLE_RINGS:
- case IORING_REGISTER_RESTRICTIONS:
- case IORING_REGISTER_FILES2:
- case IORING_REGISTER_FILES_UPDATE2:
- case IORING_REGISTER_BUFFERS2:
- case IORING_REGISTER_BUFFERS_UPDATE:
- case IORING_REGISTER_IOWQ_AFF:
- case IORING_UNREGISTER_IOWQ_AFF:
- case IORING_REGISTER_IOWQ_MAX_WORKERS:
- return false;
- default:
- return true;
- }
-}
-
-static __cold int io_ctx_quiesce(struct io_ring_ctx *ctx)
-{
- long ret;
-
- percpu_ref_kill(&ctx->refs);
-
- /*
- * Drop uring mutex before waiting for references to exit. If another
- * thread is currently inside io_uring_enter() it might need to grab the
- * uring_lock to make progress. If we hold it here across the drain
- * wait, then we can deadlock. It's safe to drop the mutex here, since
- * no new references will come in after we've killed the percpu ref.
- */
- mutex_unlock(&ctx->uring_lock);
- do {
- ret = wait_for_completion_interruptible_timeout(&ctx->ref_comp, HZ);
- if (ret) {
- ret = min(0L, ret);
- break;
- }
-
- ret = io_run_task_work_sig();
- io_req_caches_free(ctx);
- } while (ret >= 0);
- mutex_lock(&ctx->uring_lock);
-
- if (ret)
- io_refs_resurrect(&ctx->refs, &ctx->ref_comp);
- return ret;
-}
-
static int __io_uring_register(struct io_ring_ctx *ctx, unsigned opcode,
void __user *arg, unsigned nr_args)
__releases(ctx->uring_lock)
@@ -11076,12 +11004,6 @@ static int __io_uring_register(struct io_ring_ctx *ctx, unsigned opcode,
return -EACCES;
}
- if (io_register_op_must_quiesce(opcode)) {
- ret = io_ctx_quiesce(ctx);
- if (ret)
- return ret;
- }
-
switch (opcode) {
case IORING_REGISTER_BUFFERS:
ret = io_sqe_buffers_register(ctx, arg, nr_args, NULL);
@@ -11186,11 +11108,6 @@ static int __io_uring_register(struct io_ring_ctx *ctx, unsigned opcode,
break;
}
- if (io_register_op_must_quiesce(opcode)) {
- /* bring the ctx back to life */
- percpu_ref_reinit(&ctx->refs);
- reinit_completion(&ctx->ref_comp);
- }
return ret;
}
--
2.25.1
Hello.
On Fri, Feb 04, 2022 at 02:51:17PM +0000, Usama Arif <[email protected]> wrote:
> - percpu_ref_resurrect(ref);
> [...]
> - percpu_ref_reinit(&ctx->refs);
It seems to me that this patch could have also changed
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1911,7 +1911,7 @@ static __cold struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
ctx->dummy_ubuf->ubuf = -1UL;
if (percpu_ref_init(&ctx->refs, io_ring_ctx_ref_free,
- PERCPU_REF_ALLOW_REINIT, GFP_KERNEL))
+ 0, GFP_KERNEL))
goto err;
ctx->flags = p->flags;
Or are there any plans to still use the reinit/resurrect functionality
of the percpu counter?
Thanks,
Michal
On 7/15/22 9:44 AM, Michal Koutn? wrote:
> Hello.
>
> On Fri, Feb 04, 2022 at 02:51:17PM +0000, Usama Arif <[email protected]> wrote:
>> - percpu_ref_resurrect(ref);
>> [...]
>> - percpu_ref_reinit(&ctx->refs);
>
> It seems to me that this patch could have also changed
>
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -1911,7 +1911,7 @@ static __cold struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
> ctx->dummy_ubuf->ubuf = -1UL;
>
> if (percpu_ref_init(&ctx->refs, io_ring_ctx_ref_free,
> - PERCPU_REF_ALLOW_REINIT, GFP_KERNEL))
> + 0, GFP_KERNEL))
> goto err;
>
> ctx->flags = p->flags;
>
> Or are there any plans to still use the reinit/resurrect functionality
> of the percpu counter?
Ah yes indeed, good catch! Would you mind sending that as an actual
patch?
--
Jens Axboe
The commit 8bb649ee1da3 ("io_uring: remove ring quiesce for
io_uring_register") removed the worklow relying on reinit/resurrection
of the percpu_ref, hence, initialization with that requested is a relic.
This is based on code review, this causes no real bug (and theoretically
can't). Technically it's a revert of commit 214828962dea ("io_uring:
initialize percpu refcounters using PERCU_REF_ALLOW_REINIT") but since
the flag omission is now justified, I'm not making this a revert.
Fixes: 8bb649ee1da3 ("io_uring: remove ring quiesce for io_uring_register")
Signed-off-by: Michal Koutný <[email protected]>
---
fs/io_uring.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index a01ea49f3017..563f2266c674 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1911,7 +1911,7 @@ static __cold struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
ctx->dummy_ubuf->ubuf = -1UL;
if (percpu_ref_init(&ctx->refs, io_ring_ctx_ref_free,
- PERCPU_REF_ALLOW_REINIT, GFP_KERNEL))
+ 0, GFP_KERNEL))
goto err;
ctx->flags = p->flags;
--
2.37.0
On Fri, Jul 15, 2022 at 07:45:01PM +0200, Michal Koutny wrote:
> The commit 8bb649ee1da3 ("io_uring: remove ring quiesce for
> io_uring_register") removed the worklow relying on reinit/resurrection
> of the percpu_ref, hence, initialization with that requested is a relic.
>
> This is based on code review, this causes no real bug (and theoretically
> can't). Technically it's a revert of commit 214828962dea ("io_uring:
> initialize percpu refcounters using PERCU_REF_ALLOW_REINIT") but since
> the flag omission is now justified, I'm not making this a revert.
>
> Fixes: 8bb649ee1da3 ("io_uring: remove ring quiesce for io_uring_register")
> Signed-off-by: Michal Koutn? <[email protected]>
Acked-by: Roman Gushchin <[email protected]>
Thanks!
On 7/15/22 11:45 AM, Michal Koutn? wrote:
> The commit 8bb649ee1da3 ("io_uring: remove ring quiesce for
> io_uring_register") removed the worklow relying on reinit/resurrection
> of the percpu_ref, hence, initialization with that requested is a relic.
>
> This is based on code review, this causes no real bug (and theoretically
> can't). Technically it's a revert of commit 214828962dea ("io_uring:
> initialize percpu refcounters using PERCU_REF_ALLOW_REINIT") but since
> the flag omission is now justified, I'm not making this a revert.
Thanks, applied manually for 5.20 (new file location).
--
Jens Axboe