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

[Jens Axboe <axboe@kernel.dk>]

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