Re: [PATCH 2/3] io_uring: fix failed linkchain code logic

[Pavel Begunkov <asml.silence@gmail.com>]

On 8/18/21 8:43 AM, Hao Xu wrote:
> Given a linkchain like this:
> req0(link_flag)-->req1(link_flag)-->...-->reqn(no link_flag)
> 
> There is a problem:
>  - if some intermediate linked req like req1 's submittion fails, reqs
>    after it won't be cancelled.
> 
>    - sqpoll disabled: maybe it's ok since users can get the error info
>      of req1 and stop submitting the following sqes.
> 
>    - sqpoll enabled: definitely a problem, the following sqes will be
>      submitted in the next round.
> 
> The solution is to refactor the code logic to:
>  - link a linked req to the chain first, no matter its submittion fails
>    or not.
>  - if a linked req's submittion fails, just mark head as
>    failed. leverage req->result to indicate whether the req is a failed
>    one or cancelled one.
>  - submit or fail the whole chain
> 
> Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
> ---
>  fs/io_uring.c | 86 ++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 58 insertions(+), 28 deletions(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index c0b841506869..383668e07417 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -1920,11 +1920,13 @@ static void io_fail_links(struct io_kiocb *req)
>  
>  	req->link = NULL;
>  	while (link) {
> +		int res = link->result ? link->result : -ECANCELED;

btw, we don't properly initialise req->result, and don't want to.
Perhaps, can be more like 

res = -ECANCELLED;
if (req->flags & FAIL)
	res = req->result;


> +
>  		nxt = link->link;
>  		link->link = NULL;
>  
>  		trace_io_uring_fail_link(req, link);
> -		io_cqring_fill_event(link->ctx, link->user_data, -ECANCELED, 0);
> +		io_cqring_fill_event(link->ctx, link->user_data, res, 0);
>  		io_put_req_deferred(link);
>  		link = nxt;
>  	}
> @@ -5698,7 +5700,7 @@ static int io_timeout_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe,
>  	if (is_timeout_link) {
>  		struct io_submit_link *link = &req->ctx->submit_state.link;
>  
> -		if (!link->head)
> +		if (!link->head || link->head == req)
>  			return -EINVAL;
>  		if (link->last->opcode == IORING_OP_LINK_TIMEOUT)
>  			return -EINVAL;
> @@ -6622,17 +6624,38 @@ static int io_submit_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
>  	__must_hold(&ctx->uring_lock)
>  {
>  	struct io_submit_link *link = &ctx->submit_state.link;
> +	bool is_link = sqe->flags & (IOSQE_IO_LINK | IOSQE_IO_HARDLINK);
> +	struct io_kiocb *head;
>  	int ret;
>  
> +	/*
> +	 * we don't update link->last until we've done io_req_prep()
> +	 * since linked timeout uses old link->last
> +	 */
> +	if (link->head)
> +		link->last->link = req;
> +	else if (is_link)
> +		link->head = req;
> +	head = link->head;

It's a horrorsome amount of overhead. How about to set the fail flag
if failed early and actually fail on io_queue_sqe(), as below. It's
not tested and a couple more bits added, but hopefully gives the idea.


diff --git a/fs/io_uring.c b/fs/io_uring.c
index ba087f395507..3fd0730655d0 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -6530,8 +6530,10 @@ static inline void io_queue_sqe(struct io_kiocb *req)
 	if (unlikely(req->ctx->drain_active) && io_drain_req(req))
 		return;
 
-	if (likely(!(req->flags & REQ_F_FORCE_ASYNC))) {
+	if (likely(!(req->flags & (REQ_F_FORCE_ASYNC|REQ_F_FAIL)))) {
 		__io_queue_sqe(req);
+	} else if (req->flags & REQ_F_FAIL) {
+		io_req_complete_failed(req, ret);
 	} else {
 		int ret = io_req_prep_async(req);
 
@@ -6640,19 +6642,17 @@ static int io_submit_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
 	ret = io_init_req(ctx, req, sqe);
 	if (unlikely(ret)) {
 fail_req:
-		if (link->head) {
-			/* fail even hard links since we don't submit */
+		/* fail even hard links since we don't submit */
+		if (link->head)
 			req_set_fail(link->head);
-			io_req_complete_failed(link->head, -ECANCELED);
-			link->head = NULL;
-		}
-		io_req_complete_failed(req, ret);
-		return ret;
+		req_set_fail(req);
+		req->result = ret;
+	} else {
+		ret = io_req_prep(req, sqe);
+		if (unlikely(ret))
+			goto fail_req;
 	}
 
-	ret = io_req_prep(req, sqe);
-	if (unlikely(ret))
-		goto fail_req;
 
 	/* don't need @sqe from now on */
 	trace_io_uring_submit_sqe(ctx, req, req->opcode, req->user_data,
@@ -6670,8 +6670,10 @@ static int io_submit_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
 		struct io_kiocb *head = link->head;
 
 		ret = io_req_prep_async(req);
-		if (unlikely(ret))
-			goto fail_req;
+		if (unlikely(ret)) {
+			req->result = ret;
+			req_set_fail(link->head);
+		}
 		trace_io_uring_link(ctx, req, head);
 		link->last->link = req;
 		link->last = req;