* [PATCH for-5.17 0/2] small fix and code clean @ 2021-11-25 9:21 Hao Xu 2021-11-25 9:21 ` [PATCH 1/2] io_uring: fix no lock protection for ctx->cq_extra Hao Xu ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Hao Xu @ 2021-11-25 9:21 UTC (permalink / raw) To: Jens Axboe; +Cc: io-uring, Pavel Begunkov, Joseph Qi Hao Xu (2): io_uring: fix no lock protection for ctx->cq_extra io_uring: better to use REQ_F_IO_DRAIN for req->flags fs/io_uring.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) -- 2.24.4 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] io_uring: fix no lock protection for ctx->cq_extra 2021-11-25 9:21 [PATCH for-5.17 0/2] small fix and code clean Hao Xu @ 2021-11-25 9:21 ` Hao Xu 2021-11-25 14:30 ` Pavel Begunkov 2021-11-25 9:21 ` [PATCH 2/2] io_uring: better to use REQ_F_IO_DRAIN for req->flags Hao Xu 2021-11-25 16:00 ` [PATCH for-5.17 0/2] small fix and code clean Jens Axboe 2 siblings, 1 reply; 7+ messages in thread From: Hao Xu @ 2021-11-25 9:21 UTC (permalink / raw) To: Jens Axboe; +Cc: io-uring, Pavel Begunkov, Joseph Qi ctx->cq_extra should be protected by completion lock so that the req_need_defer() does the right check. Cc: [email protected] Signed-off-by: Hao Xu <[email protected]> --- fs/io_uring.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/fs/io_uring.c b/fs/io_uring.c index f666a0e7f5e8..ae9534382b26 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -6537,12 +6537,15 @@ static __cold void io_drain_req(struct io_kiocb *req) u32 seq = io_get_sequence(req); /* Still need defer if there is pending req in defer list. */ + spin_lock(&ctx->completion_lock); if (!req_need_defer(req, seq) && list_empty_careful(&ctx->defer_list)) { + spin_unlock(&ctx->completion_lock); queue: ctx->drain_active = false; io_req_task_queue(req); return; } + spin_unlock(&ctx->completion_lock); ret = io_req_prep_async(req); if (ret) { -- 2.24.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] io_uring: fix no lock protection for ctx->cq_extra 2021-11-25 9:21 ` [PATCH 1/2] io_uring: fix no lock protection for ctx->cq_extra Hao Xu @ 2021-11-25 14:30 ` Pavel Begunkov 2021-11-26 3:29 ` Hao Xu 0 siblings, 1 reply; 7+ messages in thread From: Pavel Begunkov @ 2021-11-25 14:30 UTC (permalink / raw) To: Hao Xu, Jens Axboe; +Cc: io-uring, Joseph Qi On 11/25/21 09:21, Hao Xu wrote: > ctx->cq_extra should be protected by completion lock so that the > req_need_defer() does the right check. > > Cc: [email protected] > Signed-off-by: Hao Xu <[email protected]> > --- > fs/io_uring.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/fs/io_uring.c b/fs/io_uring.c > index f666a0e7f5e8..ae9534382b26 100644 > --- a/fs/io_uring.c > +++ b/fs/io_uring.c > @@ -6537,12 +6537,15 @@ static __cold void io_drain_req(struct io_kiocb *req) > u32 seq = io_get_sequence(req); > > /* Still need defer if there is pending req in defer list. */ > + spin_lock(&ctx->completion_lock); > if (!req_need_defer(req, seq) && list_empty_careful(&ctx->defer_list)) { > + spin_unlock(&ctx->completion_lock); I haven't checked the sync assumptions, but it was as this since the very beginning, so curious if you see any hangs or other problems? In any case, as drain is pushed to slow path, I'm all for simplifying synchronisation here. > queue: > ctx->drain_active = false; > io_req_task_queue(req); > return; > } > + spin_unlock(&ctx->completion_lock); > > ret = io_req_prep_async(req); > if (ret) { > -- Pavel Begunkov ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] io_uring: fix no lock protection for ctx->cq_extra 2021-11-25 14:30 ` Pavel Begunkov @ 2021-11-26 3:29 ` Hao Xu 0 siblings, 0 replies; 7+ messages in thread From: Hao Xu @ 2021-11-26 3:29 UTC (permalink / raw) To: Pavel Begunkov, Jens Axboe; +Cc: io-uring, Joseph Qi 在 2021/11/25 下午10:30, Pavel Begunkov 写道: > On 11/25/21 09:21, Hao Xu wrote: >> ctx->cq_extra should be protected by completion lock so that the >> req_need_defer() does the right check. >> >> Cc: [email protected] >> Signed-off-by: Hao Xu <[email protected]> >> --- >> fs/io_uring.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/fs/io_uring.c b/fs/io_uring.c >> index f666a0e7f5e8..ae9534382b26 100644 >> --- a/fs/io_uring.c >> +++ b/fs/io_uring.c >> @@ -6537,12 +6537,15 @@ static __cold void io_drain_req(struct >> io_kiocb *req) >> u32 seq = io_get_sequence(req); >> /* Still need defer if there is pending req in defer list. */ >> + spin_lock(&ctx->completion_lock); >> if (!req_need_defer(req, seq) && >> list_empty_careful(&ctx->defer_list)) { >> + spin_unlock(&ctx->completion_lock); > > I haven't checked the sync assumptions, but it was as this since > the very beginning, so curious if you see any hangs or other > problems? No, I just go over it in my mind: cq_extra and cached_cq_tail are both updated in one completion_lock critical section, lacking of lock may cause wrong values of cq_extra and cached_cq_tail and thus req_need_defer() return wrong result. For example, req_need_defer() see the updated cached_cq_tail but has the old cq_extra value. This is possible since io_rsrc_put_work() runs in system-worker. The result of lacking of lock is the drain request may be delayed a little bit more or less time. There is also a cq_extra-- in io_get_sqe(), which is the hot path, so I incline to not touch it. > > In any case, as drain is pushed to slow path, I'm all for > simplifying synchronisation here. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] io_uring: better to use REQ_F_IO_DRAIN for req->flags 2021-11-25 9:21 [PATCH for-5.17 0/2] small fix and code clean Hao Xu 2021-11-25 9:21 ` [PATCH 1/2] io_uring: fix no lock protection for ctx->cq_extra Hao Xu @ 2021-11-25 9:21 ` Hao Xu 2021-11-25 14:26 ` Pavel Begunkov 2021-11-25 16:00 ` [PATCH for-5.17 0/2] small fix and code clean Jens Axboe 2 siblings, 1 reply; 7+ messages in thread From: Hao Xu @ 2021-11-25 9:21 UTC (permalink / raw) To: Jens Axboe; +Cc: io-uring, Pavel Begunkov, Joseph Qi It's better to use REQ_F_IO_DRAIN for req->flags rather than IOSQE_IO_DRAIN though they have same value. Signed-off-by: Hao Xu <[email protected]> --- fs/io_uring.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/fs/io_uring.c b/fs/io_uring.c index ae9534382b26..08b1b3de9b3f 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -7095,10 +7095,10 @@ static void io_init_req_drain(struct io_kiocb *req) * If we need to drain a request in the middle of a link, drain * the head request and the next request/link after the current * link. Considering sequential execution of links, - * IOSQE_IO_DRAIN will be maintained for every request of our + * REQ_F_IO_DRAIN will be maintained for every request of our * link. */ - head->flags |= IOSQE_IO_DRAIN | REQ_F_FORCE_ASYNC; + head->flags |= REQ_F_IO_DRAIN | REQ_F_FORCE_ASYNC; ctx->drain_next = true; } } @@ -7149,7 +7149,7 @@ static int io_init_req(struct io_ring_ctx *ctx, struct io_kiocb *req, if (unlikely(ctx->drain_next) && !ctx->submit_state.link.head) { ctx->drain_next = false; ctx->drain_active = true; - req->flags |= IOSQE_IO_DRAIN | REQ_F_FORCE_ASYNC; + req->flags |= REQ_F_IO_DRAIN | REQ_F_FORCE_ASYNC; } } -- 2.24.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] io_uring: better to use REQ_F_IO_DRAIN for req->flags 2021-11-25 9:21 ` [PATCH 2/2] io_uring: better to use REQ_F_IO_DRAIN for req->flags Hao Xu @ 2021-11-25 14:26 ` Pavel Begunkov 0 siblings, 0 replies; 7+ messages in thread From: Pavel Begunkov @ 2021-11-25 14:26 UTC (permalink / raw) To: Hao Xu, Jens Axboe; +Cc: io-uring, Joseph Qi On 11/25/21 09:21, Hao Xu wrote: > It's better to use REQ_F_IO_DRAIN for req->flags rather than > IOSQE_IO_DRAIN though they have same value. > > Signed-off-by: Hao Xu <[email protected]> > --- > fs/io_uring.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/fs/io_uring.c b/fs/io_uring.c > index ae9534382b26..08b1b3de9b3f 100644 > --- a/fs/io_uring.c > +++ b/fs/io_uring.c > @@ -7095,10 +7095,10 @@ static void io_init_req_drain(struct io_kiocb *req) > * If we need to drain a request in the middle of a link, drain > * the head request and the next request/link after the current > * link. Considering sequential execution of links, > - * IOSQE_IO_DRAIN will be maintained for every request of our > + * REQ_F_IO_DRAIN will be maintained for every request of our Don't care much, but it's more about the userspace visible behaviour, and so talks about IOSQE_IO_DRAIN. Looks good, > * link. > */ > - head->flags |= IOSQE_IO_DRAIN | REQ_F_FORCE_ASYNC; > + head->flags |= REQ_F_IO_DRAIN | REQ_F_FORCE_ASYNC; > ctx->drain_next = true; > } > } > @@ -7149,7 +7149,7 @@ static int io_init_req(struct io_ring_ctx *ctx, struct io_kiocb *req, > if (unlikely(ctx->drain_next) && !ctx->submit_state.link.head) { > ctx->drain_next = false; > ctx->drain_active = true; > - req->flags |= IOSQE_IO_DRAIN | REQ_F_FORCE_ASYNC; > + req->flags |= REQ_F_IO_DRAIN | REQ_F_FORCE_ASYNC; > } > } > > -- Pavel Begunkov ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH for-5.17 0/2] small fix and code clean 2021-11-25 9:21 [PATCH for-5.17 0/2] small fix and code clean Hao Xu 2021-11-25 9:21 ` [PATCH 1/2] io_uring: fix no lock protection for ctx->cq_extra Hao Xu 2021-11-25 9:21 ` [PATCH 2/2] io_uring: better to use REQ_F_IO_DRAIN for req->flags Hao Xu @ 2021-11-25 16:00 ` Jens Axboe 2 siblings, 0 replies; 7+ messages in thread From: Jens Axboe @ 2021-11-25 16:00 UTC (permalink / raw) To: Hao Xu; +Cc: Joseph Qi, Pavel Begunkov, io-uring On Thu, 25 Nov 2021 17:21:01 +0800, Hao Xu wrote: > Hao Xu (2): > io_uring: fix no lock protection for ctx->cq_extra > io_uring: better to use REQ_F_IO_DRAIN for req->flags > > fs/io_uring.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > [...] Applied, thanks! [1/2] io_uring: fix no lock protection for ctx->cq_extra commit: e302f1046f4c209291b07ff7bc4d15ca26891f16 [2/2] io_uring: better to use REQ_F_IO_DRAIN for req->flags commit: b6c7db32183251204f124b10d6177d46558ca7b8 Best regards, -- Jens Axboe ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-11-26 3:31 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-11-25 9:21 [PATCH for-5.17 0/2] small fix and code clean Hao Xu 2021-11-25 9:21 ` [PATCH 1/2] io_uring: fix no lock protection for ctx->cq_extra Hao Xu 2021-11-25 14:30 ` Pavel Begunkov 2021-11-26 3:29 ` Hao Xu 2021-11-25 9:21 ` [PATCH 2/2] io_uring: better to use REQ_F_IO_DRAIN for req->flags Hao Xu 2021-11-25 14:26 ` Pavel Begunkov 2021-11-25 16:00 ` [PATCH for-5.17 0/2] small fix and code clean Jens Axboe
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox