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);
--
Jens Axboe
