Re: [PATCH] io_uring: fix duplicated increase of cached_cq_overflow

[Pavel Begunkov <asml.silence@gmail.com>]

On 11/15/2019 3:17 PM, Bob Liu wrote:
> On 11/15/19 5:49 PM, Pavel Begunkov wrote:
>> On 11/15/2019 12:37 PM, Bob Liu wrote:
>>> cached_cq_overflow already be increased in function
>>> io_cqring_overflow_flush().
>>>
>>> Signed-off-by: Bob Liu <bob.liu@oracle.com>
>>> ---
>>>  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 55f8b1d..eb23451 100644
>>> --- a/fs/io_uring.c
>>> +++ b/fs/io_uring.c
>>> @@ -701,7 +701,7 @@ static void io_cqring_fill_event(struct io_kiocb *req, long res)
>>>  		WRITE_ONCE(cqe->flags, 0);
>>>  	} else if (ctx->cq_overflow_flushed) {
>>>  		WRITE_ONCE(ctx->rings->cq_overflow,
>>> -				atomic_inc_return(&ctx->cached_cq_overflow));
>>> +				atomic_read(&ctx->cached_cq_overflow));
>>
>> Not really. It won't get into io_cqring_overflow_flush() if this branch
>> is executed. 
> 
> io_cqring_overflow_flush(force=true) must have been called when this branch is executed,
> since io_cqring_overflow_flush() is the only place can set 'ctx->cq_overflow_flushed' to true.
> 
Yes, it should have been called, but it sets this flag for the future
users of io_cqring_fill_event(), so any _new_ requests in
io_cqring_fill_event() will overflow instead of being added to
@overflow_list.

Either a request completes/overflows in io_cqring_fill_event(),
or it would be added to @overflow_list to be processed by
io_cqring_overflow_flush().


> And 'ctx->cached_cq_overflow' may already be increased in io_cqring_overflow_flush() if force is true and cqe==NULL.
> 
> static void io_cqring_overflow_flush(struct io_ring_ctx *ctx, bool force)
> {
> 	...
>         if (force)
>                 ctx->cq_overflow_flushed = true;
>                 
>         while (!list_empty(&ctx->cq_overflow_list)) {
>                 cqe = io_get_cqring(ctx);
>                 if (!cqe && !force)
>                         break;
>                         
>                 req = list_first_entry(&ctx->cq_overflow_list, struct io_kiocb,
>                                                 list);
>                 list_move(&req->list, &list);   
>                 if (cqe) {
> 			...
>                 } else {
>                         WRITE_ONCE(ctx->rings->cq_overflow,
>                                 atomic_inc_return(&ctx->cached_cq_overflow));
> 			  ^^^^^^^^^^^
> 			  ctx->cached_cq_overflow is increased if 'force=true' and 'ceq==NULL'.
> 
> 
> Did I miss anything?
> 
>> See, it's enqueued for overflow in "else" right below.
>>
>> i.e. list_add_tail(&req->list, &ctx->cq_overflow_list);
>>
>>>  	} else {
>>>  		refcount_inc(&req->refs);
>>>  		req->result = res;
>>>
>