> 2019年10月11日 11:34，Jens Axboe <axboe@xxxxxxxxx> 写道：
>
> On 10/10/19 9:27 PM, Jackie Liu wrote:
>>
>>
>>> 2019年10月11日 11:17，Jens Axboe <axboe@xxxxxxxxx> 写道：
>>>
>>> On 10/10/19 9:06 PM, Jackie Liu wrote:
>>>>
>>>>
>>>>> 2019年10月11日 10:35，Jens Axboe <axboe@xxxxxxxxx> 写道：
>>>>>
>>>>> 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@xxxxxxxxxx>
>>>>>>> ---
>>>>>>> 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.
--
BR, Jackie Liu
