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

[Jens Axboe <axboe@kernel.dk>]

On 10/25/20 1:54 PM, Pavel Begunkov wrote:
> 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.

I guess we'll just have to agree to disagree on that one. Yes, you'd
have to find where it's assigned to see what happens, which is a
downside. The benefit would be that even if these things change in the
future, the callback func would always be right, so you'd never have to
touch that code again. c) is the same for both.

-- 
Jens Axboe