io_uring/rw: cleanup io_rw_done()

Commit Message

Jens Axboe Jan. 10, 2024, 5:09 p.m. UTC

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 size of the
function by 10 bytes for me on arm64.

Signed-off-by: Jens Axboe <[e-mail]>

---

Comments

Keith Busch Jan. 10, 2024, 6:32 p.m. UTC | #1

On Wed, Jan 10, 2024 at 10:09:19AM -0700, Jens Axboe wrote:
> +static inline void io_rw_done(struct kiocb *kiocb, ssize_t ret)
> +{
> +	if (ret == -EIOCBQUEUED) {
> +		return;
> +	} else if (ret >= 0) {
> +end_io:
> +		INDIRECT_CALL_2(kiocb->ki_complete, io_complete_rw_iopoll,
> +				io_complete_rw, kiocb, ret);
> +	} else {
> +		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;
> +			WARN_ON_ONCE(1);
> +			break;
> +		}
> +		goto end_io;
> +	}
> +}

Are you just trying to get the most common two conditions at the top? A
little rearringing and you can remove the 'goto'. Maybe just my opinion,
but I find using goto for flow control harder to read if there's a
structured alternative.

static inline void io_rw_done(struct kiocb *kiocb, ssize_t ret)
{
	if (ret == -EIOCBQUEUED)
		return;

	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;
			WARN_ON_ONCE(1);
			break;
		}
	}

	INDIRECT_CALL_2(kiocb->ki_complete, io_complete_rw_iopoll,
			io_complete_rw, kiocb, ret);
}

Jens Axboe Jan. 10, 2024, 6:35 p.m. UTC | #2

On 1/10/24 11:32 AM, Keith Busch wrote:
> On Wed, Jan 10, 2024 at 10:09:19AM -0700, Jens Axboe wrote:
>> +static inline void io_rw_done(struct kiocb *kiocb, ssize_t ret)
>> +{
>> +	if (ret == -EIOCBQUEUED) {
>> +		return;
>> +	} else if (ret >= 0) {
>> +end_io:
>> +		INDIRECT_CALL_2(kiocb->ki_complete, io_complete_rw_iopoll,
>> +				io_complete_rw, kiocb, ret);
>> +	} else {
>> +		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;
>> +			WARN_ON_ONCE(1);
>> +			break;
>> +		}
>> +		goto end_io;
>> +	}
>> +}
> 
> Are you just trying to get the most common two conditions at the top? A
> little rearringing and you can remove the 'goto'. Maybe just my opinion,
> but I find using goto for flow control harder to read if there's a
> structured alternative.
> 
> static inline void io_rw_done(struct kiocb *kiocb, ssize_t ret)
> {
> 	if (ret == -EIOCBQUEUED)
> 		return;
> 
> 	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;
> 			WARN_ON_ONCE(1);
> 			break;
> 		}
> 	}
> 
> 	INDIRECT_CALL_2(kiocb->ki_complete, io_complete_rw_iopoll,
> 			io_complete_rw, kiocb, ret);
> }

This does look nicer! I'll fold that in, thanks Keith.

Patch

diff --git a/io_uring/rw.c b/io_uring/rw.c
index 0c856726b15d..885d4f16d11d 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)
+{
+	if (ret == -EIOCBQUEUED) {
+		return;
+	} else if (ret >= 0) {
+end_io:
+		INDIRECT_CALL_2(kiocb->ki_complete, io_complete_rw_iopoll,
+				io_complete_rw, kiocb, ret);
+	} else {
+		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;
+			WARN_ON_ONCE(1);
+			break;
+		}
+		goto end_io;
+	}
+}
+
 static int kiocb_done(struct io_kiocb *req, ssize_t ret,
 		       unsigned int issue_flags)
 {