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.