Re: [PATCH 7/8] io_uring: fix sequencing issues with linked timeouts

[Jens Axboe <axboe@kernel.dk>]

On 11/20/19 10:19 AM, Jens Axboe wrote:
> On 11/20/19 5:42 AM, Pavel Begunkov wrote:
>> On 11/20/2019 1:13 AM, Jens Axboe wrote:
>>> On 11/19/19 1:51 PM, Pavel Begunkov wrote:
>>>> On 16/11/2019 04:53, Jens Axboe wrote:
>>>>> We have an issue with timeout links that are deeper in the submit chain,
>>>>> because we only handle it upfront, not from later submissions. Move the
>>>>> prep + issue of the timeout link to the async work prep handler, and do
>>>>> it normally for non-async queue. If we validate and prepare the timeout
>>>>> links upfront when we first see them, there's nothing stopping us from
>>>>> supporting any sort of nesting.
>>>>>
>>>>> Fixes: 2665abfd757f ("io_uring: add support for linked SQE timeouts")
>>>>> Reported-by: Pavel Begunkov <asml.silence@gmail.com>
>>>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>>>> ---
>>>>
>>>>> @@ -923,6 +942,7 @@ static void io_fail_links(struct io_kiocb *req)
>>>>>     			io_cqring_fill_event(link, -ECANCELED);
>>>>>     			__io_double_put_req(link);
>>>>>     		}
>>>>> +		kfree(sqe_to_free);
>>>>>     	}
>>>>>     
>>>>>     	io_commit_cqring(ctx);
>>>>> @@ -2668,8 +2688,12 @@ static void io_wq_submit_work(struct io_wq_work **workptr)
>>>>>     
>>>>>     	/* if a dependent link is ready, pass it back */
>>>>>     	if (!ret && nxt) {
>>>>> -		io_prep_async_work(nxt);
>>>>> +		struct io_kiocb *link;
>>>>> +
>>>>> +		io_prep_async_work(nxt, &link);
>>>>>     		*workptr = &nxt->work;
>>>> Are we safe here without synchronisation?
>>>> Probably io_link_timeout_fn() may miss the new value
>>>> (doing io-wq cancel).
>>>
>>> Miss what new value? Don't follow that part.
>>>
>>
>> As I've got the idea of postponing:
>> at the moment of io_queue_linked_timeout(), a request should be either
>> in io-wq or completed. So, @nxt->work after the assignment above should
>> be visible to asynchronously called io_wq_cancel_work().
>>
>>>>>    *workptr = &nxt->work;
>> However, there is no synchronisation for this assignment, and it could
>> be not visible from a parallel thread. Is it somehow handled in io-wq?
>>
>> The pseudo code is below (th1, th2 - parallel threads)
>> th1: *workptr = &req->work;
>> // non-atomic assignment, the new value of workptr (i.e. &req->work)
>> // isn't yet propagated to th2
>>
>> th1: io_queue_linked_timeout()
>> th2: io_linked_timeout_fn(), calls io_wq_cancel_work(), @req not found
>> th2: // memory model finally propagated *workptr = &req->work to @th2
>>
>>
>> Please, let me know if that's also not clear.
> 
> OK, so I see what you're saying, but I don't think it's missing locking.
> There is, however, a gap where we won't be able to find the request.
> What we need is a way to assign the io-wq current work before we call
> io_queue_linked_timeout(). Something ala:
> 
> 	io_prep_async_work(nxt, &link);
> 	*workptr = &nxt->work;
> +	io_wq_assign_cur();
> 	if (link)
> 		io_queue_linked_timeout(link);
> 
> where io_wq_assign_cur() ensures that worker->cur_work is set to the new
> work, so we know it's discoverable before calling
> io_queue_linked_timeout(). Probably also needs to include the
> ->get_work() call as part of that, so moving the logic around a bit in
> io_worker_handle_work().
> 
> If we do that, then by the time we arm the linked timer, we know we'll
> be able to find the new work item. The old work is done at this point
> anyway, so doing this a bit earlier is fine.

Something like this, totally untested.


diff --git a/fs/io-wq.c b/fs/io-wq.c
index b4bc377dda61..2666384aaf44 100644
--- a/fs/io-wq.c
+++ b/fs/io-wq.c
@@ -427,6 +427,9 @@ static void io_worker_handle_work(struct io_worker *worker)
 		worker->cur_work = work;
 		spin_unlock_irq(&worker->lock);
 
+		if (work->flags & IO_WQ_WORK_CB)
+			work->cb.fn(work->cb.data);
+
 		if ((work->flags & IO_WQ_WORK_NEEDS_FILES) &&
 		    current->files != work->files) {
 			task_lock(current);
diff --git a/fs/io-wq.h b/fs/io-wq.h
index 4b29f922f80c..892989f3e41e 100644
--- a/fs/io-wq.h
+++ b/fs/io-wq.h
@@ -11,6 +11,7 @@ enum {
 	IO_WQ_WORK_NEEDS_FILES	= 16,
 	IO_WQ_WORK_UNBOUND	= 32,
 	IO_WQ_WORK_INTERNAL	= 64,
+	IO_WQ_WORK_CB		= 128,
 
 	IO_WQ_HASH_SHIFT	= 24,	/* upper 8 bits are used for hash key */
 };
@@ -21,8 +22,17 @@ enum io_wq_cancel {
 	IO_WQ_CANCEL_NOTFOUND,	/* work not found */
 };
 
+struct io_wq_work;
+struct io_wq_work_cb {
+	void (*fn)(void *data);
+	void *data;
+};
+
 struct io_wq_work {
-	struct list_head list;
+	union {
+		struct list_head list;
+		struct io_wq_work_cb cb;
+	};
 	void (*func)(struct io_wq_work **);
 	unsigned flags;
 	struct files_struct *files;
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 066b59ffb54e..6f5745342eb2 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -2654,6 +2654,13 @@ static int __io_submit_sqe(struct io_kiocb *req, struct io_kiocb **nxt,
 	return 0;
 }
 
+static void io_link_work_cb(void *data)
+{
+	struct io_kiocb *link = data;
+
+	io_queue_linked_timeout(link);
+}
+
 static void io_wq_submit_work(struct io_wq_work **workptr)
 {
 	struct io_wq_work *work = *workptr;
@@ -2700,8 +2707,11 @@ static void io_wq_submit_work(struct io_wq_work **workptr)
 
 		io_prep_async_work(nxt, &link);
 		*workptr = &nxt->work;
-		if (link)
-			io_queue_linked_timeout(link);
+		if (link) {
+			nxt->work.flags |= IO_WQ_WORK_CB;
+			nxt->work.cb.fn = io_link_work_cb;
+			nxt->work.cb.data = link;
+		}
 	}
 }
 

-- 
Jens Axboe