[PATCH 1/2] io_uring: make req from defer and link list not touch async list

[PATCH 1/2] io_uring: make req from defer and link list not touch async list

[Zhengyuan Liu]

We would queue a work for each req in defer and link list without
increasing async->cnt, so we shouldn't decrease it while exiting
from workqueue as well as shouldn't process the req in async list.

Signed-off-by: Zhengyuan Liu <liuzhengyuan@kylinos.cn>
---
 fs/io_uring.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 7e932c572f26..3e48fd7cd08f 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -333,7 +333,8 @@ struct io_kiocb {
 #define REQ_F_IO_DRAIN		16	/* drain existing IO first */
 #define REQ_F_IO_DRAINED	32	/* drain done */
 #define REQ_F_LINK		64	/* linked sqes */
-#define REQ_F_FAIL_LINK		128	/* fail rest of links */
+#define REQ_F_LINKED		128	/* linked sqes done */
+#define REQ_F_FAIL_LINK		256	/* fail rest of links */
 	u64			user_data;
 	u32			result;
 	u32			sequence;
@@ -630,6 +631,7 @@ static void io_req_link_next(struct io_kiocb *req)
 			nxt->flags |= REQ_F_LINK;
 		}
 
+		nxt->flags |= REQ_F_LINKED;
 		INIT_WORK(&nxt->work, io_sq_wq_submit_work);
 		queue_work(req->ctx->sqo_wq, &nxt->work);
 	}
@@ -1845,6 +1847,10 @@ static void io_sq_wq_submit_work(struct work_struct *work)
 		/* async context always use a copy of the sqe */
 		kfree(sqe);
 
+		/* req from defer and link list needn't dec async_list->cnt */
+		if (req->flags & (REQ_F_IO_DRAINED | REQ_F_LINKED))
+			goto out;
+
 		if (!async_list)
 			break;
 		if (!list_empty(&req_list)) {
@@ -1892,6 +1898,7 @@ static void io_sq_wq_submit_work(struct work_struct *work)
 		}
 	}
 
+out:
 	if (cur_mm) {
 		set_fs(old_fs);
 		unuse_mm(cur_mm);
-- 
2.19.1

[PATCH 2/2] io_uring: fix the judging condition in io_sequence_defer

[Zhengyuan Liu]

sq->cached_sq_head and cq->cached_cq_tail are both unsigned int.
if cached_sq_head gets overflowed before cached_cq_tail, then we
may miss a barrier req. As cached_cq_tail moved always following
cached_sq_head, the NQ should be enough.

Signed-off-by: Zhengyuan Liu <liuzhengyuan@kylinos.cn>
---
 fs/io_uring.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 3e48fd7cd08f..55294ef82102 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -429,7 +429,7 @@ static inline bool io_sequence_defer(struct io_ring_ctx *ctx,
 	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->sq_ring->dropped;
+	return req->sequence != ctx->cached_cq_tail + ctx->sq_ring->dropped;
 }
 
 static struct io_kiocb *io_get_deferred_req(struct io_ring_ctx *ctx)
-- 
2.19.1

Re: [PATCH 2/2] io_uring: fix the judging condition in io_sequence_defer

[Jens Axboe]

On 7/12/19 9:58 PM, Zhengyuan Liu wrote:
> sq->cached_sq_head and cq->cached_cq_tail are both unsigned int.
> if cached_sq_head gets overflowed before cached_cq_tail, then we
> may miss a barrier req. As cached_cq_tail moved always following
> cached_sq_head, the NQ should be enough.

This (and the previous patch) looks fine to me, just wondering if
you had a test case showing this issue?

-- 
Jens Axboe

Re: [PATCH 2/2] io_uring: fix the judging condition in io_sequence_defer

[Jens Axboe]

On 7/14/19 8:34 PM, Zhengyuan Liu wrote:
> 
> On 7/14/19 5:43 AM, Jens Axboe wrote:
>> On 7/12/19 9:58 PM, Zhengyuan Liu wrote:
>>> sq->cached_sq_head and cq->cached_cq_tail are both unsigned int.
>>> if cached_sq_head gets overflowed before cached_cq_tail, then we
>>> may miss a barrier req. As cached_cq_tail moved always following
>>> cached_sq_head, the NQ should be enough.
>> This (and the previous patch) looks fine to me, just wondering if
>> you had a test case showing this issue?
>>
> Hi.
> 
> Actually I don't have a real test case,  but I have emulated a test case
> and showed the issue.
> 
> Let's preset those count to close overflowed status at the begin:
> 
>         @@ -2979,6 +2987,10 @@ static int io_allocate_scq_urings(struct
> io_ring_ctx *ctx,
>             cq_ring->ring_entries = p->cq_entries;
>             ctx->cq_mask = cq_ring->ring_mask;
>             ctx->cq_entries = cq_ring->ring_entries;
> +
> +          ctx->cached_sq_head = ctx->sq_ring->r.head =
> ctx->sq_ring->r.tail = 0xfffffff8;
> +          ctx->cached_cq_tail = ctx->cq_ring->r.head =
> ctx->cq_ring->r.tail = 0xfffffff8
> +
>             return 0;
>      }
> 
> And queue serveral sqes  following a io_uring_enter like this:
> 
>       write1 write2 ... write8  barrier write9 write10
> 
> Then we can miss the barrier almost every time.

No problem, just curious if you had a test case, since then I'd add
it to the arsenal.

I've applied this one, thank you.

-- 
Jens Axboe

Re: [PATCH 1/2] io_uring: make req from defer and link list not touch async list

[Jens Axboe]

On 7/12/19 9:58 PM, Zhengyuan Liu wrote:
> We would queue a work for each req in defer and link list without
> increasing async->cnt, so we shouldn't decrease it while exiting
> from workqueue as well as shouldn't process the req in async list.

Took a closer look at this one. First comment, can you rewrite
the subject line? I honestly don't quite know what that is
trying to say.

Apart from that:

> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 7e932c572f26..3e48fd7cd08f 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -333,7 +333,8 @@ struct io_kiocb {
>   #define REQ_F_IO_DRAIN		16	/* drain existing IO first */
>   #define REQ_F_IO_DRAINED	32	/* drain done */
>   #define REQ_F_LINK		64	/* linked sqes */
> -#define REQ_F_FAIL_LINK		128	/* fail rest of links */
> +#define REQ_F_LINKED		128	/* linked sqes done */
> +#define REQ_F_FAIL_LINK		256	/* fail rest of links */

_LINKED is poorly chosen, hard to know what the difference between
_LINK and _LINKED is here, they seem identical. Please rename it to
_LINK_DONE or something like that.

Thanks!

-- 
Jens Axboe