[PATCH for-5.17 0/2] small fix and code clean

[PATCH for-5.17 0/2] small fix and code clean

[Hao Xu]

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

[PATCH 1/2] io_uring: fix no lock protection for ctx->cq_extra

[Hao Xu]

ctx->cq_extra should be protected by completion lock so that the
req_need_defer() does the right check.

Cc: stable@vger.kernel.org
Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
---
 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

Re: [PATCH 1/2] io_uring: fix no lock protection for ctx->cq_extra

[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: stable@vger.kernel.org
> Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
> ---
>   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

Re: [PATCH 1/2] io_uring: fix no lock protection for ctx->cq_extra

[Hao Xu]

在 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: stable@vger.kernel.org
>> Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
>> ---
>>   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.

[PATCH 2/2] io_uring: better to use REQ_F_IO_DRAIN for req->flags

[Hao Xu]

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 <haoxu@linux.alibaba.com>
---
 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

Re: [PATCH 2/2] io_uring: better to use REQ_F_IO_DRAIN for req->flags

[Pavel Begunkov]

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 <haoxu@linux.alibaba.com>
> ---
>   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

Re: [PATCH for-5.17 0/2] small fix and code clean

[Jens Axboe]

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