* [PATCH 1/1] io_uring: replace inflight_wait with tctx->wait
@ 2020-11-15 12:56 Pavel Begunkov
2020-11-16 16:33 ` Jens Axboe
0 siblings, 1 reply; 6+ messages in thread
From: Pavel Begunkov @ 2020-11-15 12:56 UTC (permalink / raw)
To: Jens Axboe, io-uring
As tasks now cancel only theirs requests, and inflight_wait is awaited
only in io_uring_cancel_files(), which should be called with ->in_idle
set, instead of keeping a separate inflight_wait use tctx->wait.
That will add some spurious wakeups but actually is safer from point of
not hanging the task.
e.g.
task1 | IRQ
| *start* io_complete_rw_common(link)
| link: req1 -> req2 -> req3(with files)
*cancel_files() |
io_wq_cancel(), etc. |
| put_req(link), adds to io-wq req2
schedule() |
So, task1 will never try to cancel req2 or req3. If req2 is
long-standing (e.g. read(empty_pipe)), this may hang.
Signed-off-by: Pavel Begunkov <[email protected]>
---
fs/io_uring.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index feb8e5bd2fb2..1a7ac86a0b92 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -286,7 +286,6 @@ struct io_ring_ctx {
struct list_head timeout_list;
struct list_head cq_overflow_list;
- wait_queue_head_t inflight_wait;
struct io_uring_sqe *sq_sqes;
} ____cacheline_aligned_in_smp;
@@ -1300,7 +1299,6 @@ static struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
INIT_LIST_HEAD(&ctx->iopoll_list);
INIT_LIST_HEAD(&ctx->defer_list);
INIT_LIST_HEAD(&ctx->timeout_list);
- init_waitqueue_head(&ctx->inflight_wait);
spin_lock_init(&ctx->inflight_lock);
INIT_LIST_HEAD(&ctx->inflight_list);
INIT_DELAYED_WORK(&ctx->file_put_work, io_file_put_work);
@@ -6081,12 +6079,13 @@ static int io_req_defer(struct io_kiocb *req, const struct io_uring_sqe *sqe)
static void io_req_drop_files(struct io_kiocb *req)
{
struct io_ring_ctx *ctx = req->ctx;
+ struct io_uring_task *tctx = req->task->io_uring;
unsigned long flags;
spin_lock_irqsave(&ctx->inflight_lock, flags);
list_del(&req->inflight_entry);
- if (waitqueue_active(&ctx->inflight_wait))
- wake_up(&ctx->inflight_wait);
+ if (atomic_read(&tctx->in_idle))
+ wake_up(&tctx->wait);
spin_unlock_irqrestore(&ctx->inflight_lock, flags);
req->flags &= ~REQ_F_INFLIGHT;
put_files_struct(req->work.identity->files);
@@ -8721,8 +8720,8 @@ static void io_uring_cancel_files(struct io_ring_ctx *ctx,
break;
}
if (found)
- prepare_to_wait(&ctx->inflight_wait, &wait,
- TASK_UNINTERRUPTIBLE);
+ prepare_to_wait(&task->io_uring->wait, &wait,
+ TASK_UNINTERRUPTIBLE);
spin_unlock_irq(&ctx->inflight_lock);
/* We need to keep going until we don't find a matching req */
@@ -8735,7 +8734,7 @@ static void io_uring_cancel_files(struct io_ring_ctx *ctx,
/* cancellations _may_ trigger task work */
io_run_task_work();
schedule();
- finish_wait(&ctx->inflight_wait, &wait);
+ finish_wait(&task->io_uring->wait, &wait);
}
}
--
2.24.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] io_uring: replace inflight_wait with tctx->wait
2020-11-15 12:56 [PATCH 1/1] io_uring: replace inflight_wait with tctx->wait Pavel Begunkov
@ 2020-11-16 16:33 ` Jens Axboe
2020-11-16 16:48 ` Pavel Begunkov
0 siblings, 1 reply; 6+ messages in thread
From: Jens Axboe @ 2020-11-16 16:33 UTC (permalink / raw)
To: Pavel Begunkov, io-uring
On 11/15/20 5:56 AM, Pavel Begunkov wrote:
> As tasks now cancel only theirs requests, and inflight_wait is awaited
> only in io_uring_cancel_files(), which should be called with ->in_idle
> set, instead of keeping a separate inflight_wait use tctx->wait.
>
> That will add some spurious wakeups but actually is safer from point of
> not hanging the task.
>
> e.g.
> task1 | IRQ
> | *start* io_complete_rw_common(link)
> | link: req1 -> req2 -> req3(with files)
> *cancel_files() |
> io_wq_cancel(), etc. |
> | put_req(link), adds to io-wq req2
> schedule() |
>
> So, task1 will never try to cancel req2 or req3. If req2 is
> long-standing (e.g. read(empty_pipe)), this may hang.
This looks like it's against 5.11, but also looks like we should add
it for 5.10?
--
Jens Axboe
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] io_uring: replace inflight_wait with tctx->wait
2020-11-16 16:33 ` Jens Axboe
@ 2020-11-16 16:48 ` Pavel Begunkov
2020-11-16 16:57 ` Jens Axboe
0 siblings, 1 reply; 6+ messages in thread
From: Pavel Begunkov @ 2020-11-16 16:48 UTC (permalink / raw)
To: Jens Axboe, io-uring
On 16/11/2020 16:33, Jens Axboe wrote:
> On 11/15/20 5:56 AM, Pavel Begunkov wrote:
>> As tasks now cancel only theirs requests, and inflight_wait is awaited
>> only in io_uring_cancel_files(), which should be called with ->in_idle
>> set, instead of keeping a separate inflight_wait use tctx->wait.
>>
>> That will add some spurious wakeups but actually is safer from point of
>> not hanging the task.
>>
>> e.g.
>> task1 | IRQ
>> | *start* io_complete_rw_common(link)
>> | link: req1 -> req2 -> req3(with files)
>> *cancel_files() |
>> io_wq_cancel(), etc. |
>> | put_req(link), adds to io-wq req2
>> schedule() |
>>
>> So, task1 will never try to cancel req2 or req3. If req2 is
>> long-standing (e.g. read(empty_pipe)), this may hang.
>
> This looks like it's against 5.11, but also looks like we should add
> it for 5.10?
Yeah, 5.10 completely slipped my mind, I'll resend
--
Pavel Begunkov
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] io_uring: replace inflight_wait with tctx->wait
2020-11-16 16:48 ` Pavel Begunkov
@ 2020-11-16 16:57 ` Jens Axboe
2020-11-16 17:16 ` Pavel Begunkov
0 siblings, 1 reply; 6+ messages in thread
From: Jens Axboe @ 2020-11-16 16:57 UTC (permalink / raw)
To: Pavel Begunkov, io-uring
On 11/16/20 9:48 AM, Pavel Begunkov wrote:
> On 16/11/2020 16:33, Jens Axboe wrote:
>> On 11/15/20 5:56 AM, Pavel Begunkov wrote:
>>> As tasks now cancel only theirs requests, and inflight_wait is awaited
>>> only in io_uring_cancel_files(), which should be called with ->in_idle
>>> set, instead of keeping a separate inflight_wait use tctx->wait.
>>>
>>> That will add some spurious wakeups but actually is safer from point of
>>> not hanging the task.
>>>
>>> e.g.
>>> task1 | IRQ
>>> | *start* io_complete_rw_common(link)
>>> | link: req1 -> req2 -> req3(with files)
>>> *cancel_files() |
>>> io_wq_cancel(), etc. |
>>> | put_req(link), adds to io-wq req2
>>> schedule() |
>>>
>>> So, task1 will never try to cancel req2 or req3. If req2 is
>>> long-standing (e.g. read(empty_pipe)), this may hang.
>>
>> This looks like it's against 5.11, but also looks like we should add
>> it for 5.10?
>
> Yeah, 5.10 completely slipped my mind, I'll resend
I applied it to 5.10, and fixed up the 5.11 side of things. So all good,
just wanted to confirm.
--
Jens Axboe
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] io_uring: replace inflight_wait with tctx->wait
2020-11-16 16:57 ` Jens Axboe
@ 2020-11-16 17:16 ` Pavel Begunkov
2020-11-16 20:42 ` Jens Axboe
0 siblings, 1 reply; 6+ messages in thread
From: Pavel Begunkov @ 2020-11-16 17:16 UTC (permalink / raw)
To: Jens Axboe, io-uring
On 16/11/2020 16:57, Jens Axboe wrote:
> On 11/16/20 9:48 AM, Pavel Begunkov wrote:
>> On 16/11/2020 16:33, Jens Axboe wrote:
>>> On 11/15/20 5:56 AM, Pavel Begunkov wrote:
>>>> As tasks now cancel only theirs requests, and inflight_wait is awaited
>>>> only in io_uring_cancel_files(), which should be called with ->in_idle
>>>> set, instead of keeping a separate inflight_wait use tctx->wait.
>>>>
>>>> That will add some spurious wakeups but actually is safer from point of
>>>> not hanging the task.
>>>>
>>>> e.g.
>>>> task1 | IRQ
>>>> | *start* io_complete_rw_common(link)
>>>> | link: req1 -> req2 -> req3(with files)
>>>> *cancel_files() |
>>>> io_wq_cancel(), etc. |
>>>> | put_req(link), adds to io-wq req2
>>>> schedule() |
>>>>
>>>> So, task1 will never try to cancel req2 or req3. If req2 is
>>>> long-standing (e.g. read(empty_pipe)), this may hang.
>>>
>>> This looks like it's against 5.11, but also looks like we should add
>>> it for 5.10?
>>
>> Yeah, 5.10 completely slipped my mind, I'll resend
>
> I applied it to 5.10, and fixed up the 5.11 side of things. So all good,
> just wanted to confirm.
Hmm, this won't work with 5.10, at least without
b7e7fb9960b03c ("io_uring: cancel only requests of current task")
https://git.kernel.dk/cgit/linux-block/commit/?h=for-5.11/io_uring&id=b7e7fb9960b03ca07866b5c016ac3ce5373ef207
That's because tctx->wait is kicked only by requests of current task,
but 5.10 cancels everything with specified ->files, including owned
by other tasks.
--
Pavel Begunkov
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] io_uring: replace inflight_wait with tctx->wait
2020-11-16 17:16 ` Pavel Begunkov
@ 2020-11-16 20:42 ` Jens Axboe
0 siblings, 0 replies; 6+ messages in thread
From: Jens Axboe @ 2020-11-16 20:42 UTC (permalink / raw)
To: Pavel Begunkov, io-uring
On 11/16/20 10:16 AM, Pavel Begunkov wrote:
> On 16/11/2020 16:57, Jens Axboe wrote:
>> On 11/16/20 9:48 AM, Pavel Begunkov wrote:
>>> On 16/11/2020 16:33, Jens Axboe wrote:
>>>> On 11/15/20 5:56 AM, Pavel Begunkov wrote:
>>>>> As tasks now cancel only theirs requests, and inflight_wait is awaited
>>>>> only in io_uring_cancel_files(), which should be called with ->in_idle
>>>>> set, instead of keeping a separate inflight_wait use tctx->wait.
>>>>>
>>>>> That will add some spurious wakeups but actually is safer from point of
>>>>> not hanging the task.
>>>>>
>>>>> e.g.
>>>>> task1 | IRQ
>>>>> | *start* io_complete_rw_common(link)
>>>>> | link: req1 -> req2 -> req3(with files)
>>>>> *cancel_files() |
>>>>> io_wq_cancel(), etc. |
>>>>> | put_req(link), adds to io-wq req2
>>>>> schedule() |
>>>>>
>>>>> So, task1 will never try to cancel req2 or req3. If req2 is
>>>>> long-standing (e.g. read(empty_pipe)), this may hang.
>>>>
>>>> This looks like it's against 5.11, but also looks like we should add
>>>> it for 5.10?
>>>
>>> Yeah, 5.10 completely slipped my mind, I'll resend
>>
>> I applied it to 5.10, and fixed up the 5.11 side of things. So all good,
>> just wanted to confirm.
>
> Hmm, this won't work with 5.10, at least without
>
> b7e7fb9960b03c ("io_uring: cancel only requests of current task")
> https://git.kernel.dk/cgit/linux-block/commit/?h=for-5.11/io_uring&id=b7e7fb9960b03ca07866b5c016ac3ce5373ef207
>
> That's because tctx->wait is kicked only by requests of current task,
> but 5.10 cancels everything with specified ->files, including owned
> by other tasks.
Ah good point, let's leave it 5.11 for now.
--
Jens Axboe
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-11-16 20:42 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-11-15 12:56 [PATCH 1/1] io_uring: replace inflight_wait with tctx->wait Pavel Begunkov
2020-11-16 16:33 ` Jens Axboe
2020-11-16 16:48 ` Pavel Begunkov
2020-11-16 16:57 ` Jens Axboe
2020-11-16 17:16 ` Pavel Begunkov
2020-11-16 20:42 ` Jens Axboe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox