Re: [PATCH 1/2] io_uring: make the logic clearer for io_sequence_defer

[Jens Axboe <axboe@kernel.dk>]

On 10/10/19 9:27 PM, Jackie Liu wrote:
> 
> 
>> 2019年10月11日 11:17，Jens Axboe <axboe@kernel.dk> 写道：
>>
>> On 10/10/19 9:06 PM, Jackie Liu wrote:
>>>
>>>
>>>> 2019年10月11日 10:35，Jens Axboe <axboe@kernel.dk> 写道：
>>>>
>>>> On 10/10/19 8:24 PM, yangerkun wrote:
>>>>>
>>>>>
>>>>> On 2019/10/9 9:19, Jackie Liu wrote:
>>>>>> __io_get_deferred_req is used to get all defer lists, including defer_list
>>>>>> and timeout_list, but io_sequence_defer should be only cares about the sequence.
>>>>>>
>>>>>> Signed-off-by: Jackie Liu <liuyun01@kylinos.cn>
>>>>>> ---
>>>>>>    fs/io_uring.c | 13 +++++--------
>>>>>>    1 file changed, 5 insertions(+), 8 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>>>>>> index 8a0381f1a43b..8ec2443eb019 100644
>>>>>> --- a/fs/io_uring.c
>>>>>> +++ b/fs/io_uring.c
>>>>>> @@ -418,9 +418,7 @@ static struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
>>>>>>    static inline bool io_sequence_defer(struct io_ring_ctx *ctx,
>>>>>>    				     struct io_kiocb *req)
>>>>>>    {
>>>>>> -	/* timeout requests always honor sequence */
>>>>>> -	if (!(req->flags & REQ_F_TIMEOUT) &&
>>>>>> -	    (req->flags & (REQ_F_IO_DRAIN|REQ_F_IO_DRAINED)) != REQ_F_IO_DRAIN)
>>>>>> +	if ((req->flags & (REQ_F_IO_DRAIN|REQ_F_IO_DRAINED)) != REQ_F_IO_DRAIN)
>>>>>>    		return false;
>>>>>>
>>>>>>    	return req->sequence != ctx->cached_cq_tail + ctx->rings->sq_dropped;
>>>>>> @@ -435,12 +433,11 @@ static struct io_kiocb *__io_get_deferred_req(struct io_ring_ctx *ctx,
>>>>>>    		return NULL;
>>>>>>
>>>>>>    	req = list_first_entry(list, struct io_kiocb, list);
>>>>>> -	if (!io_sequence_defer(ctx, req)) {
>>>>>> -		list_del_init(&req->list);
>>>>>> -		return req;
>>>>>> -	}
>>>>>> +	if (!(req->flags & REQ_F_TIMEOUT) && io_sequence_defer(ctx, req))
>>>>>> +		return NULL;
>>>>> Hi,
>>>>>
>>>>> For timeout req, we should also compare the sequence to determine to
>>>>> return NULL or the req. But now we will return req directly. Actually,
>>>>> no need to compare req->flags with REQ_F_TIMEOUT.
>>>>
>>>> Yes, not sure how I missed this, the patch is broken as-is. I will kill
>>>> it, cleanup can be done differently.
>>>
>>> Sorry for miss it, I don't wanna change the logic, it's not my
>>> original meaning.
>>
>> No worries, mistakes happen.
>>
>>> Personal opinion, timeout list should not be mixed with defer_list,
>>> which makes the code more complicated and difficult to understand.
>>
>> Not sure why you feel they are mixed? They are in separate lists, but
>> they share using the sequence logic. In that respet they are really not
>> that different, the sequence will trigger either one of them. Either as
>> a timeout, or as a "can now be issued". Hence the code handling them is
>> both shared and identical.
> 
> I not sure, I think I need reread the code of timeout command.
> 
>>
>> I do agree that the check could be cleaner, which is probably how the
>> mistake in this patch happened in the first place.
>>
> 
> Yes, I agree with you. io_sequence_defer should be only cares about
> the sequence.  Thanks for point out this mistake.

How about something like this? More cleanly separates them to avoid
mixing flags. The regular defer code will honor the flags, the timeout
code will just bypass those.


diff --git a/fs/io_uring.c b/fs/io_uring.c
index c92cb09cc262..4a2bb81cb1f1 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -416,26 +416,29 @@ static struct io_ring_ctx *io_ring_ctx_alloc(struct io_uring_params *p)
 	return ctx;
 }
 
+static inline bool __io_sequence_defer(struct io_ring_ctx *ctx,
+				       struct io_kiocb *req)
+{
+	return req->sequence != ctx->cached_cq_tail + ctx->rings->sq_dropped;
+}
+
 static inline bool io_sequence_defer(struct io_ring_ctx *ctx,
 				     struct io_kiocb *req)
 {
-	/* timeout requests always honor sequence */
-	if (!(req->flags & REQ_F_TIMEOUT) &&
-	    (req->flags & (REQ_F_IO_DRAIN|REQ_F_IO_DRAINED)) != REQ_F_IO_DRAIN)
+	if ((req->flags & (REQ_F_IO_DRAIN|REQ_F_IO_DRAINED)) != REQ_F_IO_DRAIN)
 		return false;
 
-	return req->sequence != ctx->cached_cq_tail + ctx->rings->sq_dropped;
+	return __io_sequence_defer(ctx, req);
 }
 
-static struct io_kiocb *__io_get_deferred_req(struct io_ring_ctx *ctx,
-					      struct list_head *list)
+static struct io_kiocb *io_get_deferred_req(struct io_ring_ctx *ctx)
 {
 	struct io_kiocb *req;
 
-	if (list_empty(list))
+	if (list_empty(&ctx->defer_list))
 		return NULL;
 
-	req = list_first_entry(list, struct io_kiocb, list);
+	req = list_first_entry(&ctx->defer_list, struct io_kiocb, list);
 	if (!io_sequence_defer(ctx, req)) {
 		list_del_init(&req->list);
 		return req;
@@ -444,14 +447,20 @@ static struct io_kiocb *__io_get_deferred_req(struct io_ring_ctx *ctx,
 	return NULL;
 }
 
-static struct io_kiocb *io_get_deferred_req(struct io_ring_ctx *ctx)
-{
-	return __io_get_deferred_req(ctx, &ctx->defer_list);
-}
-
 static struct io_kiocb *io_get_timeout_req(struct io_ring_ctx *ctx)
 {
-	return __io_get_deferred_req(ctx, &ctx->timeout_list);
+	struct io_kiocb *req;
+
+	if (list_empty(&ctx->timeout_list))
+		return NULL;
+
+	req = list_first_entry(&ctx->timeout_list, struct io_kiocb, list);
+	if (!__io_sequence_defer(ctx, req)) {
+		list_del_init(&req->list);
+		return req;
+	}
+
+	return NULL;
 }
 
 static void __io_commit_cqring(struct io_ring_ctx *ctx)

-- 
Jens Axboe