Re: [PATCH] io_uring: fix invalid handler for double apoll

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

On 25/10/2020 19:44, Jens Axboe wrote:
> On 10/25/20 1:32 PM, Pavel Begunkov wrote:
>> On 25/10/2020 19:18, Jens Axboe wrote:
>>> On 10/25/20 1:01 PM, Pavel Begunkov wrote:
>>>> On 25/10/2020 18:42, Jens Axboe wrote:
>>>>> On 10/25/20 10:24 AM, Pavel Begunkov wrote:
>>>>>> On 25/10/2020 15:53, Jens Axboe wrote:
>>>>>>> On 10/25/20 8:26 AM, Pavel Begunkov wrote:
>>>>>>>> io_poll_double_wake() is called for both: poll requests and as apoll
>>>>>>>> (internal poll to make rw and other requests), hence when it calls
>>>>>>>> __io_async_wake() it should use a right callback depending on the
>>>>>>>> current poll type.
>>>>>>>
>>>>>>> Can we do something like this instead? Untested...
>>>>>>
>>>>>> It should work, but looks less comprehensible. Though, it'll need
>>>>>
>>>>> Not sure I agree, with a comment it'd be nicer im ho:
>>>>
>>>> I don't really care enough to start a bikeshedding, let's get yours
>>>> tested and merged.
>>>
>>> Not really bikeshedding I think, we're not debating names of
>>> functions :-)
>>
>> It's just not so important, and it even may get removed in a month,
>> who knows.
> 
> Well it might not or it might take longer, still nice to have the
> simplest fix...

Ok, then TLDR: my reasoning is that with poll->wait.func(), a person
who reads it needs to
a) go look up what's poll (assigned depending on the opcode),
b) then find out which wait.func callback it set. And it's not
as easy to associate __io_arm_poll_handler() call sites with cases
if you haven't seen this code before.
c) then go through io_{async,poll}_wake to finally find out that they
do almost the same thing, that's calling __io_async_wake().

I'm familiar with the code structure, but IMHO it's harder to
understand, if I'd be looking for the first time.

> 
>>> My approach would need conditional clearing of ->private as well,
>>> as far as I can tell. I'll give it a spin.
>>
>> Maybe
>>
>> - poll->wait.func(wait, mode, sync, key);
>> + poll->wait.func(&poll->wait, mode, sync, key);
> 
> Ah yeah, that looks better.
> 

-- 
Pavel Begunkov