IO-Uring Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] io_uring: fixup a few spots where link failure isn't flagged
@ 2019-11-06  3:32 Jens Axboe
  2019-11-06 18:44 ` Pavel Begunkov
  0 siblings, 1 reply; 3+ messages in thread
From: Jens Axboe @ 2019-11-06  3:32 UTC (permalink / raw)
  To: io-uring; +Cc: linux-block

If a request fails, we need to ensure we set REQ_F_FAIL_LINK on it if
REQ_F_LINK is set. Any failure in the chain should break the chain.

We were missing a few spots where this should be done. It might be nice
to generalize this somewhat at some point, as long as we factor in the
fact that failure looks different for each request type.

Signed-off-by: Jens Axboe <axboe@kernel.dk>

--


diff --git a/fs/io_uring.c b/fs/io_uring.c
index bda27b52fd5b..4edc94aab17e 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1672,6 +1672,8 @@ static int io_send_recvmsg(struct io_kiocb *req, const struct io_uring_sqe *sqe,
 	}
 
 	io_cqring_add_event(req->ctx, sqe->user_data, ret);
+	if (ret < 0 && (req->flags & REQ_F_LINK))
+		req->flags |= REQ_F_FAIL_LINK;
 	io_put_req(req, nxt);
 	return 0;
 }
@@ -1787,6 +1789,8 @@ static int io_poll_remove(struct io_kiocb *req, const struct io_uring_sqe *sqe)
 	spin_unlock_irq(&ctx->completion_lock);
 
 	io_cqring_add_event(req->ctx, sqe->user_data, ret);
+	if (ret < 0 && (req->flags & REQ_F_LINK))
+		req->flags |= REQ_F_FAIL_LINK;
 	io_put_req(req, NULL);
 	return 0;
 }
@@ -1994,6 +1998,8 @@ static enum hrtimer_restart io_timeout_fn(struct hrtimer *timer)
 	spin_unlock_irqrestore(&ctx->completion_lock, flags);
 
 	io_cqring_ev_posted(ctx);
+	if (req->flags & REQ_F_LINK)
+		req->flags |= REQ_F_FAIL_LINK;
 	io_put_req(req, NULL);
 	return HRTIMER_NORESTART;
 }
@@ -2035,6 +2041,8 @@ static int io_timeout_remove(struct io_kiocb *req,
 		io_commit_cqring(ctx);
 		spin_unlock_irq(&ctx->completion_lock);
 		io_cqring_ev_posted(ctx);
+		if (req->flags & REQ_F_LINK)
+			req->flags |= REQ_F_FAIL_LINK;
 		io_put_req(req, NULL);
 		return 0;
 	}
@@ -2328,6 +2336,8 @@ static void io_wq_submit_work(struct io_wq_work **workptr)
 	io_put_req(req, NULL);
 
 	if (ret) {
+		if (req->flags & REQ_F_LINK)
+			req->flags |= REQ_F_FAIL_LINK;
 		io_cqring_add_event(ctx, sqe->user_data, ret);
 		io_put_req(req, NULL);
 	}

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] io_uring: fixup a few spots where link failure isn't flagged
  2019-11-06  3:32 [PATCH] io_uring: fixup a few spots where link failure isn't flagged Jens Axboe
@ 2019-11-06 18:44 ` Pavel Begunkov
  2019-11-07  0:03   ` Jens Axboe
  0 siblings, 1 reply; 3+ messages in thread
From: Pavel Begunkov @ 2019-11-06 18:44 UTC (permalink / raw)
  To: Jens Axboe, io-uring; +Cc: linux-block

[-- Attachment #1.1: Type: text/plain, Size: 2554 bytes --]

On 06/11/2019 06:32, Jens Axboe wrote:
> If a request fails, we need to ensure we set REQ_F_FAIL_LINK on it if
> REQ_F_LINK is set. Any failure in the chain should break the chain.
> 
> We were missing a few spots where this should be done. It might be nice
> to generalize this somewhat at some point, as long as we factor in the
> fact that failure looks different for each request type.
> 

The completion path also starts to get complicated, especially
non-uniform handling of links there.

i.e. io_put_req() -> io_put_req_find_next() ->
	io_free_req() -> __io_free_req()
Plus, io_free_req_many(), which can be used only in some cases.

My compiler didn't even inlined it, so there are 4 CALLs.
Though, still in TODO list.


> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> 
> --
> 
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index bda27b52fd5b..4edc94aab17e 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -1672,6 +1672,8 @@ static int io_send_recvmsg(struct io_kiocb *req, const struct io_uring_sqe *sqe,
>  	}
>  
>  	io_cqring_add_event(req->ctx, sqe->user_data, ret);
> +	if (ret < 0 && (req->flags & REQ_F_LINK))
> +		req->flags |= REQ_F_FAIL_LINK;
>  	io_put_req(req, nxt);
>  	return 0;
>  }
> @@ -1787,6 +1789,8 @@ static int io_poll_remove(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>  	spin_unlock_irq(&ctx->completion_lock);
>  
>  	io_cqring_add_event(req->ctx, sqe->user_data, ret);
> +	if (ret < 0 && (req->flags & REQ_F_LINK))
> +		req->flags |= REQ_F_FAIL_LINK;
>  	io_put_req(req, NULL);
>  	return 0;
>  }
> @@ -1994,6 +1998,8 @@ static enum hrtimer_restart io_timeout_fn(struct hrtimer *timer)
>  	spin_unlock_irqrestore(&ctx->completion_lock, flags);
>  
>  	io_cqring_ev_posted(ctx);
> +	if (req->flags & REQ_F_LINK)
> +		req->flags |= REQ_F_FAIL_LINK;
>  	io_put_req(req, NULL);
>  	return HRTIMER_NORESTART;
>  }
> @@ -2035,6 +2041,8 @@ static int io_timeout_remove(struct io_kiocb *req,
>  		io_commit_cqring(ctx);
>  		spin_unlock_irq(&ctx->completion_lock);
>  		io_cqring_ev_posted(ctx);
> +		if (req->flags & REQ_F_LINK)
> +			req->flags |= REQ_F_FAIL_LINK;
>  		io_put_req(req, NULL);
>  		return 0;
>  	}
> @@ -2328,6 +2336,8 @@ static void io_wq_submit_work(struct io_wq_work **workptr)
>  	io_put_req(req, NULL);
>  
>  	if (ret) {
> +		if (req->flags & REQ_F_LINK)
> +			req->flags |= REQ_F_FAIL_LINK;
>  		io_cqring_add_event(ctx, sqe->user_data, ret);
>  		io_put_req(req, NULL);
>  	}
> 

-- 
Pavel Begunkov


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] io_uring: fixup a few spots where link failure isn't flagged
  2019-11-06 18:44 ` Pavel Begunkov
@ 2019-11-07  0:03   ` Jens Axboe
  0 siblings, 0 replies; 3+ messages in thread
From: Jens Axboe @ 2019-11-07  0:03 UTC (permalink / raw)
  To: Pavel Begunkov, io-uring; +Cc: linux-block

On 11/6/19 11:44 AM, Pavel Begunkov wrote:
> On 06/11/2019 06:32, Jens Axboe wrote:
>> If a request fails, we need to ensure we set REQ_F_FAIL_LINK on it if
>> REQ_F_LINK is set. Any failure in the chain should break the chain.
>>
>> We were missing a few spots where this should be done. It might be nice
>> to generalize this somewhat at some point, as long as we factor in the
>> fact that failure looks different for each request type.
>>
> 
> The completion path also starts to get complicated, especially
> non-uniform handling of links there.
> 
> i.e. io_put_req() -> io_put_req_find_next() ->
> 	io_free_req() -> __io_free_req()
> Plus, io_free_req_many(), which can be used only in some cases.

The many case is just for polling, I actually think that one is pretty
clean and self explanatory. But I agree on the proliferation of them
otherwise, at least the patches today cleaned it up a little.
 
> My compiler didn't even inlined it, so there are 4 CALLs.
> Though, still in TODO list.

Huh, that's odd!

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-06  3:32 [PATCH] io_uring: fixup a few spots where link failure isn't flagged Jens Axboe
2019-11-06 18:44 ` Pavel Begunkov
2019-11-07  0:03   ` Jens Axboe

IO-Uring Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/io-uring/0 io-uring/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 io-uring io-uring/ https://lore.kernel.org/io-uring \
		io-uring@vger.kernel.org
	public-inbox-index io-uring

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.io-uring


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git