* [PATCH 1/2] io_uring: fix issue when IOSQE_IO_DRAIN pass with IOSQE_IO_LINK @ 2019-08-14 9:35 Jackie Liu 2019-08-14 9:35 ` [PATCH 2/2] io_uring: fix an issue when IOSQE_IO_LINK is inserted into defer list Jackie Liu 2019-08-15 17:07 ` [PATCH 1/2] io_uring: fix issue when IOSQE_IO_DRAIN pass with IOSQE_IO_LINK Jens Axboe 0 siblings, 2 replies; 7+ messages in thread From: Jackie Liu @ 2019-08-14 9:35 UTC (permalink / raw) To: axboe; +Cc: linux-block, Jackie Liu Suppose there are three IOs here, and their order is as follows: Submit: [1] IO_LINK | |--- [2] IO_LINK | IO_DRAIN | |- [3] NORMAL_IO In theory, they all need to be inserted into the Link-list, but flag IO_DRAIN we have, io[2] and io[3] will be inserted into the defer_list, and finally, io[3] and io[2] will be processed at the same time. Now, it is directly forbidden to pass these two flags at the same time. Fixes: 9e645e1105c ("io_uring: add support for sqe links") Signed-off-by: Jackie Liu <liuyun01@kylinos.cn> --- fs/io_uring.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/fs/io_uring.c b/fs/io_uring.c index d542f1c..05ee628 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -2074,10 +2074,13 @@ static void io_submit_sqe(struct io_ring_ctx *ctx, struct sqe_submit *s, { struct io_uring_sqe *sqe_copy; struct io_kiocb *req; + unsigned int flags; int ret; + flags = READ_ONCE(s->sqe->flags); /* enforce forwards compatibility on users */ - if (unlikely(s->sqe->flags & ~SQE_VALID_FLAGS)) { + if (unlikely((flags & ~SQE_VALID_FLAGS) || + (flags & (IOSQE_IO_DRAIN | IOSQE_IO_LINK)))) { ret = -EINVAL; goto err; } @@ -2093,6 +2096,8 @@ static void io_submit_sqe(struct io_ring_ctx *ctx, struct sqe_submit *s, err_req: io_free_req(req); err: + if (*link) + io_fail_links(*link); io_cqring_add_event(ctx, s->sqe->user_data, ret); return; } -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] io_uring: fix an issue when IOSQE_IO_LINK is inserted into defer list 2019-08-14 9:35 [PATCH 1/2] io_uring: fix issue when IOSQE_IO_DRAIN pass with IOSQE_IO_LINK Jackie Liu @ 2019-08-14 9:35 ` Jackie Liu 2019-08-15 17:21 ` Jens Axboe 2019-08-15 17:07 ` [PATCH 1/2] io_uring: fix issue when IOSQE_IO_DRAIN pass with IOSQE_IO_LINK Jens Axboe 1 sibling, 1 reply; 7+ messages in thread From: Jackie Liu @ 2019-08-14 9:35 UTC (permalink / raw) To: axboe; +Cc: linux-block, Jackie Liu This patch may fix two issues: First, when IOSQE_IO_DARIN set, the next IOs need to be inserted into defer list to delay execution, but link io will be actively scheduled to run by calling io_queue_sqe. Second, when multiple LINK_IOs are inserted together with defer_list, the LINK_IO is no longer keep order. |-------------| | LINK_IO | ----> insert to defer_list ----------- |-------------| | | LINK_IO | ----> insert to defer_list ----------| |-------------| | | LINK_IO | ----> insert to defer_list ----------| |-------------| | | NORMAL_IO | ----> insert to defer_list ----------| |-------------| | | queue_work at same time <-----| Fixes: 9e645e1105c ("io_uring: add support for sqe links") Signed-off-by: Jackie Liu <liuyun01@kylinos.cn> --- fs/io_uring.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/fs/io_uring.c b/fs/io_uring.c index 05ee628..405134d 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -2025,6 +2025,15 @@ static int io_queue_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req, { int ret; + ret = io_req_defer(ctx, req, s->sqe); + if (ret) { + if (ret != -EIOCBQUEUED) { + io_free_req(req); + io_cqring_add_event(ctx, s->sqe->user_data, ret); + } + return 0; + } + ret = __io_submit_sqe(ctx, req, s, true); if (ret == -EAGAIN && !(req->flags & REQ_F_NOWAIT)) { struct io_uring_sqe *sqe_copy; @@ -2102,13 +2111,6 @@ static void io_submit_sqe(struct io_ring_ctx *ctx, struct sqe_submit *s, return; } - ret = io_req_defer(ctx, req, s->sqe); - if (ret) { - if (ret != -EIOCBQUEUED) - goto err_req; - return; - } - /* * If we already have a head request, queue this one for async * submittal once the head completes. If we don't have a head but -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] io_uring: fix an issue when IOSQE_IO_LINK is inserted into defer list 2019-08-14 9:35 ` [PATCH 2/2] io_uring: fix an issue when IOSQE_IO_LINK is inserted into defer list Jackie Liu @ 2019-08-15 17:21 ` Jens Axboe 0 siblings, 0 replies; 7+ messages in thread From: Jens Axboe @ 2019-08-15 17:21 UTC (permalink / raw) To: Jackie Liu; +Cc: linux-block On 8/14/19 3:35 AM, Jackie Liu wrote: > This patch may fix two issues: > > First, when IOSQE_IO_DARIN set, the next IOs need to be inserted into defer > list to delay execution, but link io will be actively scheduled to run by > calling io_queue_sqe. > > Second, when multiple LINK_IOs are inserted together with defer_list, the > LINK_IO is no longer keep order. > > |-------------| > | LINK_IO | ----> insert to defer_list ----------- > |-------------| | > | LINK_IO | ----> insert to defer_list ----------| > |-------------| | > | LINK_IO | ----> insert to defer_list ----------| > |-------------| | > | NORMAL_IO | ----> insert to defer_list ----------| > |-------------| | > | > queue_work at same time <-----| Looks good, applied. -- Jens Axboe ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] io_uring: fix issue when IOSQE_IO_DRAIN pass with IOSQE_IO_LINK 2019-08-14 9:35 [PATCH 1/2] io_uring: fix issue when IOSQE_IO_DRAIN pass with IOSQE_IO_LINK Jackie Liu 2019-08-14 9:35 ` [PATCH 2/2] io_uring: fix an issue when IOSQE_IO_LINK is inserted into defer list Jackie Liu @ 2019-08-15 17:07 ` Jens Axboe 2019-08-16 0:48 ` Jackie Liu 1 sibling, 1 reply; 7+ messages in thread From: Jens Axboe @ 2019-08-15 17:07 UTC (permalink / raw) To: Jackie Liu; +Cc: linux-block On 8/14/19 3:35 AM, Jackie Liu wrote: > Suppose there are three IOs here, and their order is as follows: > > Submit: > [1] IO_LINK > | > |--- [2] IO_LINK | IO_DRAIN > | > |- [3] NORMAL_IO > > In theory, they all need to be inserted into the Link-list, but flag > IO_DRAIN we have, io[2] and io[3] will be inserted into the defer_list, > and finally, io[3] and io[2] will be processed at the same time. > > Now, it is directly forbidden to pass these two flags at the same time. > > Fixes: 9e645e1105c ("io_uring: add support for sqe links") > Signed-off-by: Jackie Liu <liuyun01@kylinos.cn> > --- > fs/io_uring.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/fs/io_uring.c b/fs/io_uring.c > index d542f1c..05ee628 100644 > --- a/fs/io_uring.c > +++ b/fs/io_uring.c > @@ -2074,10 +2074,13 @@ static void io_submit_sqe(struct io_ring_ctx *ctx, struct sqe_submit *s, > { > struct io_uring_sqe *sqe_copy; > struct io_kiocb *req; > + unsigned int flags; > int ret; > > + flags = READ_ONCE(s->sqe->flags); > /* enforce forwards compatibility on users */ > - if (unlikely(s->sqe->flags & ~SQE_VALID_FLAGS)) { > + if (unlikely((flags & ~SQE_VALID_FLAGS) || > + (flags & (IOSQE_IO_DRAIN | IOSQE_IO_LINK)))) { This doesn't look right, as any setting of either DRAIN or LINK would now fail? Did you mean something ala: if ((flags & (IOSQE_IO_DRAIN | IOSQE_IO_LINK)) == (IOSQE_IO_DRAIN | IOSQE_IO_LINK)) { ... fail ... } which makes me worried that you didn't test this at all... -- Jens Axboe ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] io_uring: fix issue when IOSQE_IO_DRAIN pass with IOSQE_IO_LINK 2019-08-15 17:07 ` [PATCH 1/2] io_uring: fix issue when IOSQE_IO_DRAIN pass with IOSQE_IO_LINK Jens Axboe @ 2019-08-16 0:48 ` Jackie Liu 2019-08-16 1:21 ` Jens Axboe 0 siblings, 1 reply; 7+ messages in thread From: Jackie Liu @ 2019-08-16 0:48 UTC (permalink / raw) To: Jens Axboe; +Cc: linux-block 在 2019年8月16日,01:07,Jens Axboe <axboe@kernel.dk> 写道: > > On 8/14/19 3:35 AM, Jackie Liu wrote: >> Suppose there are three IOs here, and their order is as follows: >> >> Submit: >> [1] IO_LINK >> | >> |--- [2] IO_LINK | IO_DRAIN >> | >> |- [3] NORMAL_IO >> >> In theory, they all need to be inserted into the Link-list, but flag >> IO_DRAIN we have, io[2] and io[3] will be inserted into the defer_list, >> and finally, io[3] and io[2] will be processed at the same time. >> >> Now, it is directly forbidden to pass these two flags at the same time. >> >> Fixes: 9e645e1105c ("io_uring: add support for sqe links") >> Signed-off-by: Jackie Liu <liuyun01@kylinos.cn> >> --- >> fs/io_uring.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/fs/io_uring.c b/fs/io_uring.c >> index d542f1c..05ee628 100644 >> --- a/fs/io_uring.c >> +++ b/fs/io_uring.c >> @@ -2074,10 +2074,13 @@ static void io_submit_sqe(struct io_ring_ctx *ctx, struct sqe_submit *s, >> { >> struct io_uring_sqe *sqe_copy; >> struct io_kiocb *req; >> + unsigned int flags; >> int ret; >> >> + flags = READ_ONCE(s->sqe->flags); >> /* enforce forwards compatibility on users */ >> - if (unlikely(s->sqe->flags & ~SQE_VALID_FLAGS)) { >> + if (unlikely((flags & ~SQE_VALID_FLAGS) || >> + (flags & (IOSQE_IO_DRAIN | IOSQE_IO_LINK)))) { > > This doesn't look right, as any setting of either DRAIN or LINK would now > fail? > > Did you mean something ala: > > if ((flags & (IOSQE_IO_DRAIN | IOSQE_IO_LINK)) == > (IOSQE_IO_DRAIN | IOSQE_IO_LINK)) { > ... fail ... > } > > which makes me worried that you didn't test this at all... > > -- > Jens Axboe Oh, yes, it's my fault, I just simulated it in my head, thank you for pointing out. I think I'd add an [RFC PATCH] next time. For this issue, I have two solutions, first is this, just avoid passing DRAIN and LINK at the same time; second is allow, let the SQE following LINK inherit the DRAIN flag, but It's more complicated, I prefer the first one. I will rewrite this patch later, with a real test. Thanks again. -- Jackie Liu ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] io_uring: fix issue when IOSQE_IO_DRAIN pass with IOSQE_IO_LINK 2019-08-16 0:48 ` Jackie Liu @ 2019-08-16 1:21 ` Jens Axboe 2019-08-16 6:47 ` Jackie Liu 0 siblings, 1 reply; 7+ messages in thread From: Jens Axboe @ 2019-08-16 1:21 UTC (permalink / raw) To: Jackie Liu; +Cc: linux-block On 8/15/19 6:48 PM, Jackie Liu wrote: > > 在 2019年8月16日,01:07,Jens Axboe <axboe@kernel.dk> 写道: > >> >> On 8/14/19 3:35 AM, Jackie Liu wrote: >>> Suppose there are three IOs here, and their order is as follows: >>> >>> Submit: >>> [1] IO_LINK >>> | >>> |--- [2] IO_LINK | IO_DRAIN >>> | >>> |- [3] NORMAL_IO >>> >>> In theory, they all need to be inserted into the Link-list, but flag >>> IO_DRAIN we have, io[2] and io[3] will be inserted into the defer_list, >>> and finally, io[3] and io[2] will be processed at the same time. >>> >>> Now, it is directly forbidden to pass these two flags at the same time. >>> >>> Fixes: 9e645e1105c ("io_uring: add support for sqe links") >>> Signed-off-by: Jackie Liu <liuyun01@kylinos.cn> >>> --- >>> fs/io_uring.c | 7 ++++++- >>> 1 file changed, 6 insertions(+), 1 deletion(-) >>> >>> diff --git a/fs/io_uring.c b/fs/io_uring.c >>> index d542f1c..05ee628 100644 >>> --- a/fs/io_uring.c >>> +++ b/fs/io_uring.c >>> @@ -2074,10 +2074,13 @@ static void io_submit_sqe(struct io_ring_ctx *ctx, struct sqe_submit *s, >>> { >>> struct io_uring_sqe *sqe_copy; >>> struct io_kiocb *req; >>> + unsigned int flags; >>> int ret; >>> >>> + flags = READ_ONCE(s->sqe->flags); >>> /* enforce forwards compatibility on users */ >>> - if (unlikely(s->sqe->flags & ~SQE_VALID_FLAGS)) { >>> + if (unlikely((flags & ~SQE_VALID_FLAGS) || >>> + (flags & (IOSQE_IO_DRAIN | IOSQE_IO_LINK)))) { >> >> This doesn't look right, as any setting of either DRAIN or LINK would now >> fail? >> >> Did you mean something ala: >> >> if ((flags & (IOSQE_IO_DRAIN | IOSQE_IO_LINK)) == >> (IOSQE_IO_DRAIN | IOSQE_IO_LINK)) { >> ... fail ... >> } >> >> which makes me worried that you didn't test this at all... >> >> -- >> Jens Axboe > > Oh, yes, it's my fault, I just simulated it in my head, thank you for > pointing out. I think I'd add an [RFC PATCH] next time. Even for an RFC, it better be more tested than just being thought about... If something hasn't been run at all, it should always include wording to that effect ("Totally untested, but something like this perhaps"). I have higher expectations for even an RFC patch, I do expect that to be both thought about AND tested. > For this issue, I have two solutions, first is this, just avoid > passing DRAIN and LINK at the same time; second is allow, let the SQE > following LINK inherit the DRAIN flag, but It's more complicated, I > prefer the first one. > > I will rewrite this patch later, with a real test. Thanks again. If an SQE has both set, it should first wait for any inflight sqe to complete, then execute the chain. Once things have drained, it should behave like an SQE that just had LINK set. I'd be interested in seeing a patch that fixes this instead of just making it illegal, it seems to be a valid use case. -- Jens Axboe ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] io_uring: fix issue when IOSQE_IO_DRAIN pass with IOSQE_IO_LINK 2019-08-16 1:21 ` Jens Axboe @ 2019-08-16 6:47 ` Jackie Liu 0 siblings, 0 replies; 7+ messages in thread From: Jackie Liu @ 2019-08-16 6:47 UTC (permalink / raw) To: Jens Axboe; +Cc: linux-block > 在 2019年8月16日,09:21,Jens Axboe <axboe@kernel.dk> 写道: > > On 8/15/19 6:48 PM, Jackie Liu wrote: >> >> 在 2019年8月16日,01:07,Jens Axboe <axboe@kernel.dk> 写道: >> >>> >>> On 8/14/19 3:35 AM, Jackie Liu wrote: >>>> Suppose there are three IOs here, and their order is as follows: >>>> >>>> Submit: >>>> [1] IO_LINK >>>> | >>>> |--- [2] IO_LINK | IO_DRAIN >>>> | >>>> |- [3] NORMAL_IO >>>> >>>> In theory, they all need to be inserted into the Link-list, but flag >>>> IO_DRAIN we have, io[2] and io[3] will be inserted into the defer_list, >>>> and finally, io[3] and io[2] will be processed at the same time. >>>> >>>> Now, it is directly forbidden to pass these two flags at the same time. >>>> >>>> Fixes: 9e645e1105c ("io_uring: add support for sqe links") >>>> Signed-off-by: Jackie Liu <liuyun01@kylinos.cn> >>>> --- >>>> fs/io_uring.c | 7 ++++++- >>>> 1 file changed, 6 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/fs/io_uring.c b/fs/io_uring.c >>>> index d542f1c..05ee628 100644 >>>> --- a/fs/io_uring.c >>>> +++ b/fs/io_uring.c >>>> @@ -2074,10 +2074,13 @@ static void io_submit_sqe(struct io_ring_ctx *ctx, struct sqe_submit *s, >>>> { >>>> struct io_uring_sqe *sqe_copy; >>>> struct io_kiocb *req; >>>> + unsigned int flags; >>>> int ret; >>>> >>>> + flags = READ_ONCE(s->sqe->flags); >>>> /* enforce forwards compatibility on users */ >>>> - if (unlikely(s->sqe->flags & ~SQE_VALID_FLAGS)) { >>>> + if (unlikely((flags & ~SQE_VALID_FLAGS) || >>>> + (flags & (IOSQE_IO_DRAIN | IOSQE_IO_LINK)))) { >>> >>> This doesn't look right, as any setting of either DRAIN or LINK would now >>> fail? >>> >>> Did you mean something ala: >>> >>> if ((flags & (IOSQE_IO_DRAIN | IOSQE_IO_LINK)) == >>> (IOSQE_IO_DRAIN | IOSQE_IO_LINK)) { >>> ... fail ... >>> } >>> >>> which makes me worried that you didn't test this at all... >>> >>> -- >>> Jens Axboe >> >> Oh, yes, it's my fault, I just simulated it in my head, thank you for >> pointing out. I think I'd add an [RFC PATCH] next time. > > Even for an RFC, it better be more tested than just being thought > about... If something hasn't been run at all, it should always include > wording to that effect ("Totally untested, but something like this > perhaps"). I have higher expectations for even an RFC patch, I do expect > that to be both thought about AND tested. > >> For this issue, I have two solutions, first is this, just avoid >> passing DRAIN and LINK at the same time; second is allow, let the SQE >> following LINK inherit the DRAIN flag, but It's more complicated, I >> prefer the first one. >> >> I will rewrite this patch later, with a real test. Thanks again. > > If an SQE has both set, it should first wait for any inflight sqe to > complete, then execute the chain. Once things have drained, it should > behave like an SQE that just had LINK set. I'd be interested in seeing a > patch that fixes this instead of just making it illegal, it seems to be > a valid use case. > How about this, We consider link list as a whole, and any IO among them that has drain will mark the first IO as drain, which is easy to implement. Of course, there is no clear order between IO in link list and IO in defer_list, so there maybe have a problems. If we want to keep a clear order between link list and defer_list, maybe need to add more flags and variables. my initial implementation is very complicated. do you have any good idea? First idea patch like follow. untested, just a idea. --- diff --git a/fs/io_uring.c b/fs/io_uring.c index 6b572c4..bc0b535 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -1995,10 +1995,15 @@ static int io_req_set_file(struct io_ring_ctx *ctx, const struct sqe_submit *s, flags = READ_ONCE(s->sqe->flags); fd = READ_ONCE(s->sqe->fd); - if (flags & IOSQE_IO_DRAIN) { + if (flags & IOSQE_IO_DRAIN) req->flags |= REQ_F_IO_DRAIN; - req->sequence = ctx->cached_sq_head - 1; - } + + /* + * All io need record the previous position, if LINK vs DARIN, + * it can be used to mark the position of the first IO in the + * link list. + */ + req->sequence = ctx->cached_sq_head - 1; if (!io_op_needs_file(s->sqe)) return 0; @@ -2123,6 +2128,12 @@ static void io_submit_sqe(struct io_ring_ctx *ctx, struct sqe_submit *s, } s->sqe = sqe_copy; + /* + * Mark the first IO in link list as DRAIN, let all the following + * IOs enter the defer list. + */ + if (s->sqe->flags & IOSQE_IO_DRAIN) + prev->flags |= REQ_F_IO_DRAIN; memcpy(&req->submit, s, sizeof(*s)); list_add_tail(&req->list, &prev->link_list); } else if (s->sqe->flags & IOSQE_IO_LINK) { -- Jackie Liu ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-08-16 6:47 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-08-14 9:35 [PATCH 1/2] io_uring: fix issue when IOSQE_IO_DRAIN pass with IOSQE_IO_LINK Jackie Liu 2019-08-14 9:35 ` [PATCH 2/2] io_uring: fix an issue when IOSQE_IO_LINK is inserted into defer list Jackie Liu 2019-08-15 17:21 ` Jens Axboe 2019-08-15 17:07 ` [PATCH 1/2] io_uring: fix issue when IOSQE_IO_DRAIN pass with IOSQE_IO_LINK Jens Axboe 2019-08-16 0:48 ` Jackie Liu 2019-08-16 1:21 ` Jens Axboe 2019-08-16 6:47 ` Jackie Liu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).