Re: [PATCH] io_uring: use task_work for links if possible

[Pavel Begunkov <asml.silence@gmail.com>]

On 27/06/2020 04:45, Jens Axboe wrote:
> On 6/26/20 3:20 PM, Pavel Begunkov wrote:
>>>>> +		tsk = io_wq_get_task(req->ctx->io_wq);
>>>>> +		task_work_add(tsk, &req->task_work, true);
>>>>> +	}
>>>>> +	wake_up_process(tsk);
>>>>> +}
>>>>> +
>>>>>  static void io_free_req(struct io_kiocb *req)
>>>>>  {
>>>>>  	struct io_kiocb *nxt = NULL;
>>>>> @@ -1671,8 +1758,12 @@ static void io_free_req(struct io_kiocb *req)
>>>>>  	io_req_find_next(req, &nxt);
>>>>>  	__io_free_req(req);
>>>>>  
>>>>> -	if (nxt)
>>>>> -		io_queue_async_work(nxt);
>>>>> +	if (nxt) {
>>>>> +		if (nxt->flags & REQ_F_WORK_INITIALIZED)
>>>>> +			io_queue_async_work(nxt);
>>>>
>>>> Don't think it will work. E.g. io_close_prep() may have set
>>>> REQ_F_WORK_INITIALIZED but without io_req_work_grab_env().
>>>
>>> This really doesn't change the existing path, it just makes sure we
>>> don't do io_req_task_queue() on something that has already modified
>>> ->work (and hence, ->task_work). This might miss cases where we have
>>> only cleared it and done nothing else, but that just means we'll have
>>> cases that we could potentially improve the effiency of down the line.
>>
>> Before the patch it was always initialising linked reqs, and that would
>> work ok, if not this lazy grab_env().
>>
>> E.g. req1 -> close_req
>>
>> It calls, io_req_defer_prep(__close_req__, sqe, __false__)
>> which doesn't do grab_env() because of for_async=false,
>> but calls io_close_prep() which sets REQ_F_WORK_INITIALIZED.
>>
>> Then, after completion of req1 it will follow added lines
>>
>> if (nxt)
>> 	if (nxt->flags & REQ_F_WORK_INITIALIZED)
>> 		io_queue_async_work(nxt);
>>
>> Ending up in
>>
>> io_queue_async_work()
>> 	-> grab_env()
>>
>> And that's who knows from which context.
>> E.g. req1 was an rw completed in an irq.
> 
> Hmm yes, good point, that is a problem. I don't have a good immediate
> solution for this. Do you have any suggestions on how best to handle
> this?

I certainly don't want another REQ_F_GRABBED_ENV flag :)

From the start I was planning to move all grab_env() calls to
io_queue_async_work() just before we're doing punting. like

io_queue_async_work(req) {
	// simplified
	for_each_in_link(req)
		grab_env();
	...
}

If done right, this can solve a lot of problems and simplify
lifetime management. There are much more problems, I'll send
a patchset with quick fixes, and then we can do it right
without hurry.

> 
>> Not sure it's related, but fallocate shows the log below, and some
>> other tests hang the kernel as well.
> 
> Yeah, that's indeed that very thing.

Turns out it's not.

> 
>>> True, that could be false instead.
>>>
>>> Since these are just minor things, we can do a fix on top. I don't want
>>> to reshuffle this unless I have to.
>>
>> Agree, I have a pile on top myself.
> 
> Fire away :-)

I prefer to have a working branch first.


-- 
Pavel Begunkov