public inbox for [email protected]
 help / color / mirror / Atom feed
* [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

* [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 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 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

* 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

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