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. > Another problem, before this change, a timeout req can also drain the > sqe(io_queue_sqe->io_req_defer and add to refer list), i am not sure is > it a right or wrong logic, but your patch fix that. We should factor the cq_timeouts into that. -- Jens Axboe
