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
