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

[Jens Axboe <axboe@kernel.dk>]

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...

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) {

-- 
Jens Axboe