> 2019年10月11日 11:42，Jens Axboe <ax...@kernel.dk> 写道：
>
> On 10/10/19 9:41 PM, Jackie Liu wrote:
>>
>>
>>> 2019年10月11日 11:34，Jens Axboe <ax...@kernel.dk> 写道：
>>>
>>> On 10/10/19 9:27 PM, Jackie Liu wrote:
>>>>
>>>>
>>>>> 2019年10月11日 11:17，Jens Axboe <ax...@kernel.dk> 写道：
>>>>>
>>>>> On 10/10/19 9:06 PM, Jackie Liu wrote:
>>>>>>
>>>>>>
>>>>>>> 2019年10月11日 10:35，Jens Axboe <ax...@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 <liuyu...@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)
>>>
>>
>> Much better, clearly and easy understand.
>
> Agree, this is easier to read as well, and harder to inadvertently
> break. Can I add your reviewed-by to this one?
>
>
Yes, please, Reviewed-by: Jackie Liu <liuyu...@kylinos.cn>
--
BR, Jackie Liu
