io_uring: fix race with shadow drain deferrals

When we go and queue requests with drain, we check if we need to defer based on sequence. This is done safely under the lock, but then we drop the lock before actually inserting the shadow. If the original request is found on the deferred list by another completion in the mean time, it could have been started AND completed by the time we insert the shadow, which will stall the queue.

After re-grabbing the completion lock, check if the original request is still in the deferred list. If it isn't, then we know that someone else already found and issued it. If that happened, then our job is done, we can simply free the shadow.

Cc Jackie Liu <[email protected]>

Fixes 4fe2c963154c ("io_uring: add support for link with drain")

Signed-off-by Jens Axboe <[email protected]>

Quoted 7 lines · Jens Axboe

When we go and queue requests with drain, we check if we need to defer based on sequence. This is done safely under the lock, but then we drop the lock before actually inserting the shadow. If the original request is found on the deferred list by another completion in the mean time, it could have been started AND completed by the time we insert the shadow, which will stall the queue.

Quoted 8 lines

After re-grabbing the completion lock, check if the original request is still in the deferred list. If it isn't, then we know that someone else already found and issued it. If that happened, then our job is done, we can simply free the shadow.

Cc Jackie Liu <[email protected]>

Fixes 4fe2c963154c ("io_uring: add support for link with drain")

Signed-off-by Jens Axboe <[email protected]>

BTW, the other solution here is to not release the completion_lock if we're going to return -EIOCBQUEUED, and let the caller do what it needs before releasing it. That'd look something like this, with some sparse annotations to keep things happy.

I think the original I posted here is easier to follow, and the deferral list is going to be tiny in general so it won't really add any extra overhead.

Let me know what you think and prefer.

fs/io_uring.c +11 -1

index 6175e2e195c0..0d1f33bcedc0 100644
@@ -2552,6 +2552,11 @@ static int io_async_cancel(struct io_kiocb *req, const struct io_uring_sqe *sqe,
 	return 0;
 }
 
+/*
+ * Returns with ctx->completion_lock held if -EIOCBQUEUED is returned, so
+ * the caller can make decisions based on the deferral without worrying about
+ * the request being found and issued in the mean time.
+ */
 static int io_req_defer(struct io_kiocb *req)
 {
 	const struct io_uring_sqe *sqe = req->submit.sqe;

@@ -2579,7 +2584,7 @@ static int io_req_defer(struct io_kiocb *req)
 
 	trace_io_uring_defer(ctx, req, false);
 	list_add_tail(&req->list, &ctx->defer_list);
-	spin_unlock_irq(&ctx->completion_lock);
+	__release(&ctx->completion_lock);
 	return -EIOCBQUEUED;
 }
 

@@ -2954,6 +2959,7 @@ static void __io_queue_sqe(struct io_kiocb *req)
 
 static void io_queue_sqe(struct io_kiocb *req)
 {
+	struct io_ring_ctx *ctx = req->ctx;
 	int ret;
 
 	ret = io_req_defer(req);

@@ -2963,6 +2969,9 @@ static void io_queue_sqe(struct io_kiocb *req)
 			if (req->flags & REQ_F_LINK)
 				req->flags |= REQ_F_FAIL_LINK;
 			io_double_put_req(req);
+		} else {
+			__acquire(&ctx->completion_lock);
+			spin_unlock_irq(&ctx->completion_lock);
 		}
 	} else
 		__io_queue_sqe(req);

@@ -3001,16 +3010,17 @@ static void io_queue_link_head(struct io_kiocb *req, struct io_kiocb *shadow)
 				__io_free_req(shadow);
 			return;
 		}
+		__acquire(&ctx->completion_lock);
 	} else {
 		/*
 		 * If ret == 0 means that all IOs in front of link io are
 		 * running done. let's queue link head.

2019年11月21日 09:32，Jackie Liu <[email protected]> 写道：

Quoted 81 lines · 2019年11月21日 07:58，Jens Axboe <[email protected]> 写道：

Quoted 7 lines · Jens Axboe

When we go and queue requests with drain, we check if we need to defer based on sequence. This is done safely under the lock, but then we drop the lock before actually inserting the shadow. If the original request is found on the deferred list by another completion in the mean time, it could have been started AND completed by the time we insert the shadow, which will stall the queue.

Quoted 8 lines

After re-grabbing the completion lock, check if the original request is still in the deferred list. If it isn't, then we know that someone else already found and issued it. If that happened, then our job is done, we can simply free the shadow.

Cc Jackie Liu <[email protected]>

Fixes 4fe2c963154c ("io_uring: add support for link with drain")

Signed-off-by Jens Axboe <[email protected]>

BTW, the other solution here is to not release the completion_lock if we're going to return -EIOCBQUEUED, and let the caller do what it needs before releasing it. That'd look something like this, with some sparse annotations to keep things happy.

I think the original I posted here is easier to follow, and the deferral list is going to be tiny in general so it won't really add any extra overhead.

Let me know what you think and prefer.

fs/io_uring.c +11 -1

index 6175e2e195c0..0d1f33bcedc0 100644
@@ -2552,6 +2552,11 @@ static int io_async_cancel(struct io_kiocb *req, const struct io_uring_sqe *sqe,
 	return 0;
 }
 
+/*
+ * Returns with ctx->completion_lock held if -EIOCBQUEUED is returned, so
+ * the caller can make decisions based on the deferral without worrying about
+ * the request being found and issued in the mean time.
+ */
 static int io_req_defer(struct io_kiocb *req)
 {
 	const struct io_uring_sqe *sqe = req->submit.sqe;

@@ -2579,7 +2584,7 @@ static int io_req_defer(struct io_kiocb *req)
 
 	trace_io_uring_defer(ctx, req, false);
 	list_add_tail(&req->list, &ctx->defer_list);
-	spin_unlock_irq(&ctx->completion_lock);
+	__release(&ctx->completion_lock);
 	return -EIOCBQUEUED;
 }
 

@@ -2954,6 +2959,7 @@ static void __io_queue_sqe(struct io_kiocb *req)
 
 static void io_queue_sqe(struct io_kiocb *req)
 {
+	struct io_ring_ctx *ctx = req->ctx;
 	int ret;
 
 	ret = io_req_defer(req);

@@ -2963,6 +2969,9 @@ static void io_queue_sqe(struct io_kiocb *req)
 			if (req->flags & REQ_F_LINK)
 				req->flags |= REQ_F_FAIL_LINK;
 			io_double_put_req(req);
+		} else {
+			__acquire(&ctx->completion_lock);
+			spin_unlock_irq(&ctx->completion_lock);
 		}
 	} else
 		__io_queue_sqe(req);

@@ -3001,16 +3010,17 @@ static void io_queue_link_head(struct io_kiocb *req, struct io_kiocb *shadow)
 				__io_free_req(shadow);
 			return;
 		}
+		__acquire(&ctx->completion_lock);
 	} else {
 		/*
 		 * If ret == 0 means that all IOs in front of link io are
 		 * running done. let's queue link head.

fs/io_uring.c static void io_queue_link_head(struct io_kiocb *req, struct io_kiocb *shadow) · line 3026

Quoted 10 lines

fs/io_uring.c +1 -1

 		 */
 		need_submit = true;
+		spin_lock_irq(&ctx->completion_lock);
 	}
 
 	/* Insert shadow req to defer_list, blocking next IOs */
-	spin_lock_irq(&ctx->completion_lock);
 	trace_io_uring_defer(ctx, shadow, true);
 	list_add_tail(&shadow->list, &ctx->defer_list);
 	spin_unlock_irq(&ctx->completion_lock);

This is indeed a potential lock issue, thanks, I am prefer this solution, clearer than first one. But It may be a bit difficult for other people who read the code, use 'io_req_defer_may_lock'?

who about this?

fs/io_uring.c +1 -1

index 5ad652f..6fdaeb1 100644
@@ -2469,7 +2469,7 @@ static int io_async_cancel(struct io_kiocb *req, const struct io_uring_sqe *sqe,
       return 0;
 }
 
-static int io_req_defer(struct io_kiocb *req)
+static int __io_req_defer(struct io_kiocb *req)
 {
       const struct io_uring_sqe *sqe = req->submit.sqe;
       struct io_uring_sqe *sqe_copy;

fs/io_uring.c +9 -4

       spin_unlock_irq(&ctx->completion_lock);
-       return-EIOCBQUEUED;
+       return ret;
+}
+
+static int io_req_defer_may_lock(struct io_kiocb *req)
+{
+       return __io_req_defer(req);
+
 }
 
 static int __io_submit_sqe(struct io_kiocb *req, struct io_kiocb **nxt,

@@ -2927,7 +2940,7 @@ static int io_queue_link_head(struct io_kiocb *req, struct io_kiocb *shadow)
        * list.
        */
       req->flags |= REQ_F_IO_DRAIN;
-       ret = io_req_defer(req);
+       ret = io_req_defer_may_lock(req);
       if (ret) {
               if (ret != -EIOCBQUEUED) {
                       io_cqring_add_event(req, ret);

@@ -2941,10 +2954,10 @@ static int io_queue_link_head(struct io_kiocb *req, struct io_kiocb *shadow)
                * running done. let's queue link head.
                */
               need_submit = true;
+               spin_lock_irq(&ctx->completion_lock);
       }
 
       /* Insert shadow req to defer_list, blocking next IOs */
-       spin_lock_irq(&ctx->completion_lock);
       trace_io_uring_defer(ctx, shadow, true);
       list_add_tail(&shadow->list, &ctx->defer_list);
       spin_unlock_irq(&ctx->completion_lock);
 
--

BR, Jackie Liu

Quoted 117 lines

2019年11月21日 09:32，Jackie Liu <[email protected]> 写道：

Quoted 81 lines · 2019年11月21日 07:58，Jens Axboe <[email protected]> 写道：

Quoted 7 lines · Jens Axboe

When we go and queue requests with drain, we check if we need to defer based on sequence. This is done safely under the lock, but then we drop the lock before actually inserting the shadow. If the original request is found on the deferred list by another completion in the mean time, it could have been started AND completed by the time we insert the shadow, which will stall the queue.

Quoted 8 lines

After re-grabbing the completion lock, check if the original request is still in the deferred list. If it isn't, then we know that someone else already found and issued it. If that happened, then our job is done, we can simply free the shadow.

Cc Jackie Liu <[email protected]>

Fixes 4fe2c963154c ("io_uring: add support for link with drain")

Signed-off-by Jens Axboe <[email protected]>

BTW, the other solution here is to not release the completion_lock if we're going to return -EIOCBQUEUED, and let the caller do what it needs before releasing it. That'd look something like this, with some sparse annotations to keep things happy.

I think the original I posted here is easier to follow, and the deferral list is going to be tiny in general so it won't really add any extra overhead.

Let me know what you think and prefer.

fs/io_uring.c +11 -1

index 6175e2e195c0..0d1f33bcedc0 100644
@@ -2552,6 +2552,11 @@ static int io_async_cancel(struct io_kiocb *req, const struct io_uring_sqe *sqe,
 	return 0;
 }
 
+/*
+ * Returns with ctx->completion_lock held if -EIOCBQUEUED is returned, so
+ * the caller can make decisions based on the deferral without worrying about
+ * the request being found and issued in the mean time.
+ */
 static int io_req_defer(struct io_kiocb *req)
 {
 	const struct io_uring_sqe *sqe = req->submit.sqe;

@@ -2579,7 +2584,7 @@ static int io_req_defer(struct io_kiocb *req)
 
 	trace_io_uring_defer(ctx, req, false);
 	list_add_tail(&req->list, &ctx->defer_list);
-	spin_unlock_irq(&ctx->completion_lock);
+	__release(&ctx->completion_lock);
 	return -EIOCBQUEUED;
 }
 

@@ -2954,6 +2959,7 @@ static void __io_queue_sqe(struct io_kiocb *req)
 
 static void io_queue_sqe(struct io_kiocb *req)
 {
+	struct io_ring_ctx *ctx = req->ctx;
 	int ret;
 
 	ret = io_req_defer(req);

@@ -2963,6 +2969,9 @@ static void io_queue_sqe(struct io_kiocb *req)
 			if (req->flags & REQ_F_LINK)
 				req->flags |= REQ_F_FAIL_LINK;
 			io_double_put_req(req);
+		} else {
+			__acquire(&ctx->completion_lock);
+			spin_unlock_irq(&ctx->completion_lock);
 		}
 	} else
 		__io_queue_sqe(req);

@@ -3001,16 +3010,17 @@ static void io_queue_link_head(struct io_kiocb *req, struct io_kiocb *shadow)
 				__io_free_req(shadow);
 			return;
 		}
+		__acquire(&ctx->completion_lock);
 	} else {
 		/*
 		 * If ret == 0 means that all IOs in front of link io are
 		 * running done. let's queue link head.

fs/io_uring.c static void io_queue_link_head(struct io_kiocb *req, struct io_kiocb *shadow) · line 3026

Quoted 10 lines

fs/io_uring.c +1 -1

 		 */
 		need_submit = true;
+		spin_lock_irq(&ctx->completion_lock);
 	}
 
 	/* Insert shadow req to defer_list, blocking next IOs */
-	spin_lock_irq(&ctx->completion_lock);
 	trace_io_uring_defer(ctx, shadow, true);
 	list_add_tail(&shadow->list, &ctx->defer_list);
 	spin_unlock_irq(&ctx->completion_lock);

This is indeed a potential lock issue, thanks, I am prefer this solution, clearer than first one. But It may be a bit difficult for other people who read the code, use 'io_req_defer_may_lock'?

who about this?

fs/io_uring.c +1 -1

index 5ad652f..6fdaeb1 100644
@@ -2469,7 +2469,7 @@ static int io_async_cancel(struct io_kiocb *req, const struct io_uring_sqe *sqe,
        return 0;
 }
 
-static int io_req_defer(struct io_kiocb *req)
+static int __io_req_defer(struct io_kiocb *req)
 {
        const struct io_uring_sqe *sqe = req->submit.sqe;
        struct io_uring_sqe *sqe_copy;

fs/io_uring.c static int io_req_defer(struct io_kiocb *req) · line 2504

Quoted 10 lines

fs/io_uring.c +7 -0

@@ -2495,8 +2495,21 @@ static int io_req_defer(struct io_kiocb *req)
 
        trace_io_uring_defer(ctx, req, false);
        list_add_tail(&req->list, &ctx->defer_list);
+
+       return -EIOCBQUEUED;
+}
+
+static int io_req_defer(struct io_kiocb *req)
+{
+       int ret = __io_req_defer(req);

There have an problem, need fix.

static int io_req_defer(struct io_kiocb *req)

{
	int ret = __io_req_defer(req);
	if (ret == -EIOCBQUEUED)
		spin_unlock_irq(&ctx->completion_lock);
	return ret;
}

Quoted 130 lines · Jackie Liu

Quoted 117 lines

2019年11月21日 09:32，Jackie Liu <[email protected]> 写道：

Quoted 81 lines · 2019年11月21日 07:58，Jens Axboe <[email protected]> 写道：

Quoted 7 lines · Jens Axboe

When we go and queue requests with drain, we check if we need to defer based on sequence. This is done safely under the lock, but then we drop the lock before actually inserting the shadow. If the original request is found on the deferred list by another completion in the mean time, it could have been started AND completed by the time we insert the shadow, which will stall the queue.

Quoted 8 lines

After re-grabbing the completion lock, check if the original request is still in the deferred list. If it isn't, then we know that someone else already found and issued it. If that happened, then our job is done, we can simply free the shadow.

Cc Jackie Liu <[email protected]>

Fixes 4fe2c963154c ("io_uring: add support for link with drain")

Signed-off-by Jens Axboe <[email protected]>

BTW, the other solution here is to not release the completion_lock if we're going to return -EIOCBQUEUED, and let the caller do what it needs before releasing it. That'd look something like this, with some sparse annotations to keep things happy.

I think the original I posted here is easier to follow, and the deferral list is going to be tiny in general so it won't really add any extra overhead.

Let me know what you think and prefer.

fs/io_uring.c +11 -1

index 6175e2e195c0..0d1f33bcedc0 100644
@@ -2552,6 +2552,11 @@ static int io_async_cancel(struct io_kiocb *req, const struct io_uring_sqe *sqe,
 	return 0;
 }
 
+/*
+ * Returns with ctx->completion_lock held if -EIOCBQUEUED is returned, so
+ * the caller can make decisions based on the deferral without worrying about
+ * the request being found and issued in the mean time.
+ */
 static int io_req_defer(struct io_kiocb *req)
 {
 	const struct io_uring_sqe *sqe = req->submit.sqe;

@@ -2579,7 +2584,7 @@ static int io_req_defer(struct io_kiocb *req)
 
 	trace_io_uring_defer(ctx, req, false);
 	list_add_tail(&req->list, &ctx->defer_list);
-	spin_unlock_irq(&ctx->completion_lock);
+	__release(&ctx->completion_lock);
 	return -EIOCBQUEUED;
 }
 

@@ -2954,6 +2959,7 @@ static void __io_queue_sqe(struct io_kiocb *req)
 
 static void io_queue_sqe(struct io_kiocb *req)
 {
+	struct io_ring_ctx *ctx = req->ctx;
 	int ret;
 
 	ret = io_req_defer(req);

@@ -2963,6 +2969,9 @@ static void io_queue_sqe(struct io_kiocb *req)
 			if (req->flags & REQ_F_LINK)
 				req->flags |= REQ_F_FAIL_LINK;
 			io_double_put_req(req);
+		} else {
+			__acquire(&ctx->completion_lock);
+			spin_unlock_irq(&ctx->completion_lock);
 		}
 	} else
 		__io_queue_sqe(req);

@@ -3001,16 +3010,17 @@ static void io_queue_link_head(struct io_kiocb *req, struct io_kiocb *shadow)
 				__io_free_req(shadow);
 			return;
 		}
+		__acquire(&ctx->completion_lock);
 	} else {
 		/*
 		 * If ret == 0 means that all IOs in front of link io are
 		 * running done. let's queue link head.

fs/io_uring.c static void io_queue_link_head(struct io_kiocb *req, struct io_kiocb *shadow) · line 3026

Quoted 10 lines

fs/io_uring.c +1 -1

 		 */
 		need_submit = true;
+		spin_lock_irq(&ctx->completion_lock);
 	}
 
 	/* Insert shadow req to defer_list, blocking next IOs */
-	spin_lock_irq(&ctx->completion_lock);
 	trace_io_uring_defer(ctx, shadow, true);
 	list_add_tail(&shadow->list, &ctx->defer_list);
 	spin_unlock_irq(&ctx->completion_lock);

This is indeed a potential lock issue, thanks, I am prefer this solution, clearer than first one. But It may be a bit difficult for other people who read the code, use 'io_req_defer_may_lock'?

who about this?

fs/io_uring.c +1 -1

index 5ad652f..6fdaeb1 100644
@@ -2469,7 +2469,7 @@ static int io_async_cancel(struct io_kiocb *req, const struct io_uring_sqe *sqe,
         return 0;
 }
 
-static int io_req_defer(struct io_kiocb *req)
+static int __io_req_defer(struct io_kiocb *req)
 {
         const struct io_uring_sqe *sqe = req->submit.sqe;
         struct io_uring_sqe *sqe_copy;

fs/io_uring.c static int io_req_defer(struct io_kiocb *req) · line 2504

Quoted 10 lines

fs/io_uring.c +7 -0

@@ -2495,8 +2495,21 @@ static int io_req_defer(struct io_kiocb *req)
 
         trace_io_uring_defer(ctx, req, false);
         list_add_tail(&req->list, &ctx->defer_list);
+
+       return -EIOCBQUEUED;
+}
+
+static int io_req_defer(struct io_kiocb *req)
+{
+       int ret = __io_req_defer(req);

There have an problem, need fix.

Quoted 7 lines

static int io_req_defer(struct io_kiocb *req)

{
	int ret = __io_req_defer(req);
	if (ret == -EIOCBQUEUED)
		spin_unlock_irq(&ctx->completion_lock);
	return ret;
}

Mid-air collision, indeed.

But as I wrote in the previous email, I don't think this one improves on the situation... And fwiw, I did test both of mine, both are verified to fix the issue.

2019年11月21日 09:49，Jens Axboe <[email protected]> 写道：

Quoted 139 lines · Jens Axboe

Quoted 130 lines · Jackie Liu

Quoted 117 lines

2019年11月21日 09:32，Jackie Liu <[email protected]> 写道：

Quoted 81 lines · 2019年11月21日 07:58，Jens Axboe <[email protected]> 写道：

Quoted 7 lines · Jens Axboe

When we go and queue requests with drain, we check if we need to defer based on sequence. This is done safely under the lock, but then we drop the lock before actually inserting the shadow. If the original request is found on the deferred list by another completion in the mean time, it could have been started AND completed by the time we insert the shadow, which will stall the queue.

Quoted 8 lines

After re-grabbing the completion lock, check if the original request is still in the deferred list. If it isn't, then we know that someone else already found and issued it. If that happened, then our job is done, we can simply free the shadow.

Cc Jackie Liu <[email protected]>

Fixes 4fe2c963154c ("io_uring: add support for link with drain")

Signed-off-by Jens Axboe <[email protected]>

BTW, the other solution here is to not release the completion_lock if we're going to return -EIOCBQUEUED, and let the caller do what it needs before releasing it. That'd look something like this, with some sparse annotations to keep things happy.

I think the original I posted here is easier to follow, and the deferral list is going to be tiny in general so it won't really add any extra overhead.

Let me know what you think and prefer.

fs/io_uring.c +11 -1

index 6175e2e195c0..0d1f33bcedc0 100644
@@ -2552,6 +2552,11 @@ static int io_async_cancel(struct io_kiocb *req, const struct io_uring_sqe *sqe,
 	return 0;
 }
 
+/*
+ * Returns with ctx->completion_lock held if -EIOCBQUEUED is returned, so
+ * the caller can make decisions based on the deferral without worrying about
+ * the request being found and issued in the mean time.
+ */
 static int io_req_defer(struct io_kiocb *req)
 {
 	const struct io_uring_sqe *sqe = req->submit.sqe;

@@ -2579,7 +2584,7 @@ static int io_req_defer(struct io_kiocb *req)
 
 	trace_io_uring_defer(ctx, req, false);
 	list_add_tail(&req->list, &ctx->defer_list);
-	spin_unlock_irq(&ctx->completion_lock);
+	__release(&ctx->completion_lock);
 	return -EIOCBQUEUED;
 }
 

@@ -2954,6 +2959,7 @@ static void __io_queue_sqe(struct io_kiocb *req)
 
 static void io_queue_sqe(struct io_kiocb *req)
 {
+	struct io_ring_ctx *ctx = req->ctx;
 	int ret;
 
 	ret = io_req_defer(req);

@@ -2963,6 +2969,9 @@ static void io_queue_sqe(struct io_kiocb *req)
 			if (req->flags & REQ_F_LINK)
 				req->flags |= REQ_F_FAIL_LINK;
 			io_double_put_req(req);
+		} else {
+			__acquire(&ctx->completion_lock);
+			spin_unlock_irq(&ctx->completion_lock);
 		}
 	} else
 		__io_queue_sqe(req);

@@ -3001,16 +3010,17 @@ static void io_queue_link_head(struct io_kiocb *req, struct io_kiocb *shadow)
 				__io_free_req(shadow);
 			return;
 		}
+		__acquire(&ctx->completion_lock);
 	} else {
 		/*
 		 * If ret == 0 means that all IOs in front of link io are
 		 * running done. let's queue link head.

fs/io_uring.c static void io_queue_link_head(struct io_kiocb *req, struct io_kiocb *shadow) · line 3026

Quoted 10 lines

fs/io_uring.c +1 -1

 		 */
 		need_submit = true;
+		spin_lock_irq(&ctx->completion_lock);
 	}
 
 	/* Insert shadow req to defer_list, blocking next IOs */
-	spin_lock_irq(&ctx->completion_lock);
 	trace_io_uring_defer(ctx, shadow, true);
 	list_add_tail(&shadow->list, &ctx->defer_list);
 	spin_unlock_irq(&ctx->completion_lock);

This is indeed a potential lock issue, thanks, I am prefer this solution, clearer than first one. But It may be a bit difficult for other people who read the code, use 'io_req_defer_may_lock'?

who about this?

fs/io_uring.c +1 -1

index 5ad652f..6fdaeb1 100644
@@ -2469,7 +2469,7 @@ static int io_async_cancel(struct io_kiocb *req, const struct io_uring_sqe *sqe,
        return 0;
 }
 
-static int io_req_defer(struct io_kiocb *req)
+static int __io_req_defer(struct io_kiocb *req)
 {
        const struct io_uring_sqe *sqe = req->submit.sqe;
        struct io_uring_sqe *sqe_copy;

fs/io_uring.c static int io_req_defer(struct io_kiocb *req) · line 2504

Quoted 10 lines

fs/io_uring.c +7 -0

@@ -2495,8 +2495,21 @@ static int io_req_defer(struct io_kiocb *req)
 
        trace_io_uring_defer(ctx, req, false);
        list_add_tail(&req->list, &ctx->defer_list);
+
+       return -EIOCBQUEUED;
+}
+
+static int io_req_defer(struct io_kiocb *req)
+{
+       int ret = __io_req_defer(req);

There have an problem, need fix.

Quoted 7 lines

static int io_req_defer(struct io_kiocb *req)

{
	int ret = __io_req_defer(req);
	if (ret == -EIOCBQUEUED)
		spin_unlock_irq(&ctx->completion_lock);
	return ret;
}

Quoted 5 lines

Mid-air collision, indeed.

But as I wrote in the previous email, I don't think this one improves on the situation... And fwiw, I did test both of mine, both are verified to fix the issue.

Maybe we can compromise on something like this? Doesn't introduce any may_lock() naming, just uses the __io_req_defer() to take that blame. And uses the right sparse annotations to keep things happy with C=2 as well. Uses your trick to make io_req_defer() do the lock drop for the other caller.

Ran it through 400x rounds of testing, confirmed as well.

fs/io_uring.c +20 -9

index 6175e2e195c0..299a218e9552 100644
@@ -2552,7 +2552,12 @@ static int io_async_cancel(struct io_kiocb *req, const struct io_uring_sqe *sqe,
 	return 0;
 }
 
-static int io_req_defer(struct io_kiocb *req)
+/*
+ * Returns with ctx->completion_lock held if -EIOCBQUEUED is returned, so
+ * the caller can make decisions based on the deferral without worrying about
+ * the request being found and issued in the mean time.
+ */
+static int __io_req_defer(struct io_kiocb *req)
 {
 	const struct io_uring_sqe *sqe = req->submit.sqe;
 	struct io_uring_sqe *sqe_copy;

@@ -2579,10 +2584,23 @@ static int io_req_defer(struct io_kiocb *req)
 
 	trace_io_uring_defer(ctx, req, false);
 	list_add_tail(&req->list, &ctx->defer_list);
-	spin_unlock_irq(&ctx->completion_lock);
+	__release(&ctx->completion_lock);
 	return -EIOCBQUEUED;
 }
 
+static int io_req_defer(struct io_kiocb *req)
+{
+	struct io_ring_ctx *ctx = req->ctx;
+	int ret;
+
+	ret = __io_req_defer(req);
+	if (ret == -EIOCBQUEUED) {
+		__acquire(&ctx->completion_lock);
+		spin_unlock_irq(&ctx->completion_lock);
+	}
+	return ret;
+}
+
 static int __io_submit_sqe(struct io_kiocb *req, struct io_kiocb **nxt,
 			   bool force_nonblock)
 {

@@ -2957,15 +2975,14 @@ static void io_queue_sqe(struct io_kiocb *req)
 	int ret;
 
 	ret = io_req_defer(req);
-	if (ret) {
-		if (ret != -EIOCBQUEUED) {
-			io_cqring_add_event(req, ret);
-			if (req->flags & REQ_F_LINK)
-				req->flags |= REQ_F_FAIL_LINK;
-			io_double_put_req(req);
-		}

fs/io_uring.c +1 -1

 static void io_queue_link_head(struct io_kiocb *req, struct io_kiocb *shadow)

@@ -2989,7 +3006,7 @@ static void io_queue_link_head(struct io_kiocb *req, struct io_kiocb *shadow)
 	 * list.
 	 */
 	req->flags |= REQ_F_IO_DRAIN;
-	ret = io_req_defer(req);
+	ret = __io_req_defer(req);
 	if (ret) {
 		if (ret != -EIOCBQUEUED) {

err:

fs/io_uring.c +1 -0

@@ -3001,16 +3018,17 @@ static void io_queue_link_head(struct io_kiocb *req, struct io_kiocb *shadow)
 				__io_free_req(shadow);
 			return;
 		}
+		__acquire(&ctx->completion_lock);
 	} else {
 		/*
 		 * If ret == 0 means that all IOs in front of link io are
 		 * running done. let's queue link head.

Quoted 16 lines

fs/io_uring.c +3 -8

@@ -2957,15 +2975,14 @@ static void io_queue_sqe(struct io_kiocb *req)
 	int ret;
 
 	ret = io_req_defer(req);
-	if (ret) {
-		if (ret != -EIOCBQUEUED) {
-			io_cqring_add_event(req, ret);
-			if (req->flags & REQ_F_LINK)
-				req->flags |= REQ_F_FAIL_LINK;
-			io_double_put_req(req);
-		}
-	} else
+	if (!ret) {
 		__io_queue_sqe(req);
+	} else if (ret != -EIOCBQUEUED) {
+		io_cqring_add_event(req, ret);

fs/io_uring.c static void io_queue_sqe(struct io_kiocb *req) · line 2986

Quoted 5 lines

fs/io_uring.c +4 -0

+		if (req->flags & REQ_F_LINK)
+			req->flags |= REQ_F_FAIL_LINK;
+		io_double_put_req(req);
+	}
 }

Hmm.. Why we need rewrite there? clear code? Seems to be unrelated to this issue.

Quoted 49 lines

2019年11月21日 07:14，Jens Axboe <[email protected]> 写道：

Quoted 16 lines · Jackie Liu

Quoted 16 lines

fs/io_uring.c +3 -8

@@ -2957,15 +2975,14 @@ static void io_queue_sqe(struct io_kiocb *req)
 	int ret;
 
 	ret = io_req_defer(req);
-	if (ret) {
-		if (ret != -EIOCBQUEUED) {
-			io_cqring_add_event(req, ret);
-			if (req->flags & REQ_F_LINK)
-				req->flags |= REQ_F_FAIL_LINK;
-			io_double_put_req(req);
-		}
-	} else
+	if (!ret) {
 		__io_queue_sqe(req);
+	} else if (ret != -EIOCBQUEUED) {
+		io_cqring_add_event(req, ret);

Quoted 8 lines

fs/io_uring.c static void io_queue_sqe(struct io_kiocb *req) · line 2986

Quoted 5 lines

fs/io_uring.c +4 -0

+		if (req->flags & REQ_F_LINK)
+			req->flags |= REQ_F_FAIL_LINK;
+		io_double_put_req(req);
+	}
 }

Hmm.. Why we need rewrite there? clear code? Seems to be unrelated to this issue.

We don't need to, and the previous patch touched it, but it's much easier to read after this change. Before it was:

if (ret) {
	if (ret != -EIOCBQUEUED) {
		...
	}
} else {
	...
}

which is now just

if (!ret) {
	...
} else if (ret != -EIOCBQUEUED) {
	...
}

Quoted 3 lines

not related to the change really, but kind of silly to make a separate patch for imho.

Understand, thanks for explaining and fixing this problem, And now, please add:

Reviewed-by Jackie Liu <[email protected]>

Quoted 4 lines · 2019年11月21日 16:54，Pavel Begunkov <[email protected]> 写道：

If there is a DRAIN in the middle of a link, it uses shadow req. Defer the next request/link instead. This:

Quoted 4 lines

Pros:
1. removes semi-duplicated code
2. doesn't allocate memory for shadows
3. works better if only the head marked for drain

I thought about this before, just only drain the head, but if the latter IO depends on the link-list, then latter IO will run in front of the link-list. If we think it is acceptable, then I think it is ok for me.

--
BR, Jackie Liu

Quoted 26 lines · Jackie Liu

Quoted 10 lines

Quoted 8 lines · 2019年11月21日 16:54，Pavel Begunkov <[email protected]> 写道：

If there is a DRAIN in the middle of a link, it uses shadow req. Defer the next request/link instead. This:

Pros:
1. removes semi-duplicated code
2. doesn't allocate memory for shadows
3. works better if only the head marked for drain

Quoted 5 lines

I thought about this before, just only drain the head, but if the latter IO depends
on the link-list, then latter IO will run in front of the link-list. If we think it
is acceptable, then I think it is ok for me.

If I got your point right, latter requests won't run ahead of the link-list. There shouldn't be change of behaviour.

The purpose of shadow requests is to mark some request right ahead of the link for draining. This patch uses not a specially added shadow request, but the following regular one. And, as drained IO shouldn't be issued until every request behind completed, this should give the same effect.

Quoted 1 line

Am I missed something?

Thanks for explaining. This is also correct, if I understand correctly, It seems that other IOs will wait for all the links are done. this is a little different, is it?

Signature · 2 lines

-- 
Jackie Liu

Quoted 37 lines · Pavel Begunkov

Quoted 27 lines · Pavel Begunkov

Quoted 26 lines · Jackie Liu

Quoted 10 lines

Quoted 8 lines · 2019年11月21日 16:54，Pavel Begunkov <[email protected]> 写道：

If there is a DRAIN in the middle of a link, it uses shadow req. Defer the next request/link instead. This:

Pros:
1. removes semi-duplicated code
2. doesn't allocate memory for shadows
3. works better if only the head marked for drain

Quoted 5 lines

I thought about this before, just only drain the head, but if the latter IO depends
on the link-list, then latter IO will run in front of the link-list. If we think it
is acceptable, then I think it is ok for me.

If I got your point right, latter requests won't run ahead of the link-list. There shouldn't be change of behaviour.

The purpose of shadow requests is to mark some request right ahead of the link for draining. This patch uses not a specially added shadow request, but the following regular one. And, as drained IO shouldn't be issued until every request behind completed, this should give the same effect.

Quoted 5 lines

Quoted 1 line

Am I missed something?

Thanks for explaining. This is also correct, if I understand correctly, It seems that other IOs will wait for all the links are done. this is a little different, is it?

Yes, you're right, it also was briefly stated in the patch description (see Cons). I hope, links + drain in the middle is an uncommon case. But it can be added back, but may become a little bit uglier.

Quoted 1 line

What do you think, should we care about this case?

Yes, this is a very tiny scene. When I first time wrote this part of the code, my suggestion was to ban it directly.

For this patch, I am fine, Jens, what do you think.

The mailing list always rejects my mail, is my smtp server IP banned?

Quoted 3 lines

Was that a reviewed-by from you? It'd be nice to get these more formally so I can add the attributes. I'll drop the other patch.

I want to do more tests. There is no test machine at this time, at least until tomorrow morning.

Quoted 5 lines

Quoted 2 lines

The mailing list always rejects my mail, is my smtp server IP banned?

Probably because you have text/html in your email, the list is pretty picky when it comes to anything that isn't just text/plain.

I don't know, I use thunderbird to write emails, and it has been set to plain text, perhaps because of the signature, I have to check my mail client.

Signature · 1 line

Jackie Liu

Quoted 7 lines

Quoted 3 lines

Was that a reviewed-by from you? It'd be nice to get these more formally so I can add the attributes. I'll drop the other patch.

I want to do more tests. There is no test machine at this time, at least until tomorrow morning.

BTW, aside from the locking problem, that it fixes another one. If allocation for a shadow req is failed, io_submit_sqes() just continues without it ignoring draining.

Quoted 7 lines · Pavel Begunkov

Quoted 7 lines

Quoted 3 lines

Was that a reviewed-by from you? It'd be nice to get these more formally so I can add the attributes. I'll drop the other patch.

I want to do more tests. There is no test machine at this time, at least until tomorrow morning.

Quoted 3 lines

BTW, aside from the locking problem, that it fixes another one. If allocation for a shadow req is failed, io_submit_sqes() just continues without it ignoring draining.

Indeed. BTW, your commit message is way too weak for this patch. It doesn't explain why we're making this change at all. I'm going to fix it up.

Quoted 12 lines · Jens Axboe

Quoted 10 lines · Pavel Begunkov

Quoted 7 lines

Quoted 3 lines

Was that a reviewed-by from you? It'd be nice to get these more formally so I can add the attributes. I'll drop the other patch.

I want to do more tests. There is no test machine at this time, at least until tomorrow morning.

BTW, aside from the locking problem, that it fixes another one. If allocation for a shadow req is failed, io_submit_sqes() just continues without it ignoring draining.

Quoted 4 lines

Indeed. BTW, your commit message is way too weak for this patch. It doesn't explain why we're making this change at all. I'm going to fix it up.

Yeah, I were going to do that today. This one is of quick-before-leaving kind, I didn't even looked up into the problem discussion properly.

It needs 2 problems statements + reported-by

Quoted 48 lines · Jens Axboe

Quoted 46 lines · Jackie Liu

Quoted 33 lines · Pavel Begunkov

Quoted 27 lines · Pavel Begunkov

Quoted 26 lines · Jackie Liu

Quoted 10 lines

Quoted 8 lines · 2019年11月21日 16:54，Pavel Begunkov <[email protected]> 写道：

If there is a DRAIN in the middle of a link, it uses shadow req. Defer the next request/link instead. This:

Pros:
1. removes semi-duplicated code
2. doesn't allocate memory for shadows
3. works better if only the head marked for drain

Quoted 5 lines

I thought about this before, just only drain the head, but if the latter IO depends
on the link-list, then latter IO will run in front of the link-list. If we think it
is acceptable, then I think it is ok for me.

If I got your point right, latter requests won't run ahead of the link-list. There shouldn't be change of behaviour.

The purpose of shadow requests is to mark some request right ahead of the link for draining. This patch uses not a specially added shadow request, but the following regular one. And, as drained IO shouldn't be issued until every request behind completed, this should give the same effect.

Quoted 5 lines

Quoted 1 line

Am I missed something?

Thanks for explaining. This is also correct, if I understand correctly, It seems that other IOs will wait for all the links are done. this is a little different, is it?

Quoted 5 lines

Yes, you're right, it also was briefly stated in the patch description (see Cons). I hope, links + drain in the middle is an uncommon case. But it can be added back, but may become a little bit uglier.

What do you think, should we care about this case?

Yes, this is a very tiny scene. When I first time wrote this part of the code, my suggestion was to ban it directly.

For this patch, I am fine, Jens, what do you think.

Quoted 5 lines

I am fine with it as well, it'd be nice to get rid of needing that extra request.

Was that a reviewed-by from you? It'd be nice to get these more formally so I can add the attributes. I'll drop the other patch.

I want to do more tests. There is no test machine at this time, at least until tomorrow morning.

Quoted 4 lines

Quoted 1 line

The mailing list always rejects my mail, is my smtp server IP banned?

Probably because you have text/html in your email, the list is pretty picky when it comes to anything that isn't just text/plain.

I don't know, I use thunderbird to write emails, and it has been set to plain text, perhaps because of the signature, I have to check my mail client.

Quoted 81 lines · 2019年11月21日 07:58，Jens Axboe <[email protected]> 写道：

Quoted 7 lines · Jens Axboe

When we go and queue requests with drain, we check if we need to defer based on sequence. This is done safely under the lock, but then we drop the lock before actually inserting the shadow. If the original request is found on the deferred list by another completion in the mean time, it could have been started AND completed by the time we insert the shadow, which will stall the queue.

Quoted 8 lines

After re-grabbing the completion lock, check if the original request is still in the deferred list. If it isn't, then we know that someone else already found and issued it. If that happened, then our job is done, we can simply free the shadow.

Cc Jackie Liu <[email protected]>

Fixes 4fe2c963154c ("io_uring: add support for link with drain")

Signed-off-by Jens Axboe <[email protected]>

BTW, the other solution here is to not release the completion_lock if we're going to return -EIOCBQUEUED, and let the caller do what it needs before releasing it. That'd look something like this, with some sparse annotations to keep things happy.

I think the original I posted here is easier to follow, and the deferral list is going to be tiny in general so it won't really add any extra overhead.

Let me know what you think and prefer.

fs/io_uring.c +11 -1

index 6175e2e195c0..0d1f33bcedc0 100644
@@ -2552,6 +2552,11 @@ static int io_async_cancel(struct io_kiocb *req, const struct io_uring_sqe *sqe,
 	return 0;
 }
 
+/*
+ * Returns with ctx->completion_lock held if -EIOCBQUEUED is returned, so
+ * the caller can make decisions based on the deferral without worrying about
+ * the request being found and issued in the mean time.
+ */
 static int io_req_defer(struct io_kiocb *req)
 {
 	const struct io_uring_sqe *sqe = req->submit.sqe;

@@ -2579,7 +2584,7 @@ static int io_req_defer(struct io_kiocb *req)
 
 	trace_io_uring_defer(ctx, req, false);
 	list_add_tail(&req->list, &ctx->defer_list);
-	spin_unlock_irq(&ctx->completion_lock);
+	__release(&ctx->completion_lock);
 	return -EIOCBQUEUED;
 }
 

@@ -2954,6 +2959,7 @@ static void __io_queue_sqe(struct io_kiocb *req)
 
 static void io_queue_sqe(struct io_kiocb *req)
 {
+	struct io_ring_ctx *ctx = req->ctx;
 	int ret;
 
 	ret = io_req_defer(req);

@@ -2963,6 +2969,9 @@ static void io_queue_sqe(struct io_kiocb *req)
 			if (req->flags & REQ_F_LINK)
 				req->flags |= REQ_F_FAIL_LINK;
 			io_double_put_req(req);
+		} else {
+			__acquire(&ctx->completion_lock);
+			spin_unlock_irq(&ctx->completion_lock);
 		}
 	} else
 		__io_queue_sqe(req);

@@ -3001,16 +3010,17 @@ static void io_queue_link_head(struct io_kiocb *req, struct io_kiocb *shadow)
 				__io_free_req(shadow);
 			return;
 		}
+		__acquire(&ctx->completion_lock);
 	} else {
 		/*
 		 * If ret == 0 means that all IOs in front of link io are
 		 * running done. let's queue link head.

fs/io_uring.c static void io_queue_link_head(struct io_kiocb *req, struct io_kiocb *shadow) · line 3026

Quoted 10 lines

fs/io_uring.c +1 -1

 		 */
 		need_submit = true;
+		spin_lock_irq(&ctx->completion_lock);
 	}
 
 	/* Insert shadow req to defer_list, blocking next IOs */
-	spin_lock_irq(&ctx->completion_lock);
 	trace_io_uring_defer(ctx, shadow, true);
 	list_add_tail(&shadow->list, &ctx->defer_list);
 	spin_unlock_irq(&ctx->completion_lock);

This is indeed a potential lock issue, thanks, I am prefer this solution, clearer than first one. But It may be a bit difficult for other people who read the code, use 'io_req_defer_may_lock'?

who about this?