Re: io_uring: RWF_NOWAIT support

From: "Stefan Bühler" <source@stbuehler.de>
To: Jens Axboe <axboe@kernel.dk>,
	linux-block@vger.kernel.org, linux-fsdevel@vger.kernel.org
Subject: Re: io_uring: RWF_NOWAIT support
Date: Sat, 27 Apr 2019 18:05:44 +0200	[thread overview]
Message-ID: <7bcb0eb3-46d1-70e4-1108-dfd9a348bb7c@stbuehler.de> (raw)
In-Reply-To: <740192f8-1d64-9e64-0aea-a73e5d6d4d46@kernel.dk>

Hi,

On 24.04.19 18:09, Jens Axboe wrote:
> On 4/23/19 4:07 PM, Jens Axboe wrote:
>>>> (Also RWF_NOWAIT doesn't work in io_uring right now: IOCB_NOWAIT is
>>>> always removed in the workqueue context, and I don't see an early EAGAIN
>>>> completion).
>>>
>>> That's a case I didn't consider, that you'd want to see EAGAIN after
>>> it's been punted. Once punted, we're not going to return EAGAIN since
>>> we can now block. Not sure how you'd want to handle that any better...
>>
>> I think I grok this one too now - what you're saying is that if the
>> caller has RWF_NOWAIT set, then the EAGAIN should be returned instead of
>> being punted to the workqueue? I totally agree with that, that's a bug.
> 
> This should do it for the EAGAIN part, if the user has set RWF_NOWAIT
> in the sqe, then we don't do the automatic punt to workqueue. We just
> return the EAGAIN instead.
> 
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 58ec6e449fd8..6c0d49c3736b 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -822,7 +822,7 @@ static int io_prep_rw(struct io_kiocb *req, const struct sqe_submit *s,
>  	ret = kiocb_set_rw_flags(kiocb, READ_ONCE(sqe->rw_flags));
>  	if (unlikely(ret))
>  		return ret;
> -	if (force_nonblock) {
> +	if (force_nonblock && !(kiocb->ki_flags & IOCB_NOWAIT)) {
>  		kiocb->ki_flags |= IOCB_NOWAIT;
>  		req->flags |= REQ_F_FORCE_NONBLOCK;
>  	}
> @@ -1828,7 +1828,7 @@ static int io_submit_sqe(struct io_ring_ctx *ctx, struct sqe_submit *s,
>  	}
>  
>  	ret = __io_submit_sqe(ctx, req, s, true);
> -	if (ret == -EAGAIN) {
> +	if (ret == -EAGAIN && (req->flags & REQ_F_FORCE_NONBLOCK)) {
>  		struct io_uring_sqe *sqe_copy;
>  
>  		sqe_copy = kmalloc(sizeof(*sqe_copy), GFP_KERNEL);
> 

I think this breaks other request types.

Only io_prep_rw ever sets REQ_F_FORCE_NONBLOCK, but e.g. IORING_OP_FSYNC
always wants to punt.

Given that REQ_F_FORCE_NONBLOCK wasn't actually used before (never read
afaict), maybe remove it and insert a new flag REQ_F_NOWAIT that will
prevent the punt (inverted semantics!), e.g:

> -	if (ret == -EAGAIN) {
> +	if (ret == -EAGAIN && !(req->flags & REQ_F_NOWAIT)) {

And set this flag in io_prep_rw if RWF_NOWAIT is set.

cheers,
Stefan