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.
--
Jens Axboe
