aio poll V22 (aka 2.0)
Subsystems (3)
Hi all,
this series adds support for the IOCB_CMD_POLL operation to poll for the readyness of file descriptors using the aio subsystem. The API is based on patches that existed in RHAS2.1 and RHEL3, which means it already is supported by libaio.
As our dear leader didn't like the ->poll_mask method this tries to implement the behavior using plain old ->poll which is rather painful. For one we only support ->poll instances with a single wait queue behind them and reject the request otherwise, which isn't really different from the previous ->poll_mask requirement, just implemented in a rathet awkward way.
Second we had to implement a refcount on struct aio_iocb (although it is kept as a no-op for non-poll commands) so that we can safely handle the case of ->poll returning a mask after it got a wakeup. This also means there is a lot of open coded magic for the waitqueue removals and dealing with ki_list to deal with these cases. Last but not least to avoid a guaranteed context switch on every wakeup we trust keyed wakeups, which from an audit of the users seems to be good. The only thing it loses is batching of multiple wakeups in a short time period into a single result.
The changes were sponsored by Scylladb.
git://git.infradead.org/users/hch/vfs.git aio-poll.22
Gitweb:
http://git.infradead.org/users/hch/vfs.git/shortlog/refs/heads/aio-poll.22
Libaio changes:
https://pagure.io/libaio.git io-poll
Seastar changes:
https://github.com/avikivity/seastar/commits/aio
Changes since v21:
- rework the cancellation and early complete logic based on feedback from Al
Changes since v20:
- use a refcount_t instead of an atomic_t for ki_refcnt
Changes since v13:
- rewritten to use ->poll
Changes since v12:
- remove iocb from ki_list only after ki_cancel has completed
- fix __poll_t annotations
- turn __poll_t sparse checkin on by default
- call fput after aio_complete
- only add the iocb to active_reqs if we wait for it
Changes since v11:
- simplify cancellation by completion poll requests from a workqueue if we can't take the ctx_lock
Changes since v10:
- fixed a mismerge that let a sock_rps_record_flow sneak into tcp_poll_mask
- remove the now unused struct proto_ops get_poll_head method
Changes since v9:
- add to the delayed_cancel_reqs earlier to avoid a race
- get rid of POLL_TO_PTR magic
Changes since v8:
- make delayed cancellation conditional again
- add a cancel_kiocb file operation to split delayed vs normal cancel
Changes since v7:
- make delayed cancellation safe and unconditional
Changes since v6:
- reworked cancellation
Changes since v5:
- small changelog updates
- rebased on top of the aio-fsync changes
Changes since v4:
- rebased ontop of Linux 4.16-rc4
Changes since v3:
- remove the pre-sleep ->poll_mask call in vfs_poll, allow ->get_poll_head to return POLL* values.
Changes since v2:
- removed a double initialization
- new vfs_get_poll_head helper
- document that ->get_poll_head can return NULL
- call ->poll_mask before sleeping
- various ACKs
- add conversion of random to ->poll_mask
- add conversion of af_alg to ->poll_mask
- lacking ->poll_mask support now returns -EINVAL for IOCB_CMD_POLL
- reshuffled the series so that prep patches and everything not requiring the new in-kernel poll API is in the beginning
Changes since v1:
- handle the NULL ->poll case in vfs_poll
- dropped the file argument to the ->poll_mask socket operation
- replace the ->pre_poll socket operation with ->get_poll_head as in the file operations
PATCH 1/4 timerfd: add support for keyed wakeups
This prepares timerfd for use with aio poll.
--- fs/timerfd.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
fs/timerfd.c +3 -3
@@ -66,7 +66,7 @@ static void timerfd_triggered(struct timerfd_ctx *ctx)spin_lock_irqsave (&ctx->wqh.lock, flags); ctx->expired =1 ; ctx->ticks++; -wake_up_locked (&ctx->wqh); +wake_up_locked_poll (&ctx->wqh,EPOLLIN );spin_unlock_irqrestore (&ctx->wqh.lock, flags); }
@@ -107,7 +107,7 @@ void timerfd_clock_was_set(void)if (ctx->moffs != moffs) { ctx->moffs =KTIME_MAX ; ctx->ticks++; -wake_up_locked (&ctx->wqh); +wake_up_locked_poll (&ctx->wqh,EPOLLIN ); }spin_unlock_irqrestore (&ctx->wqh.lock, flags); }
@@ -345,7 +345,7 @@ static long timerfd_ioctl(struct file *file, unsigned int cmd, unsigned long argspin_lock_irq (&ctx->wqh.lock);if (!timerfd_canceled (ctx)) { ctx->ticks = ticks; -wake_up_locked (&ctx->wqh); +wake_up_locked_poll (&ctx->wqh,EPOLLIN ); }else ret = -ECANCELED ;spin_unlock_irq (&ctx->wqh.lock);
Signature · 1 line
2.18.0
PATCH 2/4 aio: add a iocb refcount
In reply to Christoph HellwigThis is needed to prevent races caused by the way the ->poll API works. To avoid introducing overhead for other users of the iocbs we initialize it to zero and only do refcount operations if it is non-zero in the completion path.
--- fs/aio.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)
fs/aio.c +13 -4
@@ -18,6 +18,7 @@#include <linux/export.h> #include <linux/syscalls.h> #include <linux/backing-dev.h> +#include <linux/refcount.h> #include <linux/uio.h> #include <linux/sched/signal.h>
@@ -178,6 +179,7 @@ struct aio_kiocb {struct list_head ki_list;/* the aio core uses this * for cancellation */ +refcount_t ki_refcnt;/* * If the aio_resfd field of the userspace iocb is not zero,
@@ -1015,6 +1017,7 @@ static inline struct aio_kiocb *aio_get_req(struct kioctx *ctx)percpu_ref_get (&ctx->reqs);INIT_LIST_HEAD (&req->ki_list); +refcount_set (&req->ki_refcnt,0 ); req->ki_ctx = ctx;return req; out_put:
@@ -1049,6 +1052,15 @@ static struct kioctx *lookup_ioctx(unsigned long ctx_id)return ret; } +static inline void iocb_put (struct aio_kiocb *iocb) +{ +if (refcount_read (&iocb->ki_refcnt) ==0 || +refcount_dec_and_test (&iocb->ki_refcnt)) { +percpu_ref_put (&iocb->ki_ctx->reqs); +kmem_cache_free (kiocb_cachep, iocb); + } +} +/* aio_complete * Called when the io request on the given iocb is complete. */
@@ -1118,8 +1130,6 @@ static void aio_complete(struct aio_kiocb *iocb, long res, long res2)eventfd_ctx_put (iocb->ki_eventfd); } -kmem_cache_free (kiocb_cachep, iocb); -/* * We have to order our ring_info tail store above and test * of the wait list below outside the wait lock. This is
@@ -1130,8 +1140,7 @@ static void aio_complete(struct aio_kiocb *iocb, long res, long res2)if (waitqueue_active (&ctx->wait))wake_up (&ctx->wait); - -percpu_ref_put (&ctx->reqs); +iocb_put (iocb); }/* aio_read_events_ring
Signature · 1 line
2.18.0
PATCH 3/4 aio: implement IOCB_CMD_POLL
In reply to Christoph HellwigSimple one-shot poll through the io_submit() interface. To poll for a file descriptor the application should submit an iocb of type IOCB_CMD_POLL. It will poll the fd for the events specified in the the first 32 bits of the aio_buf field of the iocb.
Unlike poll or epoll without EPOLLONESHOT this interface always works in one shot mode, that is once the iocb is completed, it will have to be resubmitted.
--- fs/aio.c | 178 +++++++++++++++++++++++++++++++++++ include/uapi/linux/aio_abi.h | 6 +- 2 files changed, 180 insertions(+), 4 deletions(-)
fs/aio.c +178 -0
@@ -5,6 +5,7 @@* Implements an efficient asynchronous io interface. * * Copyright 2000, 2001, 2002 Red Hat, Inc. All Rights Reserved. +* Copyright 2018 Christoph Hellwig. * * See ../COPYING for licensing terms. */
@@ -165,10 +166,21 @@ struct fsync_iocb {bool datasync; }; +struct poll_iocb { +struct file *file; +struct wait_queue_head *head; +__poll_t events; +bool woken; +bool cancelled; +struct wait_queue_entry wait; +struct work_struct work; +}; +struct aio_kiocb {union {struct kiocb rw;struct fsync_iocb fsync; +struct poll_iocb poll; };struct kioctx *ki_ctx;
@@ -1601,6 +1613,169 @@ static int aio_fsync(struct fsync_iocb *req, struct iocb *iocb, bool datasync)return 0 ; } +static inline void aio_poll_complete (struct aio_kiocb *iocb,__poll_t mask) +{ +struct file *file = iocb->poll.file; + +aio_complete (iocb,mangle_poll (mask),0 ); +fput (file); +} + +static void aio_poll_complete_work (struct work_struct *work) +{ + struct poll_iocb *req = container_of(work, struct poll_iocb, work); + struct aio_kiocb *iocb = container_of(req, struct aio_kiocb, poll); +struct poll_table_struct pt = { ._key = req->events }; +struct kioctx *ctx = iocb->ki_ctx; +__poll_t mask =0 ; + +if (!READ_ONCE (req->cancelled)) + mask =vfs_poll (req->file, &pt) & req->events; + +/* +* Note that ->ki_cancel callers also delete iocb from active_reqs after +* calling ->ki_cancel. We need the ctx_lock roundtrip here to +* synchronize with them. In the cancellation case the list_del_init +* itself is not actually needed, but harmless so we keep it in to +* avoid further branches in the fast path. +*/ +spin_lock_irq (&ctx->ctx_lock); +if (!mask && !READ_ONCE (req->cancelled)) { +add_wait_queue (req->head, &req->wait); +spin_unlock_irq (&ctx->ctx_lock); +return ; + } +list_del_init (&iocb->ki_list); +spin_unlock_irq (&ctx->ctx_lock); + +aio_poll_complete (iocb, mask); +} + +/* assumes we are called with irqs disabled */ +static int aio_poll_cancel (struct kiocb *iocb) +{ + struct aio_kiocb *aiocb = container_of(iocb, struct aio_kiocb, rw); +struct poll_iocb *req = &aiocb->poll; + +spin_lock (&req->head->lock); +WRITE_ONCE (req->cancelled, true); +if (!list_empty (&req->wait.entry)) { +list_del_init (&req->wait.entry); +schedule_work (&aiocb->poll.work); + } +spin_unlock (&req->head->lock); + +return 0 ; +} + +static int aio_poll_wake (struct wait_queue_entry *wait,unsigned mode,int sync, +void *key) +{ + struct poll_iocb *req = container_of(wait, struct poll_iocb, wait); +__poll_t mask =key_to_poll (key); + + req->woken = true; + +/* for instances that support it check for an event match first: */ +if (mask && !(mask & req->events)) +return 0 ; + +list_del_init (&req->wait.entry); +schedule_work (&req->work); +return 1 ; +} + +struct aio_poll_table { +struct poll_table_struct pt; +struct aio_kiocb *iocb; +int error; +}; + +static void +aio_poll_queue_proc (struct file *file,struct wait_queue_head *head, +struct poll_table_struct *p) +{ + struct aio_poll_table *pt = container_of(p, struct aio_poll_table, pt); + +/* multiple wait queues per file are not supported */ +if (unlikely (pt->iocb->poll.head)) { + pt->error = -EINVAL ; +return ; + } + + pt->error =0 ; + pt->iocb->poll.head = head; +add_wait_queue (head, &pt->iocb->poll.wait); +} + +static ssize_t aio_poll (struct aio_kiocb *aiocb,struct iocb *iocb) +{ +struct kioctx *ctx = aiocb->ki_ctx; +struct poll_iocb *req = &aiocb->poll; +struct aio_poll_table apt; +__poll_t mask; + +/* reject any unknown events outside the normal event mask. */ +if ((u16 )iocb->aio_buf != iocb->aio_buf) +return -EINVAL ; +/* reject fields that are not defined for poll */ +if (iocb->aio_offset || iocb->aio_nbytes || iocb->aio_rw_flags) +return -EINVAL ; + +INIT_WORK (&req->work, aio_poll_complete_work); + req->events =demangle_poll (iocb->aio_buf) |EPOLLERR |EPOLLHUP ; + req->file =fget (iocb->aio_fildes); +if (unlikely (!req->file)) +return -EBADF ; + + apt.pt._qproc = aio_poll_queue_proc; + apt.pt._key = req->events; + apt.iocb = aiocb; + apt.error = -EINVAL ;/* same as no support for IOCB_CMD_POLL */ + +/* initialized the list so that we can do list_empty checks */ +INIT_LIST_HEAD (&req->wait.entry); +init_waitqueue_func_entry (&req->wait, aio_poll_wake); + +/* one for removal from waitqueue, one for this function */ +refcount_set (&aiocb->ki_refcnt,2 ); + + mask =vfs_poll (req->file, &apt.pt) & req->events; +if (unlikely (!req->head)) { +/* we did not manage to set up a waitqueue, done */ + goto out; + } + +spin_lock_irq (&ctx->ctx_lock); +spin_lock (&req->head->lock); +if (req->woken) { +/* wake_up context handles the rest */ + mask =0 ; + apt.error =0 ; + }else if (mask || apt.error) { +/* if we get an error or a mask we are done */ +WARN_ON_ONCE (list_empty (&req->wait.entry)); +list_del_init (&req->wait.entry); + }else { +/* actually waiting for an event */ +list_add_tail (&aiocb->ki_list, &ctx->active_reqs); + aiocb->ki_cancel = aio_poll_cancel; + } +spin_unlock (&req->head->lock); +spin_unlock_irq (&ctx->ctx_lock); + +out: +if (unlikely (apt.error)) { +fput (req->file); +return apt.error; + } + +if (mask) +aio_poll_complete (aiocb, mask); +iocb_put (aiocb); +return 0 ; +} +static int io_submit_one (struct kioctx *ctx,struct iocb __user *user_iocb,bool compat) {
@@ -1674,6 +1849,9 @@ static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,case IOCB_CMD_FDSYNC : ret =aio_fsync (&req->fsync, &iocb, true);break ; +case IOCB_CMD_POLL : + ret =aio_poll (req, &iocb); +break ;default :pr_debug ("invalid aio operation %d\n" , iocb.aio_lio_opcode); ret = -EINVAL ;
include/uapi/linux/aio_abi.h +2 -4
@@ -38,10 +38,8 @@ enum {IOCB_CMD_PWRITE =1 ,IOCB_CMD_FSYNC =2 ,IOCB_CMD_FDSYNC =3 , -/* These two are experimental. -* IOCB_CMD_PREADX = 4, -* IOCB_CMD_POLL = 5, -*/ +/* 4 was the experimental IOCB_CMD_PREADX */ +IOCB_CMD_POLL =5 ,IOCB_CMD_NOOP =6 ,IOCB_CMD_PREADV =7 , IOCB_CMD_PWRITEV = 8,
Signature · 1 line
2.18.0
PATCH 4/4 aio: allow direct aio poll comletions for keyed wakeups
In reply to Christoph HellwigIf we get a keyed wakeup for a aio poll waitqueue and wake can acquire the ctx_lock without spinning we can just complete the iocb straight from the wakeup callback to avoid a context switch.
--- fs/aio.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-)
fs/aio.c +15 -2
@@ -1672,13 +1672,26 @@ static int aio_poll_wake(struct wait_queue_entry *wait, unsigned mode, int sync, void *key) { struct poll_iocb *req = container_of(wait, struct poll_iocb, wait); + struct aio_kiocb *iocb = container_of(req, struct aio_kiocb, poll);__poll_t mask =key_to_poll (key); req->woken = true;/* for instances that support it check for an event match first: */ -if (mask && !(mask & req->events)) -return 0 ; +if (mask) { +if (!(mask & req->events)) +return 0 ; + +/* try to complete the iocb inline if we can: */ +if (spin_trylock (&iocb->ki_ctx->ctx_lock)) { +list_del (&iocb->ki_list); +spin_unlock (&iocb->ki_ctx->ctx_lock); + +list_del_init (&req->wait.entry); +aio_poll_complete (iocb, mask); +return 1 ; + } + }list_del_init (&req->wait.entry);schedule_work (&req->work);
Signature · 1 line
2.18.0
Re: [PATCH 4/4] aio: allow direct aio poll comletions for keyed wakeups
Quoted 3 lines · On Mon, 6 Aug 2018 10:30:58 +0200 Christoph Hellwig <[email protected]> wrote:
If we get a keyed wakeup for a aio poll waitqueue and wake can acquire the
ctx_lockwithout spinning we can just complete the iocb straight from the wakeup callback to avoid a context switch.
Why do we try to avoid spinning on the lock?
Quoted 9 lines
fs/aio.c +1
-0@@ -1672,13 +1672,26 @@ static int aio_poll_wake(struct wait_queue_entry *wait, unsigned mode, int sync, void *key) { struct poll_iocb *req = container_of(wait, struct poll_iocb, wait); + struct aio_kiocb *iocb = container_of(req, struct aio_kiocb, poll);__poll_t mask =key_to_poll (key);
fs/aio.c static int aio_poll_wake(struct wait_queue_entry *wait, unsigned mode, int sync, · line 1685
Quoted 10 lines
fs/aio.c +5
-2req->woken = true;/* for instances that support it check for an event match first: */ -if (mask && !(mask & req->events)) -return 0 ; +if (mask) { +if (!(mask & req->events)) +return 0 ; + +/* try to complete the iocb inline if we can: */
ie, this comment explains 'what" but not "why".
(There's a typo in Subject:, btw)
Quoted 12 lines
fs/aio.c +9
-0+if (spin_trylock (&iocb->ki_ctx->ctx_lock)) { +list_del (&iocb->ki_list); +spin_unlock (&iocb->ki_ctx->ctx_lock); + +list_del_init (&req->wait.entry); +aio_poll_complete (iocb, mask); +return 1 ; + } + }list_del_init (&req->wait.entry);schedule_work (&req->work);
Re: [PATCH 4/4] aio: allow direct aio poll comletions for keyed wakeups
Quoted 7 lines · Andrew Morton
Quoted 3 lines · On Mon, 6 Aug 2018 10:30:58 +0200 Christoph Hellwig <[email protected]> wrote:
If we get a keyed wakeup for a aio poll waitqueue and wake can acquire the
ctx_lockwithout spinning we can just complete the iocb straight from the wakeup callback to avoid a context switch.Why do we try to avoid spinning on the lock?
Because we are called with the lock on the waitqueue called, which nests inside it.
Quoted 5 lines
fs/aio.c static int aio_poll_wake(struct wait_queue_entry *wait, unsigned mode, int sync, · line 1685
Quoted 1 line
fs/aio.c +1
-0+/* try to complete the iocb inline if we can: */ ie, this comment explains 'what" but not "why".
(There's a typo in Subject:, btw)
Because it is faster obviously. I can update the comment.
Re: [PATCH 4/4] aio: allow direct aio poll comletions for keyed wakeups
Quoted 11 lines · On Tue, 7 Aug 2018 09:25:55 +0200 Christoph Hellwig <[email protected]> wrote:
Quoted 7 lines · Andrew Morton
Quoted 3 lines · On Mon, 6 Aug 2018 10:30:58 +0200 Christoph Hellwig <[email protected]> wrote:
If we get a keyed wakeup for a aio poll waitqueue and wake can acquire the
ctx_lockwithout spinning we can just complete the iocb straight from the wakeup callback to avoid a context switch.Why do we try to avoid spinning on the lock?
Because we are called with the lock on the waitqueue called, which nests inside it.
Ah.
Quoted 7 lines
Quoted 5 lines
fs/aio.c static int aio_poll_wake(struct wait_queue_entry *wait, unsigned mode, int sync, · line 1685
Quoted 1 line
fs/aio.c +1
-0+/* try to complete the iocb inline if we can: */ ie, this comment explains 'what" but not "why".
(There's a typo in Subject:, btw)
Because it is faster obviously. I can update the comment.
I meant the comment could explain why it's a trylock instead of a spin_lock().
Re: [PATCH 4/4] aio: allow direct aio poll comletions for keyed wakeups
Quoted 4 lines · Andrew Morton
Quoted 1 line
Because it is faster obviously. I can update the comment.
I meant the comment could explain why it's a trylock instead of a
spin_lock().
We could something like this the patch below.
Al, do you want me to resend or can you just fold it in?
fs/aio.c +2 -1
@@ -1684,7 +1684,8 @@ static int aio_poll_wake(struct wait_queue_entry *wait, unsigned mode, int sync,/* * Try to complete the iocb inline if we can to avoid a costly -* context switch. +* context switch. As the waitqueue lock nests inside the ctx +* lock we can only do that if we can get it without waiting. */ if (spin_trylock (&iocb->ki_ctx->ctx_lock)) {list_del (&iocb->ki_list);
PATCH v2 4/4 aio: allow direct aio poll comletions for keyed wakeups
In reply to Christoph HellwigIf we get a keyed wakeup for a aio poll waitqueue and wake can acquire the ctx_lock without spinning we can just complete the iocb straight from the wakeup callback to avoid a context switch.
---
Fix a subject line typo and improve a comment.
fs/aio.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)
fs/aio.c +18 -2
@@ -1672,13 +1672,29 @@ static int aio_poll_wake(struct wait_queue_entry *wait, unsigned mode, int sync, void *key) { struct poll_iocb *req = container_of(wait, struct poll_iocb, wait); + struct aio_kiocb *iocb = container_of(req, struct aio_kiocb, poll);__poll_t mask =key_to_poll (key); req->woken = true;/* for instances that support it check for an event match first: */ -if (mask && !(mask & req->events)) -return 0 ; +if (mask) { +if (!(mask & req->events)) +return 0 ; + +/* +* Try to complete the iocb inline if we can to avoid a costly +* context switch. +*/ +if (spin_trylock (&iocb->ki_ctx->ctx_lock)) { +list_del (&iocb->ki_list); +spin_unlock (&iocb->ki_ctx->ctx_lock); + +list_del_init (&req->wait.entry); +aio_poll_complete (iocb, mask); +return 1 ; + } + }list_del_init (&req->wait.entry);schedule_work (&req->work);
Signature · 1 line
2.18.0
Quoted 3 lines · Christoph Hellwig
As our dear leader didn't like the ->poll_mask method this tries to implement the behavior using plain old ->poll which is rather painful.
I'm not seeing what's painful for this. Looking at the patches, this is *much* more straightforward than your previous patch,
It adds refcounting to aio_iocb, but that's *much* better than messing up every other subsystem.
Or is there some follow-up patches that are pending but you didn't post that are the painful part? Because the diffstat says that this second version is *way* less painful, at about 200 lines of code in a couple of files, mostly aio, vs ~700 lines of changes all over the place, together with a performance regression.
Linus
Quoted 11 lines · Linus Torvalds
I'm not seeing what's painful for this. Looking at the patches, this is *much* more straightforward than your previous patch,
It adds refcounting to
aio_iocb, but that's *much* better than messing up every other subsystem.Or is there some follow-up patches that are pending but you didn't post that are the painful part? Because the diffstat says that this second version is *way* less painful, at about 200 lines of code in a couple of files, mostly aio, vs ~700 lines of changes all over the place, together with a performance regression.
It requires additional lock roundtrips and very strange life time rules. But we've already established that our preference here are different, so I'm not surprised by your different view.