Re: [PATCH 1/1] io_uring/rw: forbid multishot async reads

[Pavel Begunkov <asml.silence@gmail.com>]

On 2/17/25 15:37, Jens Axboe wrote:
> On 2/17/25 7:08 AM, Pavel Begunkov wrote:
>> On 2/17/25 13:58, Jens Axboe wrote:
>>> On 2/17/25 6:37 AM, Pavel Begunkov wrote:
>>>
>>> The kiocb semantics of ki_complete == NULL -> sync kiocb is also odd,
>>
>> That's what is_sync_kiocb() does. Would be cleaner to use
>> init_sync_kiocb(), but there is a larger chance to do sth
>> wrong as it's reinitialises it entirely.
> 
> Sorry if that wasn't clear, yeah I do realize this is what
> is_sync_kiocb() checks. I do agree that manually clearing is saner.
> 
>>> but probably fine for this case as read mshot strictly deals with
>>> pollable files. Otherwise you'd just be blocking off this issue,
>>> regardless of whether or not IOCB_NOWAIT is set.
>>>
>>> In any case, it'd be much nicer to container this in io_read_mshot()
>>> where it arguably belongs, rather than sprinkle it in __io_read().
>>> Possible?
>>
>> That's what I tried first, but __io_read() -> io_rw_init_file()
>> reinitialises it, so I don't see any good way without some
>> broader refactoring.
> 
> Would it be bad? The only reason the kiocb init part is in there is
> because of the ->iopoll() check, that could still be there with the rest
> of the init going into normal prep (as it arguably should).
> 
> Something like the below, totally untested...

fwiw, turns out we can't move all of it like in the diff as
ki_flags setup and checks depend on having the file set.

> diff --git a/io_uring/rw.c b/io_uring/rw.c
> index 16f12f94943f..f8dd9a9fe9ca 100644
> --- a/io_uring/rw.c
> +++ b/io_uring/rw.c
> @@ -264,6 +264,9 @@ static int io_prep_rw_pi(struct io_kiocb *req, struct io_rw *rw, int ddir,
>   	return ret;
>   }
>   
> +static void io_complete_rw(struct kiocb *kiocb, long res);
> +static void io_complete_rw_iopoll(struct kiocb *kiocb, long res);
> +
>   static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe,
>   		      int ddir, bool do_import)
>   {
> @@ -288,6 +291,18 @@ static int io_prep_rw(struct io_kiocb *req, const struct io_uring_sqe *sqe,
>   	}
>   	rw->kiocb.dio_complete = NULL;
>   	rw->kiocb.ki_flags = 0;
> +	if (req->ctx->flags & IORING_SETUP_IOPOLL) {
> +		if (!(rw->kiocb.ki_flags & IOCB_DIRECT))
> +			return -EOPNOTSUPP;
> +
> +		rw->kiocb.private = NULL;
> +		rw->kiocb.ki_flags |= IOCB_HIPRI;
> +		rw->kiocb.ki_complete = io_complete_rw_iopoll;
> +	} else {
> +		if (rw->kiocb.ki_flags & IOCB_HIPRI)
> +			return -EINVAL;
> +		rw->kiocb.ki_complete = io_complete_rw;
> +	}
>   
>   	rw->addr = READ_ONCE(sqe->addr);
>   	rw->len = READ_ONCE(sqe->len);
> @@ -810,23 +825,15 @@ static int io_rw_init_file(struct io_kiocb *req, fmode_t mode, int rw_type)
>   	    ((file->f_flags & O_NONBLOCK && !(req->flags & REQ_F_SUPPORT_NOWAIT))))
>   		req->flags |= REQ_F_NOWAIT;
>   
> -	if (ctx->flags & IORING_SETUP_IOPOLL) {
> -		if (!(kiocb->ki_flags & IOCB_DIRECT) || !file->f_op->iopoll)
> +	if (kiocb->ki_flags & IOCB_HIPRI) {
> +		if (!file->f_op->iopoll)
>   			return -EOPNOTSUPP;
> -
> -		kiocb->private = NULL;
> -		kiocb->ki_flags |= IOCB_HIPRI;
> -		kiocb->ki_complete = io_complete_rw_iopoll;
>   		req->iopoll_completed = 0;
>   		if (ctx->flags & IORING_SETUP_HYBRID_IOPOLL) {
>   			/* make sure every req only blocks once*/
>   			req->flags &= ~REQ_F_IOPOLL_STATE;
>   			req->iopoll_start = ktime_get_ns();
>   		}
> -	} else {
> -		if (kiocb->ki_flags & IOCB_HIPRI)
> -			return -EINVAL;
> -		kiocb->ki_complete = io_complete_rw;
>   	}
>   
>   	if (req->flags & REQ_F_HAS_METADATA) {
> 

-- 
Pavel Begunkov