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

[Jens Axboe <axboe@kernel.dk>]

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.

-- 
Jens Axboe