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

[Bob Liu <bob.liu@oracle.com>]

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.

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;
>>