[PATCH v2] io_uring/rw: cleanup io_rw_done()

[PATCH v2] io_uring/rw: cleanup io_rw_done()

[Jens Axboe]

This originally came from the aio side, and it's laid out rather oddly.
The common case here is that we either get -EIOCBQUEUED from submitting
an async request, or that we complete the request correctly with the
given number of bytes. Handling the odd internal restart error codes
is not a common operation.

Lay it out a bit more optimally that better explains the normal flow,
and switch to avoiding the indirect call completely as this is our
kiocb and we know the completion handler can only be one of two
possible variants. While at it, move it to where it belongs in the
file, with fellow end IO helpers.

Outside of being easier to read, this also reduces the text size of the
function by 24 bytes for me on arm64.

Signed-off-by: Jens Axboe <axboe@kernel.dk>

---

v2:	- Further cleanup readability (Keith)
	- Add a few comments

diff --git a/io_uring/rw.c b/io_uring/rw.c
index 0c856726b15d..118cc9f1cf16 100644
--- a/io_uring/rw.c
+++ b/io_uring/rw.c
@@ -168,27 +168,6 @@ void io_readv_writev_cleanup(struct io_kiocb *req)
 	kfree(io->free_iovec);
 }
 
-static inline void io_rw_done(struct kiocb *kiocb, ssize_t ret)
-{
-	switch (ret) {
-	case -EIOCBQUEUED:
-		break;
-	case -ERESTARTSYS:
-	case -ERESTARTNOINTR:
-	case -ERESTARTNOHAND:
-	case -ERESTART_RESTARTBLOCK:
-		/*
-		 * We can't just restart the syscall, since previously
-		 * submitted sqes may already be in progress. Just fail this
-		 * IO with EINTR.
-		 */
-		ret = -EINTR;
-		fallthrough;
-	default:
-		kiocb->ki_complete(kiocb, ret);
-	}
-}
-
 static inline loff_t *io_kiocb_update_pos(struct io_kiocb *req)
 {
 	struct io_rw *rw = io_kiocb_to_cmd(req, struct io_rw);
@@ -371,6 +350,33 @@ static void io_complete_rw_iopoll(struct kiocb *kiocb, long res)
 	smp_store_release(&req->iopoll_completed, 1);
 }
 
+static inline void io_rw_done(struct kiocb *kiocb, ssize_t ret)
+{
+	/* IO was queued async, completion will happen later */
+	if (ret == -EIOCBQUEUED)
+		return;
+
+	/* transform internal restart error codes */
+	if (unlikely(ret < 0)) {
+		switch (ret) {
+		case -ERESTARTSYS:
+		case -ERESTARTNOINTR:
+		case -ERESTARTNOHAND:
+		case -ERESTART_RESTARTBLOCK:
+			/*
+			 * We can't just restart the syscall, since previously
+			 * submitted sqes may already be in progress. Just fail
+			 * this IO with EINTR.
+			 */
+			ret = -EINTR;
+			break;
+		}
+	}
+
+	INDIRECT_CALL_2(kiocb->ki_complete, io_complete_rw_iopoll,
+			io_complete_rw, kiocb, ret);
+}
+
 static int kiocb_done(struct io_kiocb *req, ssize_t ret,
 		       unsigned int issue_flags)
 {

-- 
Jens Axboe

Re: [PATCH v2] io_uring/rw: cleanup io_rw_done()

[Keith Busch]

On Wed, Jan 10, 2024 at 11:38:02AM -0700, Jens Axboe wrote:
> This originally came from the aio side, and it's laid out rather oddly.
> The common case here is that we either get -EIOCBQUEUED from submitting
> an async request, or that we complete the request correctly with the
> given number of bytes. Handling the odd internal restart error codes
> is not a common operation.
> 
> Lay it out a bit more optimally that better explains the normal flow,
> and switch to avoiding the indirect call completely as this is our
> kiocb and we know the completion handler can only be one of two
> possible variants. While at it, move it to where it belongs in the
> file, with fellow end IO helpers.
> 
> Outside of being easier to read, this also reduces the text size of the
> function by 24 bytes for me on arm64.
> 
> Signed-off-by: Jens Axboe <axboe@kernel.dk>

Looks good!

Reviewed-by: Keith Busch <kbusch@kernel.org>