[PATCH v2 0/2] async hybrid for pollable requests

[PATCH v2 0/2] async hybrid for pollable requests

[Hao Xu]

1/2 is a prep patch. 2/2 is the main one.
The good thing: see commit message.
the side effect: for normal io-worker path, added two if and two local
variables. for FORCE_ASYNC path, added three if and several dereferences

I think it is fine since the io-worker path is not the fast path, and
the benefit of this patchset is worth it.

Btw, we need to tweak the io-cancel.c a bit, not a big problem.
liburing tests will come later.

v1-->v2:
 - split logic of force_nonblock
 - tweak added code in io_wq_submit_work to reduce overhead
 from Pavel's commments.

Hao Xu (2):
  io_uring: split logic of force_nonblock
  io_uring: implement async hybrid mode for pollable requests

 fs/io_uring.c                 | 85 ++++++++++++++++++++++++++---------
 include/uapi/linux/io_uring.h |  4 +-
 2 files changed, 66 insertions(+), 23 deletions(-)

-- 
2.24.4

[PATCH 1/2] io_uring: split logic of force_nonblock

[Hao Xu]

Currently force_nonblock stands for three meanings:
 - nowait or not
 - in an io-worker or not(hold uring_lock or not)

Let's split the logic to two flags, IO_URING_F_NONBLOCK and
IO_URING_F_UNLOCKED for convenience of the next patch.

Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
---
 fs/io_uring.c | 50 ++++++++++++++++++++++++++++----------------------
 1 file changed, 28 insertions(+), 22 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index b6da03c26122..727cad6c36fc 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -199,6 +199,7 @@ struct io_rings {
 
 enum io_uring_cmd_flags {
 	IO_URING_F_COMPLETE_DEFER	= 1,
+	IO_URING_F_UNLOCKED		= 2,
 	/* int's last bit, sign checks are usually faster than a bit test */
 	IO_URING_F_NONBLOCK		= INT_MIN,
 };
@@ -2926,7 +2927,7 @@ static void kiocb_done(struct kiocb *kiocb, ssize_t ret,
 			struct io_ring_ctx *ctx = req->ctx;
 
 			req_set_fail(req);
-			if (!(issue_flags & IO_URING_F_NONBLOCK)) {
+			if (issue_flags & IO_URING_F_UNLOCKED) {
 				mutex_lock(&ctx->uring_lock);
 				__io_req_complete(req, issue_flags, ret, cflags);
 				mutex_unlock(&ctx->uring_lock);
@@ -3036,7 +3037,7 @@ static struct io_buffer *io_buffer_select(struct io_kiocb *req, size_t *len,
 {
 	struct io_buffer *kbuf = req->kbuf;
 	struct io_buffer *head;
-	bool needs_lock = !(issue_flags & IO_URING_F_NONBLOCK);
+	bool needs_lock = issue_flags & IO_URING_F_UNLOCKED;
 
 	if (req->flags & REQ_F_BUFFER_SELECTED)
 		return kbuf;
@@ -3341,7 +3342,7 @@ static inline int io_rw_prep_async(struct io_kiocb *req, int rw)
 	int ret;
 
 	/* submission path, ->uring_lock should already be taken */
-	ret = io_import_iovec(rw, req, &iov, &iorw->s, IO_URING_F_NONBLOCK);
+	ret = io_import_iovec(rw, req, &iov, &iorw->s, 0);
 	if (unlikely(ret < 0))
 		return ret;
 
@@ -3452,6 +3453,7 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags)
 	struct iovec *iovec;
 	struct kiocb *kiocb = &req->rw.kiocb;
 	bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK;
+	bool in_worker = issue_flags & IO_URING_F_UNLOCKED;
 	struct io_async_rw *rw;
 	ssize_t ret, ret2;
 
@@ -3495,7 +3497,7 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags)
 	if (ret == -EAGAIN || (req->flags & REQ_F_REISSUE)) {
 		req->flags &= ~REQ_F_REISSUE;
 		/* IOPOLL retry should happen for io-wq threads */
-		if (!force_nonblock && !(req->ctx->flags & IORING_SETUP_IOPOLL))
+		if (in_worker && !(req->ctx->flags & IORING_SETUP_IOPOLL))
 			goto done;
 		/* no retry on NONBLOCK nor RWF_NOWAIT */
 		if (req->flags & REQ_F_NOWAIT)
@@ -3503,7 +3505,7 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags)
 		ret = 0;
 	} else if (ret == -EIOCBQUEUED) {
 		goto out_free;
-	} else if (ret == req->result || ret <= 0 || !force_nonblock ||
+	} else if (ret == req->result || ret <= 0 || in_worker ||
 		   (req->flags & REQ_F_NOWAIT) || !need_read_all(req)) {
 		/* read all, failed, already did sync or don't want to retry */
 		goto done;
@@ -3581,6 +3583,7 @@ static int io_write(struct io_kiocb *req, unsigned int issue_flags)
 	struct iovec *iovec;
 	struct kiocb *kiocb = &req->rw.kiocb;
 	bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK;
+	bool in_worker = issue_flags & IO_URING_F_UNLOCKED;
 	ssize_t ret, ret2;
 
 	if (!req_has_async_data(req)) {
@@ -3651,7 +3654,7 @@ static int io_write(struct io_kiocb *req, unsigned int issue_flags)
 	/* no retry on NONBLOCK nor RWF_NOWAIT */
 	if (ret2 == -EAGAIN && (req->flags & REQ_F_NOWAIT))
 		goto done;
-	if (!force_nonblock || ret2 != -EAGAIN) {
+	if (in_worker || ret2 != -EAGAIN) {
 		/* IOPOLL retry should happen for io-wq threads */
 		if ((req->ctx->flags & IORING_SETUP_IOPOLL) && ret2 == -EAGAIN)
 			goto copy_iov;
@@ -4314,9 +4317,9 @@ static int io_remove_buffers(struct io_kiocb *req, unsigned int issue_flags)
 	struct io_ring_ctx *ctx = req->ctx;
 	struct io_buffer *head;
 	int ret = 0;
-	bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK;
+	bool needs_lock = issue_flags & IO_URING_F_UNLOCKED;
 
-	io_ring_submit_lock(ctx, !force_nonblock);
+	io_ring_submit_lock(ctx, needs_lock);
 
 	lockdep_assert_held(&ctx->uring_lock);
 
@@ -4329,7 +4332,7 @@ static int io_remove_buffers(struct io_kiocb *req, unsigned int issue_flags)
 
 	/* complete before unlock, IOPOLL may need the lock */
 	__io_req_complete(req, issue_flags, ret, 0);
-	io_ring_submit_unlock(ctx, !force_nonblock);
+	io_ring_submit_unlock(ctx, needs_lock);
 	return 0;
 }
 
@@ -4401,9 +4404,9 @@ static int io_provide_buffers(struct io_kiocb *req, unsigned int issue_flags)
 	struct io_ring_ctx *ctx = req->ctx;
 	struct io_buffer *head, *list;
 	int ret = 0;
-	bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK;
+	bool needs_lock = issue_flags & IO_URING_F_UNLOCKED;
 
-	io_ring_submit_lock(ctx, !force_nonblock);
+	io_ring_submit_lock(ctx, needs_lock);
 
 	lockdep_assert_held(&ctx->uring_lock);
 
@@ -4419,7 +4422,7 @@ static int io_provide_buffers(struct io_kiocb *req, unsigned int issue_flags)
 		req_set_fail(req);
 	/* complete before unlock, IOPOLL may need the lock */
 	__io_req_complete(req, issue_flags, ret, 0);
-	io_ring_submit_unlock(ctx, !force_nonblock);
+	io_ring_submit_unlock(ctx, needs_lock);
 	return 0;
 }
 
@@ -6279,6 +6282,7 @@ static int io_async_cancel(struct io_kiocb *req, unsigned int issue_flags)
 {
 	struct io_ring_ctx *ctx = req->ctx;
 	u64 sqe_addr = req->cancel.addr;
+	bool needs_lock = issue_flags & IO_URING_F_UNLOCKED;
 	struct io_tctx_node *node;
 	int ret;
 
@@ -6287,7 +6291,7 @@ static int io_async_cancel(struct io_kiocb *req, unsigned int issue_flags)
 		goto done;
 
 	/* slow path, try all io-wq's */
-	io_ring_submit_lock(ctx, !(issue_flags & IO_URING_F_NONBLOCK));
+	io_ring_submit_lock(ctx, needs_lock);
 	ret = -ENOENT;
 	list_for_each_entry(node, &ctx->tctx_list, ctx_node) {
 		struct io_uring_task *tctx = node->task->io_uring;
@@ -6296,7 +6300,7 @@ static int io_async_cancel(struct io_kiocb *req, unsigned int issue_flags)
 		if (ret != -ENOENT)
 			break;
 	}
-	io_ring_submit_unlock(ctx, !(issue_flags & IO_URING_F_NONBLOCK));
+	io_ring_submit_unlock(ctx, needs_lock);
 done:
 	if (ret < 0)
 		req_set_fail(req);
@@ -6323,6 +6327,7 @@ static int io_rsrc_update_prep(struct io_kiocb *req,
 static int io_files_update(struct io_kiocb *req, unsigned int issue_flags)
 {
 	struct io_ring_ctx *ctx = req->ctx;
+	bool needs_lock = issue_flags & IO_URING_F_UNLOCKED;
 	struct io_uring_rsrc_update2 up;
 	int ret;
 
@@ -6332,10 +6337,10 @@ static int io_files_update(struct io_kiocb *req, unsigned int issue_flags)
 	up.tags = 0;
 	up.resv = 0;
 
-	io_ring_submit_lock(ctx, !(issue_flags & IO_URING_F_NONBLOCK));
+	io_ring_submit_lock(ctx, needs_lock);
 	ret = __io_register_rsrc_update(ctx, IORING_RSRC_FILE,
 					&up, req->rsrc_update.nr_args);
-	io_ring_submit_unlock(ctx, !(issue_flags & IO_URING_F_NONBLOCK));
+	io_ring_submit_unlock(ctx, needs_lock);
 
 	if (ret < 0)
 		req_set_fail(req);
@@ -6745,7 +6750,7 @@ static void io_wq_submit_work(struct io_wq_work *work)
 
 	if (!ret) {
 		do {
-			ret = io_issue_sqe(req, 0);
+			ret = io_issue_sqe(req, IO_URING_F_UNLOCKED);
 			/*
 			 * We can get EAGAIN for polled IO even though we're
 			 * forcing a sync submission from here, since we can't
@@ -8333,12 +8338,12 @@ static int io_install_fixed_file(struct io_kiocb *req, struct file *file,
 				 unsigned int issue_flags, u32 slot_index)
 {
 	struct io_ring_ctx *ctx = req->ctx;
-	bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK;
+	bool needs_lock = issue_flags & IO_URING_F_UNLOCKED;
 	bool needs_switch = false;
 	struct io_fixed_file *file_slot;
 	int ret = -EBADF;
 
-	io_ring_submit_lock(ctx, !force_nonblock);
+	io_ring_submit_lock(ctx, needs_lock);
 	if (file->f_op == &io_uring_fops)
 		goto err;
 	ret = -ENXIO;
@@ -8379,7 +8384,7 @@ static int io_install_fixed_file(struct io_kiocb *req, struct file *file,
 err:
 	if (needs_switch)
 		io_rsrc_node_switch(ctx, ctx->file_data);
-	io_ring_submit_unlock(ctx, !force_nonblock);
+	io_ring_submit_unlock(ctx, needs_lock);
 	if (ret)
 		fput(file);
 	return ret;
@@ -8389,11 +8394,12 @@ static int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags)
 {
 	unsigned int offset = req->close.file_slot - 1;
 	struct io_ring_ctx *ctx = req->ctx;
+	bool needs_lock = issue_flags & IO_URING_F_UNLOCKED;
 	struct io_fixed_file *file_slot;
 	struct file *file;
 	int ret, i;
 
-	io_ring_submit_lock(ctx, !(issue_flags & IO_URING_F_NONBLOCK));
+	io_ring_submit_lock(ctx, needs_lock);
 	ret = -ENXIO;
 	if (unlikely(!ctx->file_data))
 		goto out;
@@ -8419,7 +8425,7 @@ static int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags)
 	io_rsrc_node_switch(ctx, ctx->file_data);
 	ret = 0;
 out:
-	io_ring_submit_unlock(ctx, !(issue_flags & IO_URING_F_NONBLOCK));
+	io_ring_submit_unlock(ctx, needs_lock);
 	return ret;
 }
 
-- 
2.24.4

[PATCH 2/2] io_uring: implement async hybrid mode for pollable requests

[Hao Xu]

The current logic of requests with IOSQE_ASYNC is first queueing it to
io-worker, then execute it in a synchronous way. For unbound works like
pollable requests(e.g. read/write a socketfd), the io-worker may stuck
there waiting for events for a long time. And thus other works wait in
the list for a long time too.
Let's introduce a new way for unbound works (currently pollable
requests), with this a request will first be queued to io-worker, then
executed in a nonblock try rather than a synchronous way. Failure of
that leads it to arm poll stuff and then the worker can begin to handle
other works.
The detail process of this kind of requests is:

step1: original context:
           queue it to io-worker
step2: io-worker context:
           nonblock try(the old logic is a synchronous try here)
               |
               |--fail--> arm poll
                            |
                            |--(fail/ready)-->synchronous issue
                            |
                            |--(succeed)-->worker finish it's job, tw
                                           take over the req

This works much better than the old IOSQE_ASYNC logic in cases where
unbound max_worker is relatively small. In this case, number of
io-worker eazily increments to max_worker, new worker cannot be created
and running workers stuck there handling old works in IOSQE_ASYNC mode.

In my 64-core machine, set unbound max_worker to 20, run echo-server,
turns out:
(arguments: register_file, connetion number is 1000, message size is 12
Byte)
original IOSQE_ASYNC: 76664.151 tps
after this patch: 166934.985 tps

Suggested-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
---
 fs/io_uring.c                 | 39 +++++++++++++++++++++++++++++++++--
 include/uapi/linux/io_uring.h |  4 +++-
 2 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 727cad6c36fc..a3247a5dafc9 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -3497,7 +3497,8 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags)
 	if (ret == -EAGAIN || (req->flags & REQ_F_REISSUE)) {
 		req->flags &= ~REQ_F_REISSUE;
 		/* IOPOLL retry should happen for io-wq threads */
-		if (in_worker && !(req->ctx->flags & IORING_SETUP_IOPOLL))
+		if (in_worker && !force_nonblock &&
+		    !(req->ctx->flags & IORING_SETUP_IOPOLL))
 			goto done;
 		/* no retry on NONBLOCK nor RWF_NOWAIT */
 		if (req->flags & REQ_F_NOWAIT)
@@ -6749,8 +6750,18 @@ static void io_wq_submit_work(struct io_wq_work *work)
 		ret = -ECANCELED;
 
 	if (!ret) {
+		bool needs_poll = false;
+		unsigned int issue_flags = IO_URING_F_UNLOCKED;
+
+		if (req->flags & REQ_F_FORCE_ASYNC) {
+			needs_poll = req->file && file_can_poll(req->file);
+			if (needs_poll)
+				issue_flags |= IO_URING_F_NONBLOCK;
+		}
+
 		do {
-			ret = io_issue_sqe(req, IO_URING_F_UNLOCKED);
+issue_sqe:
+			ret = io_issue_sqe(req, issue_flags);
 			/*
 			 * We can get EAGAIN for polled IO even though we're
 			 * forcing a sync submission from here, since we can't
@@ -6758,6 +6769,30 @@ static void io_wq_submit_work(struct io_wq_work *work)
 			 */
 			if (ret != -EAGAIN)
 				break;
+			if (needs_poll) {
+				bool armed = false;
+
+				ret = 0;
+				needs_poll = false;
+				issue_flags &= ~IO_URING_F_NONBLOCK;
+
+				switch (io_arm_poll_handler(req)) {
+				case IO_APOLL_READY:
+					goto issue_sqe;
+				case IO_APOLL_ABORTED:
+					/*
+					 * somehow we failed to arm the poll infra,
+					 * fallback it to a normal async worker try.
+					 */
+					break;
+				case IO_APOLL_OK:
+					armed = true;
+					break;
+				}
+
+				if (armed)
+					break;
+			}
 			cond_resched();
 		} while (1);
 	}
diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h
index c45b5e9a9387..3e49a7dbe636 100644
--- a/include/uapi/linux/io_uring.h
+++ b/include/uapi/linux/io_uring.h
@@ -70,6 +70,7 @@ enum {
 	IOSQE_IO_HARDLINK_BIT,
 	IOSQE_ASYNC_BIT,
 	IOSQE_BUFFER_SELECT_BIT,
+	IOSQE_ASYNC_HYBRID_BIT,
 };
 
 /*
@@ -87,7 +88,8 @@ enum {
 #define IOSQE_ASYNC		(1U << IOSQE_ASYNC_BIT)
 /* select buffer from sqe->buf_group */
 #define IOSQE_BUFFER_SELECT	(1U << IOSQE_BUFFER_SELECT_BIT)
-
+/* first force async then arm poll */
+#define IOSQE_ASYNC_HYBRID	(1U << IOSQE_ASYNC_HYBRID_BIT)
 /*
  * io_uring_setup() flags
  */
-- 
2.24.4

Re: [PATCH 2/2] io_uring: implement async hybrid mode for pollable requests

[Hao Xu]

在 2021/10/18 下午7:29, Hao Xu 写道:
> The current logic of requests with IOSQE_ASYNC is first queueing it to
> io-worker, then execute it in a synchronous way. For unbound works like
> pollable requests(e.g. read/write a socketfd), the io-worker may stuck
> there waiting for events for a long time. And thus other works wait in
> the list for a long time too.
> Let's introduce a new way for unbound works (currently pollable
> requests), with this a request will first be queued to io-worker, then
> executed in a nonblock try rather than a synchronous way. Failure of
> that leads it to arm poll stuff and then the worker can begin to handle
> other works.
> The detail process of this kind of requests is:
> 
> step1: original context:
>             queue it to io-worker
> step2: io-worker context:
>             nonblock try(the old logic is a synchronous try here)
>                 |
>                 |--fail--> arm poll
>                              |
>                              |--(fail/ready)-->synchronous issue
>                              |
>                              |--(succeed)-->worker finish it's job, tw
>                                             take over the req
> 
> This works much better than the old IOSQE_ASYNC logic in cases where
> unbound max_worker is relatively small. In this case, number of
> io-worker eazily increments to max_worker, new worker cannot be created
> and running workers stuck there handling old works in IOSQE_ASYNC mode.
> 
> In my 64-core machine, set unbound max_worker to 20, run echo-server,
> turns out:
> (arguments: register_file, connetion number is 1000, message size is 12
> Byte)
> original IOSQE_ASYNC: 76664.151 tps
> after this patch: 166934.985 tps
> 
> Suggested-by: Jens Axboe <axboe@kernel.dk>
> Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
An irrelevant question: why do we do linked timeout logic in
io_wq_submit_work() again regarding that we've already done it in
io_queue_async_work().

Re: [PATCH 2/2] io_uring: implement async hybrid mode for pollable requests

[Pavel Begunkov]

On 10/18/21 11:34, Hao Xu wrote:
> 在 2021/10/18 下午7:29, Hao Xu 写道:
>> The current logic of requests with IOSQE_ASYNC is first queueing it to
>> io-worker, then execute it in a synchronous way. For unbound works like
>> pollable requests(e.g. read/write a socketfd), the io-worker may stuck
>> there waiting for events for a long time. And thus other works wait in
>> the list for a long time too.
>> Let's introduce a new way for unbound works (currently pollable
>> requests), with this a request will first be queued to io-worker, then
>> executed in a nonblock try rather than a synchronous way. Failure of
>> that leads it to arm poll stuff and then the worker can begin to handle
>> other works.
>> The detail process of this kind of requests is:
>>
>> step1: original context:
>>             queue it to io-worker
>> step2: io-worker context:
>>             nonblock try(the old logic is a synchronous try here)
>>                 |
>>                 |--fail--> arm poll
>>                              |
>>                              |--(fail/ready)-->synchronous issue
>>                              |
>>                              |--(succeed)-->worker finish it's job, tw
>>                                             take over the req
>>
>> This works much better than the old IOSQE_ASYNC logic in cases where
>> unbound max_worker is relatively small. In this case, number of
>> io-worker eazily increments to max_worker, new worker cannot be created
>> and running workers stuck there handling old works in IOSQE_ASYNC mode.
>>
>> In my 64-core machine, set unbound max_worker to 20, run echo-server,
>> turns out:
>> (arguments: register_file, connetion number is 1000, message size is 12
>> Byte)
>> original IOSQE_ASYNC: 76664.151 tps
>> after this patch: 166934.985 tps
>>
>> Suggested-by: Jens Axboe <axboe@kernel.dk>
>> Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
> An irrelevant question: why do we do linked timeout logic in
> io_wq_submit_work() again regarding that we've already done it in
> io_queue_async_work().

Because io_wq_free_work() may enqueue new work (by returning it)
without going through io_queue_async_work(), and we don't care
enough to split those cases.

-- 
Pavel Begunkov

Re: [PATCH 1/2] io_uring: split logic of force_nonblock

[Pavel Begunkov]

On 10/18/21 11:29, Hao Xu wrote:
> Currently force_nonblock stands for three meanings:
>   - nowait or not
>   - in an io-worker or not(hold uring_lock or not)

We should have done it long ago. You can send it separately if
it'd help.

One more recently added spot is missing: io_iopoll_req_issued()


> Let's split the logic to two flags, IO_URING_F_NONBLOCK and
> IO_URING_F_UNLOCKED for convenience of the next patch.
> 
> Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
> ---
>   fs/io_uring.c | 50 ++++++++++++++++++++++++++++----------------------
>   1 file changed, 28 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index b6da03c26122..727cad6c36fc 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -199,6 +199,7 @@ struct io_rings {
>   
>   enum io_uring_cmd_flags {
>   	IO_URING_F_COMPLETE_DEFER	= 1,
> +	IO_URING_F_UNLOCKED		= 2,
>   	/* int's last bit, sign checks are usually faster than a bit test */
>   	IO_URING_F_NONBLOCK		= INT_MIN,
>   };
> @@ -2926,7 +2927,7 @@ static void kiocb_done(struct kiocb *kiocb, ssize_t ret,
>   			struct io_ring_ctx *ctx = req->ctx;
>   
>   			req_set_fail(req);
> -			if (!(issue_flags & IO_URING_F_NONBLOCK)) {
> +			if (issue_flags & IO_URING_F_UNLOCKED) {
>   				mutex_lock(&ctx->uring_lock);
>   				__io_req_complete(req, issue_flags, ret, cflags);
>   				mutex_unlock(&ctx->uring_lock);
> @@ -3036,7 +3037,7 @@ static struct io_buffer *io_buffer_select(struct io_kiocb *req, size_t *len,
>   {
>   	struct io_buffer *kbuf = req->kbuf;
>   	struct io_buffer *head;
> -	bool needs_lock = !(issue_flags & IO_URING_F_NONBLOCK);
> +	bool needs_lock = issue_flags & IO_URING_F_UNLOCKED;
>   
>   	if (req->flags & REQ_F_BUFFER_SELECTED)
>   		return kbuf;
> @@ -3341,7 +3342,7 @@ static inline int io_rw_prep_async(struct io_kiocb *req, int rw)
>   	int ret;
>   
>   	/* submission path, ->uring_lock should already be taken */
> -	ret = io_import_iovec(rw, req, &iov, &iorw->s, IO_URING_F_NONBLOCK);
> +	ret = io_import_iovec(rw, req, &iov, &iorw->s, 0);
>   	if (unlikely(ret < 0))
>   		return ret;
>   
> @@ -3452,6 +3453,7 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags)
>   	struct iovec *iovec;
>   	struct kiocb *kiocb = &req->rw.kiocb;
>   	bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK;
> +	bool in_worker = issue_flags & IO_URING_F_UNLOCKED;
>   	struct io_async_rw *rw;
>   	ssize_t ret, ret2;
>   
> @@ -3495,7 +3497,7 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags)
>   	if (ret == -EAGAIN || (req->flags & REQ_F_REISSUE)) {
>   		req->flags &= ~REQ_F_REISSUE;
>   		/* IOPOLL retry should happen for io-wq threads */
> -		if (!force_nonblock && !(req->ctx->flags & IORING_SETUP_IOPOLL))
> +		if (in_worker && !(req->ctx->flags & IORING_SETUP_IOPOLL))
>   			goto done;
>   		/* no retry on NONBLOCK nor RWF_NOWAIT */
>   		if (req->flags & REQ_F_NOWAIT)
> @@ -3503,7 +3505,7 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags)
>   		ret = 0;
>   	} else if (ret == -EIOCBQUEUED) {
>   		goto out_free;
> -	} else if (ret == req->result || ret <= 0 || !force_nonblock ||
> +	} else if (ret == req->result || ret <= 0 || in_worker ||
>   		   (req->flags & REQ_F_NOWAIT) || !need_read_all(req)) {
>   		/* read all, failed, already did sync or don't want to retry */
>   		goto done;
> @@ -3581,6 +3583,7 @@ static int io_write(struct io_kiocb *req, unsigned int issue_flags)
>   	struct iovec *iovec;
>   	struct kiocb *kiocb = &req->rw.kiocb;
>   	bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK;
> +	bool in_worker = issue_flags & IO_URING_F_UNLOCKED;
>   	ssize_t ret, ret2;
>   
>   	if (!req_has_async_data(req)) {
> @@ -3651,7 +3654,7 @@ static int io_write(struct io_kiocb *req, unsigned int issue_flags)
>   	/* no retry on NONBLOCK nor RWF_NOWAIT */
>   	if (ret2 == -EAGAIN && (req->flags & REQ_F_NOWAIT))
>   		goto done;
> -	if (!force_nonblock || ret2 != -EAGAIN) {
> +	if (in_worker || ret2 != -EAGAIN) {
>   		/* IOPOLL retry should happen for io-wq threads */
>   		if ((req->ctx->flags & IORING_SETUP_IOPOLL) && ret2 == -EAGAIN)
>   			goto copy_iov;
> @@ -4314,9 +4317,9 @@ static int io_remove_buffers(struct io_kiocb *req, unsigned int issue_flags)
>   	struct io_ring_ctx *ctx = req->ctx;
>   	struct io_buffer *head;
>   	int ret = 0;
> -	bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK;
> +	bool needs_lock = issue_flags & IO_URING_F_UNLOCKED;
>   
> -	io_ring_submit_lock(ctx, !force_nonblock);
> +	io_ring_submit_lock(ctx, needs_lock);
>   
>   	lockdep_assert_held(&ctx->uring_lock);
>   
> @@ -4329,7 +4332,7 @@ static int io_remove_buffers(struct io_kiocb *req, unsigned int issue_flags)
>   
>   	/* complete before unlock, IOPOLL may need the lock */
>   	__io_req_complete(req, issue_flags, ret, 0);
> -	io_ring_submit_unlock(ctx, !force_nonblock);
> +	io_ring_submit_unlock(ctx, needs_lock);
>   	return 0;
>   }
>   
> @@ -4401,9 +4404,9 @@ static int io_provide_buffers(struct io_kiocb *req, unsigned int issue_flags)
>   	struct io_ring_ctx *ctx = req->ctx;
>   	struct io_buffer *head, *list;
>   	int ret = 0;
> -	bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK;
> +	bool needs_lock = issue_flags & IO_URING_F_UNLOCKED;
>   
> -	io_ring_submit_lock(ctx, !force_nonblock);
> +	io_ring_submit_lock(ctx, needs_lock);
>   
>   	lockdep_assert_held(&ctx->uring_lock);
>   
> @@ -4419,7 +4422,7 @@ static int io_provide_buffers(struct io_kiocb *req, unsigned int issue_flags)
>   		req_set_fail(req);
>   	/* complete before unlock, IOPOLL may need the lock */
>   	__io_req_complete(req, issue_flags, ret, 0);
> -	io_ring_submit_unlock(ctx, !force_nonblock);
> +	io_ring_submit_unlock(ctx, needs_lock);
>   	return 0;
>   }
>   
> @@ -6279,6 +6282,7 @@ static int io_async_cancel(struct io_kiocb *req, unsigned int issue_flags)
>   {
>   	struct io_ring_ctx *ctx = req->ctx;
>   	u64 sqe_addr = req->cancel.addr;
> +	bool needs_lock = issue_flags & IO_URING_F_UNLOCKED;
>   	struct io_tctx_node *node;
>   	int ret;
>   
> @@ -6287,7 +6291,7 @@ static int io_async_cancel(struct io_kiocb *req, unsigned int issue_flags)
>   		goto done;
>   
>   	/* slow path, try all io-wq's */
> -	io_ring_submit_lock(ctx, !(issue_flags & IO_URING_F_NONBLOCK));
> +	io_ring_submit_lock(ctx, needs_lock);
>   	ret = -ENOENT;
>   	list_for_each_entry(node, &ctx->tctx_list, ctx_node) {
>   		struct io_uring_task *tctx = node->task->io_uring;
> @@ -6296,7 +6300,7 @@ static int io_async_cancel(struct io_kiocb *req, unsigned int issue_flags)
>   		if (ret != -ENOENT)
>   			break;
>   	}
> -	io_ring_submit_unlock(ctx, !(issue_flags & IO_URING_F_NONBLOCK));
> +	io_ring_submit_unlock(ctx, needs_lock);
>   done:
>   	if (ret < 0)
>   		req_set_fail(req);
> @@ -6323,6 +6327,7 @@ static int io_rsrc_update_prep(struct io_kiocb *req,
>   static int io_files_update(struct io_kiocb *req, unsigned int issue_flags)
>   {
>   	struct io_ring_ctx *ctx = req->ctx;
> +	bool needs_lock = issue_flags & IO_URING_F_UNLOCKED;
>   	struct io_uring_rsrc_update2 up;
>   	int ret;
>   
> @@ -6332,10 +6337,10 @@ static int io_files_update(struct io_kiocb *req, unsigned int issue_flags)
>   	up.tags = 0;
>   	up.resv = 0;
>   
> -	io_ring_submit_lock(ctx, !(issue_flags & IO_URING_F_NONBLOCK));
> +	io_ring_submit_lock(ctx, needs_lock);
>   	ret = __io_register_rsrc_update(ctx, IORING_RSRC_FILE,
>   					&up, req->rsrc_update.nr_args);
> -	io_ring_submit_unlock(ctx, !(issue_flags & IO_URING_F_NONBLOCK));
> +	io_ring_submit_unlock(ctx, needs_lock);
>   
>   	if (ret < 0)
>   		req_set_fail(req);
> @@ -6745,7 +6750,7 @@ static void io_wq_submit_work(struct io_wq_work *work)
>   
>   	if (!ret) {
>   		do {
> -			ret = io_issue_sqe(req, 0);
> +			ret = io_issue_sqe(req, IO_URING_F_UNLOCKED);
>   			/*
>   			 * We can get EAGAIN for polled IO even though we're
>   			 * forcing a sync submission from here, since we can't
> @@ -8333,12 +8338,12 @@ static int io_install_fixed_file(struct io_kiocb *req, struct file *file,
>   				 unsigned int issue_flags, u32 slot_index)
>   {
>   	struct io_ring_ctx *ctx = req->ctx;
> -	bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK;
> +	bool needs_lock = issue_flags & IO_URING_F_UNLOCKED;
>   	bool needs_switch = false;
>   	struct io_fixed_file *file_slot;
>   	int ret = -EBADF;
>   
> -	io_ring_submit_lock(ctx, !force_nonblock);
> +	io_ring_submit_lock(ctx, needs_lock);
>   	if (file->f_op == &io_uring_fops)
>   		goto err;
>   	ret = -ENXIO;
> @@ -8379,7 +8384,7 @@ static int io_install_fixed_file(struct io_kiocb *req, struct file *file,
>   err:
>   	if (needs_switch)
>   		io_rsrc_node_switch(ctx, ctx->file_data);
> -	io_ring_submit_unlock(ctx, !force_nonblock);
> +	io_ring_submit_unlock(ctx, needs_lock);
>   	if (ret)
>   		fput(file);
>   	return ret;
> @@ -8389,11 +8394,12 @@ static int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags)
>   {
>   	unsigned int offset = req->close.file_slot - 1;
>   	struct io_ring_ctx *ctx = req->ctx;
> +	bool needs_lock = issue_flags & IO_URING_F_UNLOCKED;
>   	struct io_fixed_file *file_slot;
>   	struct file *file;
>   	int ret, i;
>   
> -	io_ring_submit_lock(ctx, !(issue_flags & IO_URING_F_NONBLOCK));
> +	io_ring_submit_lock(ctx, needs_lock);
>   	ret = -ENXIO;
>   	if (unlikely(!ctx->file_data))
>   		goto out;
> @@ -8419,7 +8425,7 @@ static int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags)
>   	io_rsrc_node_switch(ctx, ctx->file_data);
>   	ret = 0;
>   out:
> -	io_ring_submit_unlock(ctx, !(issue_flags & IO_URING_F_NONBLOCK));
> +	io_ring_submit_unlock(ctx, needs_lock);
>   	return ret;
>   }
>   
> 

-- 
Pavel Begunkov

Re: [PATCH 2/2] io_uring: implement async hybrid mode for pollable requests

[Hao Xu]

在 2021/10/18 下午8:10, Pavel Begunkov 写道:
> On 10/18/21 11:34, Hao Xu wrote:
>> 在 2021/10/18 下午7:29, Hao Xu 写道:
>>> The current logic of requests with IOSQE_ASYNC is first queueing it to
>>> io-worker, then execute it in a synchronous way. For unbound works like
>>> pollable requests(e.g. read/write a socketfd), the io-worker may stuck
>>> there waiting for events for a long time. And thus other works wait in
>>> the list for a long time too.
>>> Let's introduce a new way for unbound works (currently pollable
>>> requests), with this a request will first be queued to io-worker, then
>>> executed in a nonblock try rather than a synchronous way. Failure of
>>> that leads it to arm poll stuff and then the worker can begin to handle
>>> other works.
>>> The detail process of this kind of requests is:
>>>
>>> step1: original context:
>>>             queue it to io-worker
>>> step2: io-worker context:
>>>             nonblock try(the old logic is a synchronous try here)
>>>                 |
>>>                 |--fail--> arm poll
>>>                              |
>>>                              |--(fail/ready)-->synchronous issue
>>>                              |
>>>                              |--(succeed)-->worker finish it's job, tw
>>>                                             take over the req
>>>
>>> This works much better than the old IOSQE_ASYNC logic in cases where
>>> unbound max_worker is relatively small. In this case, number of
>>> io-worker eazily increments to max_worker, new worker cannot be created
>>> and running workers stuck there handling old works in IOSQE_ASYNC mode.
>>>
>>> In my 64-core machine, set unbound max_worker to 20, run echo-server,
>>> turns out:
>>> (arguments: register_file, connetion number is 1000, message size is 12
>>> Byte)
>>> original IOSQE_ASYNC: 76664.151 tps
>>> after this patch: 166934.985 tps
>>>
>>> Suggested-by: Jens Axboe <axboe@kernel.dk>
>>> Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
>> An irrelevant question: why do we do linked timeout logic in
>> io_wq_submit_work() again regarding that we've already done it in
>> io_queue_async_work().
> 
> Because io_wq_free_work() may enqueue new work (by returning it)
> without going through io_queue_async_work(), and we don't care
> enough to split those cases.
Make sense. Thanks.
> 

Re: [PATCH 1/2] io_uring: split logic of force_nonblock

[Pavel Begunkov]

On 10/18/21 11:29, Hao Xu wrote:
> Currently force_nonblock stands for three meanings:
>   - nowait or not
>   - in an io-worker or not(hold uring_lock or not)
> 
> Let's split the logic to two flags, IO_URING_F_NONBLOCK and
> IO_URING_F_UNLOCKED for convenience of the next patch.
> 
> Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
> ---
>   fs/io_uring.c | 50 ++++++++++++++++++++++++++++----------------------
>   1 file changed, 28 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index b6da03c26122..727cad6c36fc 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -199,6 +199,7 @@ struct io_rings {
>   
>   enum io_uring_cmd_flags {
>   	IO_URING_F_COMPLETE_DEFER	= 1,
> +	IO_URING_F_UNLOCKED		= 2,
>   	/* int's last bit, sign checks are usually faster than a bit test */
>   	IO_URING_F_NONBLOCK		= INT_MIN,
>   };
> @@ -2926,7 +2927,7 @@ static void kiocb_done(struct kiocb *kiocb, ssize_t ret,
>   			struct io_ring_ctx *ctx = req->ctx;
>   
>   			req_set_fail(req);
> -			if (!(issue_flags & IO_URING_F_NONBLOCK)) {
> +			if (issue_flags & IO_URING_F_UNLOCKED) {
>   				mutex_lock(&ctx->uring_lock);
>   				__io_req_complete(req, issue_flags, ret, cflags);
>   				mutex_unlock(&ctx->uring_lock);
> @@ -3036,7 +3037,7 @@ static struct io_buffer *io_buffer_select(struct io_kiocb *req, size_t *len,
>   {
>   	struct io_buffer *kbuf = req->kbuf;
>   	struct io_buffer *head;
> -	bool needs_lock = !(issue_flags & IO_URING_F_NONBLOCK);
> +	bool needs_lock = issue_flags & IO_URING_F_UNLOCKED;
>   
>   	if (req->flags & REQ_F_BUFFER_SELECTED)
>   		return kbuf;
> @@ -3341,7 +3342,7 @@ static inline int io_rw_prep_async(struct io_kiocb *req, int rw)
>   	int ret;
>   
>   	/* submission path, ->uring_lock should already be taken */
> -	ret = io_import_iovec(rw, req, &iov, &iorw->s, IO_URING_F_NONBLOCK);
> +	ret = io_import_iovec(rw, req, &iov, &iorw->s, 0);
>   	if (unlikely(ret < 0))
>   		return ret;
>   
> @@ -3452,6 +3453,7 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags)
>   	struct iovec *iovec;
>   	struct kiocb *kiocb = &req->rw.kiocb;
>   	bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK;
> +	bool in_worker = issue_flags & IO_URING_F_UNLOCKED;

io_read shouldn't have notion of worker or whatever. I'd say let's
leave only force_nonblock here.

I assume 2/2 relies ot it, but if so you can make sure it ends up
in sync (!force_nonblock) at some point if all other ways fail.


>   	struct io_async_rw *rw;
>   	ssize_t ret, ret2;
>   
> @@ -3495,7 +3497,7 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags)
>   	if (ret == -EAGAIN || (req->flags & REQ_F_REISSUE)) {
>   		req->flags &= ~REQ_F_REISSUE;
>   		/* IOPOLL retry should happen for io-wq threads */
> -		if (!force_nonblock && !(req->ctx->flags & IORING_SETUP_IOPOLL))
> +		if (in_worker && !(req->ctx->flags & IORING_SETUP_IOPOLL))
>   			goto done;
>   		/* no retry on NONBLOCK nor RWF_NOWAIT */
>   		if (req->flags & REQ_F_NOWAIT)
> @@ -3503,7 +3505,7 @@ static int io_read(struct io_kiocb *req, unsigned int issue_flags)
>   		ret = 0;
>   	} else if (ret == -EIOCBQUEUED) {
>   		goto out_free;
> -	} else if (ret == req->result || ret <= 0 || !force_nonblock ||
> +	} else if (ret == req->result || ret <= 0 || in_worker ||
>   		   (req->flags & REQ_F_NOWAIT) || !need_read_all(req)) {
>   		/* read all, failed, already did sync or don't want to retry */
>   		goto done;
> @@ -3581,6 +3583,7 @@ static int io_write(struct io_kiocb *req, unsigned int issue_flags)
>   	struct iovec *iovec;
>   	struct kiocb *kiocb = &req->rw.kiocb;
>   	bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK;
> +	bool in_worker = issue_flags & IO_URING_F_UNLOCKED;
>   	ssize_t ret, ret2;
>   
>   	if (!req_has_async_data(req)) {
> @@ -3651,7 +3654,7 @@ static int io_write(struct io_kiocb *req, unsigned int issue_flags)
>   	/* no retry on NONBLOCK nor RWF_NOWAIT */
>   	if (ret2 == -EAGAIN && (req->flags & REQ_F_NOWAIT))
>   		goto done;
> -	if (!force_nonblock || ret2 != -EAGAIN) {
> +	if (in_worker || ret2 != -EAGAIN) {
>   		/* IOPOLL retry should happen for io-wq threads */
>   		if ((req->ctx->flags & IORING_SETUP_IOPOLL) && ret2 == -EAGAIN)
>   			goto copy_iov;
> @@ -4314,9 +4317,9 @@ static int io_remove_buffers(struct io_kiocb *req, unsigned int issue_flags)
>   	struct io_ring_ctx *ctx = req->ctx;
>   	struct io_buffer *head;
>   	int ret = 0;
> -	bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK;
> +	bool needs_lock = issue_flags & IO_URING_F_UNLOCKED;
>   
> -	io_ring_submit_lock(ctx, !force_nonblock);
> +	io_ring_submit_lock(ctx, needs_lock);
>   
>   	lockdep_assert_held(&ctx->uring_lock);
>   
> @@ -4329,7 +4332,7 @@ static int io_remove_buffers(struct io_kiocb *req, unsigned int issue_flags)
>   
>   	/* complete before unlock, IOPOLL may need the lock */
>   	__io_req_complete(req, issue_flags, ret, 0);
> -	io_ring_submit_unlock(ctx, !force_nonblock);
> +	io_ring_submit_unlock(ctx, needs_lock);
>   	return 0;
>   }
>   
> @@ -4401,9 +4404,9 @@ static int io_provide_buffers(struct io_kiocb *req, unsigned int issue_flags)
>   	struct io_ring_ctx *ctx = req->ctx;
>   	struct io_buffer *head, *list;
>   	int ret = 0;
> -	bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK;
> +	bool needs_lock = issue_flags & IO_URING_F_UNLOCKED;
>   
> -	io_ring_submit_lock(ctx, !force_nonblock);
> +	io_ring_submit_lock(ctx, needs_lock);
>   
>   	lockdep_assert_held(&ctx->uring_lock);
>   
> @@ -4419,7 +4422,7 @@ static int io_provide_buffers(struct io_kiocb *req, unsigned int issue_flags)
>   		req_set_fail(req);
>   	/* complete before unlock, IOPOLL may need the lock */
>   	__io_req_complete(req, issue_flags, ret, 0);
> -	io_ring_submit_unlock(ctx, !force_nonblock);
> +	io_ring_submit_unlock(ctx, needs_lock);
>   	return 0;
>   }
>   
> @@ -6279,6 +6282,7 @@ static int io_async_cancel(struct io_kiocb *req, unsigned int issue_flags)
>   {
>   	struct io_ring_ctx *ctx = req->ctx;
>   	u64 sqe_addr = req->cancel.addr;
> +	bool needs_lock = issue_flags & IO_URING_F_UNLOCKED;
>   	struct io_tctx_node *node;
>   	int ret;
>   
> @@ -6287,7 +6291,7 @@ static int io_async_cancel(struct io_kiocb *req, unsigned int issue_flags)
>   		goto done;
>   
>   	/* slow path, try all io-wq's */
> -	io_ring_submit_lock(ctx, !(issue_flags & IO_URING_F_NONBLOCK));
> +	io_ring_submit_lock(ctx, needs_lock);
>   	ret = -ENOENT;
>   	list_for_each_entry(node, &ctx->tctx_list, ctx_node) {
>   		struct io_uring_task *tctx = node->task->io_uring;
> @@ -6296,7 +6300,7 @@ static int io_async_cancel(struct io_kiocb *req, unsigned int issue_flags)
>   		if (ret != -ENOENT)
>   			break;
>   	}
> -	io_ring_submit_unlock(ctx, !(issue_flags & IO_URING_F_NONBLOCK));
> +	io_ring_submit_unlock(ctx, needs_lock);
>   done:
>   	if (ret < 0)
>   		req_set_fail(req);
> @@ -6323,6 +6327,7 @@ static int io_rsrc_update_prep(struct io_kiocb *req,
>   static int io_files_update(struct io_kiocb *req, unsigned int issue_flags)
>   {
>   	struct io_ring_ctx *ctx = req->ctx;
> +	bool needs_lock = issue_flags & IO_URING_F_UNLOCKED;
>   	struct io_uring_rsrc_update2 up;
>   	int ret;
>   
> @@ -6332,10 +6337,10 @@ static int io_files_update(struct io_kiocb *req, unsigned int issue_flags)
>   	up.tags = 0;
>   	up.resv = 0;
>   
> -	io_ring_submit_lock(ctx, !(issue_flags & IO_URING_F_NONBLOCK));
> +	io_ring_submit_lock(ctx, needs_lock);
>   	ret = __io_register_rsrc_update(ctx, IORING_RSRC_FILE,
>   					&up, req->rsrc_update.nr_args);
> -	io_ring_submit_unlock(ctx, !(issue_flags & IO_URING_F_NONBLOCK));
> +	io_ring_submit_unlock(ctx, needs_lock);
>   
>   	if (ret < 0)
>   		req_set_fail(req);
> @@ -6745,7 +6750,7 @@ static void io_wq_submit_work(struct io_wq_work *work)
>   
>   	if (!ret) {
>   		do {
> -			ret = io_issue_sqe(req, 0);
> +			ret = io_issue_sqe(req, IO_URING_F_UNLOCKED);
>   			/*
>   			 * We can get EAGAIN for polled IO even though we're
>   			 * forcing a sync submission from here, since we can't
> @@ -8333,12 +8338,12 @@ static int io_install_fixed_file(struct io_kiocb *req, struct file *file,
>   				 unsigned int issue_flags, u32 slot_index)
>   {
>   	struct io_ring_ctx *ctx = req->ctx;
> -	bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK;
> +	bool needs_lock = issue_flags & IO_URING_F_UNLOCKED;
>   	bool needs_switch = false;
>   	struct io_fixed_file *file_slot;
>   	int ret = -EBADF;
>   
> -	io_ring_submit_lock(ctx, !force_nonblock);
> +	io_ring_submit_lock(ctx, needs_lock);
>   	if (file->f_op == &io_uring_fops)
>   		goto err;
>   	ret = -ENXIO;
> @@ -8379,7 +8384,7 @@ static int io_install_fixed_file(struct io_kiocb *req, struct file *file,
>   err:
>   	if (needs_switch)
>   		io_rsrc_node_switch(ctx, ctx->file_data);
> -	io_ring_submit_unlock(ctx, !force_nonblock);
> +	io_ring_submit_unlock(ctx, needs_lock);
>   	if (ret)
>   		fput(file);
>   	return ret;
> @@ -8389,11 +8394,12 @@ static int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags)
>   {
>   	unsigned int offset = req->close.file_slot - 1;
>   	struct io_ring_ctx *ctx = req->ctx;
> +	bool needs_lock = issue_flags & IO_URING_F_UNLOCKED;
>   	struct io_fixed_file *file_slot;
>   	struct file *file;
>   	int ret, i;
>   
> -	io_ring_submit_lock(ctx, !(issue_flags & IO_URING_F_NONBLOCK));
> +	io_ring_submit_lock(ctx, needs_lock);
>   	ret = -ENXIO;
>   	if (unlikely(!ctx->file_data))
>   		goto out;
> @@ -8419,7 +8425,7 @@ static int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags)
>   	io_rsrc_node_switch(ctx, ctx->file_data);
>   	ret = 0;
>   out:
> -	io_ring_submit_unlock(ctx, !(issue_flags & IO_URING_F_NONBLOCK));
> +	io_ring_submit_unlock(ctx, needs_lock);
>   	return ret;
>   }
>   
> 

-- 
Pavel Begunkov

Re: [PATCH v2 0/2] async hybrid for pollable requests

[Pavel Begunkov]

On 10/18/21 11:29, Hao Xu wrote:
> 1/2 is a prep patch. 2/2 is the main one.
> The good thing: see commit message.
> the side effect: for normal io-worker path, added two if and two local
> variables. for FORCE_ASYNC path, added three if and several dereferences
> 
> I think it is fine since the io-worker path is not the fast path, and
> the benefit of this patchset is worth it.

We don't care about overhead in iowq, but would be better to get rid
of the in_worker in io_read(). See comments to 1/2.

Btw, you told that it performs better comparing to normal
IOSQE_ASYNC. I'm confused, didn't we agree that it can be
merged into IOSQE_ASYNC without extra flags?

> 
> Btw, we need to tweak the io-cancel.c a bit, not a big problem.
> liburing tests will come later.
> 
> v1-->v2:
>   - split logic of force_nonblock
>   - tweak added code in io_wq_submit_work to reduce overhead
>   from Pavel's commments.
> 
> Hao Xu (2):
>    io_uring: split logic of force_nonblock
>    io_uring: implement async hybrid mode for pollable requests
> 
>   fs/io_uring.c                 | 85 ++++++++++++++++++++++++++---------
>   include/uapi/linux/io_uring.h |  4 +-
>   2 files changed, 66 insertions(+), 23 deletions(-)
> 

-- 
Pavel Begunkov

Re: [PATCH v2 0/2] async hybrid for pollable requests

[Hao Xu]

在 2021/10/18 下午8:31, Pavel Begunkov 写道:
> On 10/18/21 11:29, Hao Xu wrote:
>> 1/2 is a prep patch. 2/2 is the main one.
>> The good thing: see commit message.
>> the side effect: for normal io-worker path, added two if and two local
>> variables. for FORCE_ASYNC path, added three if and several dereferences
>>
>> I think it is fine since the io-worker path is not the fast path, and
>> the benefit of this patchset is worth it.
> 
> We don't care about overhead in iowq, but would be better to get rid
> of the in_worker in io_read(). See comments to 1/2.
> 
> Btw, you told that it performs better comparing to normal
> IOSQE_ASYNC. I'm confused, didn't we agree that it can be
> merged into IOSQE_ASYNC without extra flags?
I said 'better than the old logic IOSQE_ASYNC logic for pollable cases'..
> 
>>
>> Btw, we need to tweak the io-cancel.c a bit, not a big problem.
>> liburing tests will come later.
>>
>> v1-->v2:
>>   - split logic of force_nonblock
>>   - tweak added code in io_wq_submit_work to reduce overhead
>>   from Pavel's commments.
>>
>> Hao Xu (2):
>>    io_uring: split logic of force_nonblock
>>    io_uring: implement async hybrid mode for pollable requests
>>
>>   fs/io_uring.c                 | 85 ++++++++++++++++++++++++++---------
>>   include/uapi/linux/io_uring.h |  4 +-
>>   2 files changed, 66 insertions(+), 23 deletions(-)
>>
> 

Re: [PATCH 1/2] io_uring: split logic of force_nonblock

[Hao Xu]

在 2021/10/18 下午8:27, Pavel Begunkov 写道:
> On 10/18/21 11:29, Hao Xu wrote:
>> Currently force_nonblock stands for three meanings:
>>   - nowait or not
>>   - in an io-worker or not(hold uring_lock or not)
>>
>> Let's split the logic to two flags, IO_URING_F_NONBLOCK and
>> IO_URING_F_UNLOCKED for convenience of the next patch.
>>
>> Signed-off-by: Hao Xu <haoxu@linux.alibaba.com>
>> ---
>>   fs/io_uring.c | 50 ++++++++++++++++++++++++++++----------------------
>>   1 file changed, 28 insertions(+), 22 deletions(-)
>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index b6da03c26122..727cad6c36fc 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -199,6 +199,7 @@ struct io_rings {
>>   enum io_uring_cmd_flags {
>>       IO_URING_F_COMPLETE_DEFER    = 1,
>> +    IO_URING_F_UNLOCKED        = 2,
>>       /* int's last bit, sign checks are usually faster than a bit 
>> test */
>>       IO_URING_F_NONBLOCK        = INT_MIN,
>>   };
>> @@ -2926,7 +2927,7 @@ static void kiocb_done(struct kiocb *kiocb, 
>> ssize_t ret,
>>               struct io_ring_ctx *ctx = req->ctx;
>>               req_set_fail(req);
>> -            if (!(issue_flags & IO_URING_F_NONBLOCK)) {
>> +            if (issue_flags & IO_URING_F_UNLOCKED) {
>>                   mutex_lock(&ctx->uring_lock);
>>                   __io_req_complete(req, issue_flags, ret, cflags);
>>                   mutex_unlock(&ctx->uring_lock);
>> @@ -3036,7 +3037,7 @@ static struct io_buffer *io_buffer_select(struct 
>> io_kiocb *req, size_t *len,
>>   {
>>       struct io_buffer *kbuf = req->kbuf;
>>       struct io_buffer *head;
>> -    bool needs_lock = !(issue_flags & IO_URING_F_NONBLOCK);
>> +    bool needs_lock = issue_flags & IO_URING_F_UNLOCKED;
>>       if (req->flags & REQ_F_BUFFER_SELECTED)
>>           return kbuf;
>> @@ -3341,7 +3342,7 @@ static inline int io_rw_prep_async(struct 
>> io_kiocb *req, int rw)
>>       int ret;
>>       /* submission path, ->uring_lock should already be taken */
>> -    ret = io_import_iovec(rw, req, &iov, &iorw->s, IO_URING_F_NONBLOCK);
>> +    ret = io_import_iovec(rw, req, &iov, &iorw->s, 0);
>>       if (unlikely(ret < 0))
>>           return ret;
>> @@ -3452,6 +3453,7 @@ static int io_read(struct io_kiocb *req, 
>> unsigned int issue_flags)
>>       struct iovec *iovec;
>>       struct kiocb *kiocb = &req->rw.kiocb;
>>       bool force_nonblock = issue_flags & IO_URING_F_NONBLOCK;
>> +    bool in_worker = issue_flags & IO_URING_F_UNLOCKED;
> 
> io_read shouldn't have notion of worker or whatever. I'd say let's
> leave only force_nonblock here.
> 
> I assume 2/2 relies ot it, but if so you can make sure it ends up
> in sync (!force_nonblock) at some point if all other ways fail.
I re-read the code, found you're right, will send v3.

Re: [PATCH v2 0/2] async hybrid for pollable requests

[Hao Xu]

在 2021/10/18 下午8:31, Pavel Begunkov 写道:
> On 10/18/21 11:29, Hao Xu wrote:
>> 1/2 is a prep patch. 2/2 is the main one.
>> The good thing: see commit message.
>> the side effect: for normal io-worker path, added two if and two local
>> variables. for FORCE_ASYNC path, added three if and several dereferences
>>
>> I think it is fine since the io-worker path is not the fast path, and
>> the benefit of this patchset is worth it.
> 
> We don't care about overhead in iowq, but would be better to get rid
> of the in_worker in io_read(). See comments to 1/2.
> 
> Btw, you told that it performs better comparing to normal
> IOSQE_ASYNC. I'm confused, didn't we agree that it can be
> merged into IOSQE_ASYNC without extra flags?
I see what you are saying, forgot to remove the flag
stuff..will be more careful.
> 
>>
>> Btw, we need to tweak the io-cancel.c a bit, not a big problem.
>> liburing tests will come later.
>>
>> v1-->v2:
>>   - split logic of force_nonblock
>>   - tweak added code in io_wq_submit_work to reduce overhead
>>   from Pavel's commments.
>>
>> Hao Xu (2):
>>    io_uring: split logic of force_nonblock
>>    io_uring: implement async hybrid mode for pollable requests
>>
>>   fs/io_uring.c                 | 85 ++++++++++++++++++++++++++---------
>>   include/uapi/linux/io_uring.h |  4 +-
>>   2 files changed, 66 insertions(+), 23 deletions(-)
>>
>