| Message ID | 1bf1093730a68f8939bfd7e6747add7af37ad321.1603635991.git.asml.silence@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | io_uring: fix invalid handler for double apoll | expand |
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... diff --git a/fs/io_uring.c b/fs/io_uring.c index b42dfa0243bf..a0147c0e5320 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -4978,7 +4978,7 @@ static int io_poll_double_wake(struct wait_queue_entry *wait, unsigned mode, wait->private = NULL; spin_unlock(&poll->head->lock); if (!done) - __io_async_wake(req, poll, mask, io_poll_task_func); + poll->wait.func(wait, mode, sync, key); } refcount_dec(&req->refs); return 1; -- Jens Axboe
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 some refactoring in the future either way. > > diff --git a/fs/io_uring.c b/fs/io_uring.c > index b42dfa0243bf..a0147c0e5320 100644 > --- a/fs/io_uring.c > +++ b/fs/io_uring.c > @@ -4978,7 +4978,7 @@ static int io_poll_double_wake(struct wait_queue_entry *wait, unsigned mode, > wait->private = NULL; > spin_unlock(&poll->head->lock); > if (!done) > - __io_async_wake(req, poll, mask, io_poll_task_func); > + poll->wait.func(wait, mode, sync, key); > } > refcount_dec(&req->refs); > return 1; >
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 imho: /* call appropriate handler for this request type */ poll->wait.func(wait, mode, sync, key); instead of having to manually dig at the opcode to figure out which one to use. -- Jens Axboe
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. > > /* call appropriate handler for this request type */ > poll->wait.func(wait, mode, sync, key); > > instead of having to manually dig at the opcode to figure out which one > to use. > -- Pavel Begunkov
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 :-) My approach would need conditional clearing of ->private as well, as far as I can tell. I'll give it a spin.
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. > > 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); -- Pavel Begunkov
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... >> 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.
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
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.
diff --git a/fs/io_uring.c b/fs/io_uring.c index 20781e939940..b2d72bd18fcf 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -4927,6 +4927,8 @@ static void io_poll_task_func(struct callback_head *cb) percpu_ref_put(&ctx->refs); } +static void io_async_task_func(struct callback_head *cb); + static int io_poll_double_wake(struct wait_queue_entry *wait, unsigned mode, int sync, void *key) { @@ -4950,8 +4952,12 @@ static int io_poll_double_wake(struct wait_queue_entry *wait, unsigned mode, /* make sure double remove sees this as being gone */ wait->private = NULL; spin_unlock(&poll->head->lock); - if (!done) - __io_async_wake(req, poll, mask, io_poll_task_func); + if (!done) { + if (req->opcode == IORING_OP_POLL_ADD) + __io_async_wake(req, poll, mask, io_poll_task_func); + else + __io_async_wake(req, poll, mask, io_async_task_func); + } } refcount_dec(&req->refs); return 1;
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. Cc: stable@vger.kernel.org # v5.8+ Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> --- fs/io_uring.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)