On 2/8/24 4:05 PM, Jens Axboe wrote:
On 2/8/24 3:41 PM, Bart Van Assche wrote:
Just move the function higher up? It doesn't have any dependencies.
aio_cancel_and_del() calls aio_poll_cancel(). aio_poll_cancel() calls poll_iocb_lock_wq(). poll_iocb_lock_wq() is defined below the first call of aio_cancel_and_del(). It's probably possible to get rid of that function declaration but a nontrivial amount of code would have to be moved.
Ah yes, I mixed it up with the cancel add helper. Forward decl is fine then, keeps the patch smaller for backporting too.
+{
- void (*cancel_kiocb)(struct kiocb *) =
req->rw.ki_filp->f_op->cancel_kiocb;- struct kioctx *ctx = req->ki_ctx;
- lockdep_assert_held(&ctx->ctx_lock);
- switch (req->ki_opcode) {
- case IOCB_CMD_PREAD:
- case IOCB_CMD_PWRITE:
- case IOCB_CMD_PREADV:
- case IOCB_CMD_PWRITEV:
if (cancel_kiocb)cancel_kiocb(&req->rw);break;- case IOCB_CMD_FSYNC:
- case IOCB_CMD_FDSYNC:
break;- case IOCB_CMD_POLL:
aio_poll_cancel(req);break;- default:
WARN_ONCE(true, "invalid aio operation %d\n", req->ki_opcode);- }
- list_del_init(&req->ki_list);
+}
Why don't you just keep ki_cancel() and just change it to a void return that takes an aio_kiocb? Then you don't need this odd switch, or adding an opcode field just for this. That seems cleaner.
Keeping .ki_cancel() means that it must be set before I/O starts and only if the I/O is submitted by libaio. That would require an approach to recognize whether or not a struct kiocb is embedded in struct aio_kiocb, e.g. the patch that you posted as a reply on version one of this patch. Does anyone else want to comment on this?
Maybe I wasn't clear, but this is in aio_req. You already add an opcode in there, only to then add a switch here based on that opcode. Just have a cancel callback which takes aio_req as an argument. For POLL, this can be aio_poll_cancel(). Add a wrapper for read/write which then calls req->rw.ki_filp->f_op->cancel_kiocb(&req->rw); Then the above can become:
aio_rw_cancel(req) { void (*cancel_kiocb)(struct kiocb *) = req->rw.ki_filp->f_op->cancel_kiocb;
cancel_kiocb(&req->rw); }
aio_read() { ... req->cancel = aio_rw_cancel; ... }
static void aio_cancel_and_del(struct aio_kiocb *req) { void (*cancel_kiocb)(struct kiocb *) = req->rw.ki_filp->f_op->cancel_kiocb; struct kioctx *ctx = req->ki_ctx;
lockdep_assert_held(&ctx->ctx_lock); if (req->cancel) req->cancel(req); list_del_init(&req->ki_list); }
or something like that. fsync/fdsync clears ->cancel() to NULL, poll sets it to aio_poll_cancel(), and read/write like the above.
Totally untested incremental. I think this is cleaner, and it's less code too.
diff --git a/fs/aio.c b/fs/aio.c index 9dc0be703aa6..a7770f59269f 100644 --- a/fs/aio.c +++ b/fs/aio.c @@ -202,6 +202,8 @@ struct aio_kiocb { struct poll_iocb poll; };
+ void (*ki_cancel)(struct aio_kiocb *); + struct kioctx *ki_ctx;
struct io_event ki_res; @@ -210,8 +212,6 @@ struct aio_kiocb { * for cancellation */ refcount_t ki_refcnt;
- u16 ki_opcode; /* IOCB_CMD_* */ - /* * If the aio_resfd field of the userspace iocb is not zero, * this is the underlying eventfd context to deliver events to. @@ -1576,6 +1576,11 @@ static inline void aio_rw_done(struct kiocb *req, ssize_t ret) } }
+static void aio_rw_cancel(struct aio_kiocb *req) +{ + iocb->ki_filp->f_op->cancel_kiocb(iocb); +} + static int aio_read(struct kiocb *req, const struct iocb *iocb, bool vectored, bool compat) { @@ -1722,50 +1727,14 @@ static void poll_iocb_unlock_wq(struct poll_iocb *req) rcu_read_unlock(); }
-/* Must be called only for IOCB_CMD_POLL requests. */ -static void aio_poll_cancel(struct aio_kiocb *aiocb) -{ - struct poll_iocb *req = &aiocb->poll; - struct kioctx *ctx = aiocb->ki_ctx; - - lockdep_assert_held(&ctx->ctx_lock); - - if (!poll_iocb_lock_wq(req)) - return; - - WRITE_ONCE(req->cancelled, true); - if (!req->work_scheduled) { - schedule_work(&aiocb->poll.work); - req->work_scheduled = true; - } - poll_iocb_unlock_wq(req); -} - static void aio_cancel_and_del(struct aio_kiocb *req) { - void (*cancel_kiocb)(struct kiocb *) = - req->rw.ki_filp->f_op->cancel_kiocb; struct kioctx *ctx = req->ki_ctx;
lockdep_assert_held(&ctx->ctx_lock);
- switch (req->ki_opcode) { - case IOCB_CMD_PREAD: - case IOCB_CMD_PWRITE: - case IOCB_CMD_PREADV: - case IOCB_CMD_PWRITEV: - if (cancel_kiocb) - cancel_kiocb(&req->rw); - break; - case IOCB_CMD_FSYNC: - case IOCB_CMD_FDSYNC: - break; - case IOCB_CMD_POLL: - aio_poll_cancel(req); - break; - default: - WARN_ONCE(true, "invalid aio operation %d\n", req->ki_opcode); - } + if (req->ki_cancel) + req->ki_cancel(req);
list_del_init(&req->ki_list); } @@ -1922,6 +1891,25 @@ aio_poll_queue_proc(struct file *file, struct wait_queue_head *head, add_wait_queue(head, &pt->iocb->poll.wait); }
+/* Must be called only for IOCB_CMD_POLL requests. */ +static void aio_poll_cancel(struct aio_kiocb *aiocb) +{ + struct poll_iocb *req = &aiocb->poll; + struct kioctx *ctx = aiocb->ki_ctx; + + lockdep_assert_held(&ctx->ctx_lock); + + if (!poll_iocb_lock_wq(req)) + return; + + WRITE_ONCE(req->cancelled, true); + if (!req->work_scheduled) { + schedule_work(&aiocb->poll.work); + req->work_scheduled = true; + } + poll_iocb_unlock_wq(req); +} + static int aio_poll(struct aio_kiocb *aiocb, const struct iocb *iocb) { struct kioctx *ctx = aiocb->ki_ctx; @@ -2028,23 +2016,27 @@ static int __io_submit_one(struct kioctx *ctx, const struct iocb *iocb, req->ki_res.data = iocb->aio_data; req->ki_res.res = 0; req->ki_res.res2 = 0; - - req->ki_opcode = iocb->aio_lio_opcode; + req->ki_cancel = NULL;
switch (iocb->aio_lio_opcode) { case IOCB_CMD_PREAD: + req->ki_cancel = aio_rw_cancel; return aio_read(&req->rw, iocb, false, compat); case IOCB_CMD_PWRITE: + req->ki_cancel = aio_rw_cancel; return aio_write(&req->rw, iocb, false, compat); case IOCB_CMD_PREADV: + req->ki_cancel = aio_rw_cancel; return aio_read(&req->rw, iocb, true, compat); case IOCB_CMD_PWRITEV: + req->ki_cancel = aio_rw_cancel; return aio_write(&req->rw, iocb, true, compat); case IOCB_CMD_FSYNC: return aio_fsync(&req->fsync, iocb, false); case IOCB_CMD_FDSYNC: return aio_fsync(&req->fsync, iocb, true); case IOCB_CMD_POLL: + req->ki_cancel = aio_poll_cancel; return aio_poll(req, iocb); default: pr_debug("invalid aio operation %d\n", iocb->aio_lio_opcode);