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

[Jens Axboe <axboe@kernel.dk>]

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?

> 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.

>> 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 :-)

-- 
Jens Axboe