Re: [PATCH v2] fs, USB gadget: Rework kiocb cancellation

[Jens Axboe <axboe@kernel.dk>]

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