On 16/11/2019 00:26, Jens Axboe wrote:
> On 11/15/19 2:22 PM, Pavel Begunkov wrote:
>> On 15/11/2019 22:34, Jens Axboe wrote:
>>> How about something like this? Should work (and be valid) to have any
>>> sequence of timeout links, as long as there's something in front of it.
>>> Commit message has more details.
>>
>> If you don't mind, I'll give a try rewriting this. A bit tight
>> on time, so hopefully by this Sunday.
>>
>> In any case, there are enough of edge cases, I need to spend some
>> time to review and check it.
> 
> Of course, appreciate more eyes on this for sure. We'll see what happens
> with 5.4 release, I suspect it won't happen until 11/24. In any case,
> this is not really staged yet, just sitting in for-5.5/io_uring-post
> as part of a series that'll likely go in after the initial merge.
> 
Tangled up in cancellation and io-wq, so no reworking here until I
understand it well enough. I've sent a separate series for what spotted

I've wanted to return back a version with queuing linked timeout
before issuing request, but make the timeout callback mark the
master request as cancelled. e.g. 

io_link_timeout_fn(req) {
	req->work.flags |= IO_WQ_WORK_CANCEL;
	try_to_cancel();
}
issue_req(req) {
	next = get_next(req);
	queue_linked_timeout(next);
	__issue_req(req);
}



There is another point to discuss until de-staged it. Did you consider
reverse syntax? e.g. LINKED_TIMEOUT -> REQ instead.

I think it's a bit more consistent, as any request affects only those
on right of it (from the user perspective).

Also, I'm tempted to implement the scheme above, and after remove all
linked timeout handling from submission path and move it into
io_issue_sqe() (last __io_submit_sqe) as yet another case in the switch.


>> REQ1 -> LINKED_TIMEOUT -> REQ2 -> REQ3
>> Is this a valid case? Just to check that I got this "can't have both" right.
>> If no, why so? I think there are a lot of use cases for this.
> 
> Yes, it's valid. With the recently posted stuff, the only invalid case
> is having a linked timeout as the first entry since that's nonsensical.
> It has to be linked from a previous request. We no longer need to
> restrict where the linked timeout appears otherwise.
> 

-- 
Pavel Begunkov