On 10/10/19 9:41 PM, Jackie Liu wrote: > > >> 2019年10月11日 11:34，Jens Axboe <[email protected]> 写道： >> >> On 10/10/19 9:27 PM, Jackie Liu wrote: >>> >>> >>>> 2019年10月11日 11:17，Jens Axboe <[email protected]> 写道： >>>> >>>> On 10/10/19 9:06 PM, Jackie Liu wrote: >>>>> >>>>> >>>>>> 2019年10月11日 10:35，Jens Axboe <[email protected]> 写道： >>>>>> >>>>>> 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 <[email protected]> >>>>>>>> --- >>>>>>>> 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? -- Jens Axboe
