| Message ID | YbOdV8CPbyPAF234@sol.localdomain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [GIT,PULL] aio poll fixes for 5.16-rc5 | expand |
On Fri, Dec 10, 2021 at 10:33 AM Eric Biggers <ebiggers@kernel.org> wrote: > > This has been tested with the libaio test suite, as well as with test > programs I wrote that reproduce the first two bugs. I am sending this > pull request myself as no one seems to be maintaining this code. Pulled. The "nobody really maintains or cares about epoll/aio" makes me wonder if we should just remove the "if EXPERT" from the config options we have on them, and start encouraging people to perhaps not even build that code any more? Because I'm sure we have users of it, but maybe they are few enough that saying "don't enable this feature unless you need it" is the right thing to do... Linus
The pull request you sent on Fri, 10 Dec 2021 10:32:55 -0800:
> https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git tags/aio-poll-for-linus
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/0d21e6684779493d90f3dee90d4457d5b4aed8ad
Thank you!
On Fri, Dec 10, 2021 at 02:18:12PM -0800, Linus Torvalds wrote: > On Fri, Dec 10, 2021 at 10:33 AM Eric Biggers <ebiggers@kernel.org> wrote: > > > > This has been tested with the libaio test suite, as well as with test > > programs I wrote that reproduce the first two bugs. I am sending this > > pull request myself as no one seems to be maintaining this code. > > Pulled. > > The "nobody really maintains or cares about epoll/aio" makes me wonder > if we should just remove the "if EXPERT" from the config options we > have on them, and start encouraging people to perhaps not even build > that code any more? > > Because I'm sure we have users of it, but maybe they are few enough > that saying "don't enable this feature unless you need it" is the > right thing to do... Isn't epoll more commonly used than aio? Either way, removing 'if EXPERT' from both would make sense, so that they aren't forced on just because someone didn't set CONFIG_EXPERT. I think that a lot of people have these options enabled but don't need them. Android used to be in that boat for CONFIG_AIO (i.e. it wasn't needed but was sometimes enabled anyway, maybe due to !CONFIG_EXPERT). Unfortunately Android has started depending on CONFIG_AIO, so it seems Android will need to keep it set, but I think most other Linux systems don't need it. - Eric
On Fri, Dec 10, 2021 at 03:00:36PM -0800, Eric Biggers wrote: > > Isn't epoll more commonly used than aio? Either way, removing 'if EXPERT' from > both would make sense, so that they aren't forced on just because someone didn't > set CONFIG_EXPERT. I think that a lot of people have these options enabled but > don't need them. Android used to be in that boat for CONFIG_AIO (i.e. it wasn't > needed but was sometimes enabled anyway, maybe due to !CONFIG_EXPERT). > Unfortunately Android has started depending on CONFIG_AIO, so it seems Android > will need to keep it set, but I think most other Linux systems don't need it. Mysql and Postgress both can use libaio, and I suspect many distributions are compiling them with AIO enabled, since you can get better performance with AIO. Fio also uses AIO, and many fio recipes that are trying to benchmark file systems or block devices use AIO/DIO. It's fair to say that the libaio programming interface is pretty horrendo, and so very few application programmers will happy choosing to use it. But if you really care about storage performance, whether you're implementing a database, or a cluster file system, it's likely that you will find yourself deciding to try to use it. The fact that so many storage-centric userspace programs use it *desite* libaio's developer-hostile interface is a proof point of how much AIO can help with performance. Maybe over time we can get folks to switch to io_uring, and we will eventually be able to get us to the happy place where most Linux systems won't need CONFIG_AIO. But that day is not today. :-/ So removing the dependency on CONFIG_EXPERT is probably a good idea, at least for now. Cheers, - Ted
On Fri, Dec 10, 2021 at 07:45:39PM -0500, Theodore Y. Ts'o wrote: > distributions are compiling them with AIO enabled, since you can get > better performance with AIO. Fio also uses AIO, and many fio recipes > that are trying to benchmark file systems or block devices use > AIO/DIO. As does qemu and many commercial databases.
The following changes since commit 0fcfb00b28c0b7884635dacf38e46d60bf3d4eb1: Linux 5.16-rc4 (2021-12-05 14:08:22 -0800) are available in the Git repository at: https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git tags/aio-poll-for-linus for you to fetch changes up to 4b3749865374899e115aa8c48681709b086fe6d3: aio: Fix incorrect usage of eventfd_signal_allowed() (2021-12-09 10:52:55 -0800) ---------------------------------------------------------------- Fix three bugs in aio poll, and one issue with POLLFREE more broadly: - aio poll didn't handle POLLFREE, causing a use-after-free. - aio poll could block while the file is ready. - aio poll called eventfd_signal() when it isn't allowed. - POLLFREE didn't handle multiple exclusive waiters correctly. This has been tested with the libaio test suite, as well as with test programs I wrote that reproduce the first two bugs. I am sending this pull request myself as no one seems to be maintaining this code. ---------------------------------------------------------------- Eric Biggers (5): wait: add wake_up_pollfree() binder: use wake_up_pollfree() signalfd: use wake_up_pollfree() aio: keep poll requests on waitqueue until completed aio: fix use-after-free due to missing POLLFREE handling Xie Yongji (1): aio: Fix incorrect usage of eventfd_signal_allowed() drivers/android/binder.c | 21 ++--- fs/aio.c | 186 ++++++++++++++++++++++++++++++++-------- fs/signalfd.c | 12 +-- include/linux/wait.h | 26 ++++++ include/uapi/asm-generic/poll.h | 2 +- kernel/sched/wait.c | 7 ++ 6 files changed, 196 insertions(+), 58 deletions(-)