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
