Re: [PATCH v2] io_uring/net: ensure async prep handlers always initialize ->done_io

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

On 3/16/24 23:58, Jens Axboe wrote:
> On 3/16/24 11:42 AM, Pavel Begunkov wrote:
>> On 3/16/24 17:01, Jens Axboe wrote:
>>> On 3/16/24 10:57 AM, Pavel Begunkov wrote:
>>>> On 3/16/24 16:51, Jens Axboe wrote:
>>>>> On 3/16/24 10:46 AM, Pavel Begunkov wrote:
>>>>>> On 3/16/24 16:42, Jens Axboe wrote:
>>>>>>> On 3/16/24 10:36 AM, Pavel Begunkov wrote:
>>>>>>>> On 3/16/24 16:36, Jens Axboe wrote:
...
>>>>>>>>
>>>>>>>> It should only set REQ_F_EARLY_FAIL if we fail
>>>>>>>> _before_ prep is called
>>>>>>>
>>>>>>> I did try both ways, fails if we just have:
>>>>>>
>>>>>> Ok, but the point is that the sendzc's ->fail doesn't
>>>>>> need to be called unless you've done ->prep first.
>>>>>
>>>>> But it fails, not sure how else to say it.
>>>>
>>>> liburing tests? Which test case? If so, it should be another
>>>
>>> Like I mentioned earlier, it's send zc and it's failing the test case
>>> for that. test/send-zerocopy.t.
>>>
>>>> bug. REQ_F_NEED_CLEANUP is only set by opcodes, if a request is
>>>> terminated before ->prep is called, it means it never entered
>>>> any of the opdef callbacks and have never seen any of net.c
>>>> code, so there should be no REQ_F_NEED_CLEANUP, and so
>>>> io_sendrecv_fail() wouldn't try to set F_MORE. I don't know
>>>> what's wrong.
>>>
>>> Feel free to take a look! I do like the simplicity of the early error
>>> flag.
>>
>> ./send-zerocopy.t works fine
> 
> Huh, I wonder what I messed up. But:

My blind guess would be it called ->prep(), which is assumingly
failed, but then there was no ->fail() following it. BTW, we might
want to loose up that case, similar to mshots sendzc might decide
not to post notifications for any reason and the user should always
check F_MORE first.

>> @@ -2250,7 +2249,13 @@ static inline int io_submit_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
>>       int ret;
>>   
>>       ret = io_init_req(ctx, req, sqe);
>> -    if (unlikely(ret))
>> +    if (unlikely(ret)) {
>> +        req->flags |= REQ_F_UNPREPPED_FAIL;
>> +        return io_submit_fail_init(sqe, req, ret);
>> +    }
>> +
>> +    ret = def->prep(req, sqe);
>> +    if (ret)
>>           return io_submit_fail_init(sqe, req, ret);
> 
> this obviously won't compile, assuming this is not the one you ran.

Urgh, yeah, some left unstaged



diff --git a/include/linux/io_uring_types.h b/include/linux/io_uring_types.h
index ea7e5488b3be..de3a2c67c4a7 100644
--- a/include/linux/io_uring_types.h
+++ b/include/linux/io_uring_types.h
@@ -478,6 +478,7 @@ enum {
  	REQ_F_CAN_POLL_BIT,
  	REQ_F_BL_EMPTY_BIT,
  	REQ_F_BL_NO_RECYCLE_BIT,
+	REQ_F_UNPREPPED_FAIL_BIT,
  
  	/* not a real bit, just to check we're not overflowing the space */
  	__REQ_F_LAST_BIT,
@@ -556,6 +557,8 @@ enum {
  	REQ_F_BL_EMPTY		= IO_REQ_FLAG(REQ_F_BL_EMPTY_BIT),
  	/* don't recycle provided buffers for this request */
  	REQ_F_BL_NO_RECYCLE	= IO_REQ_FLAG(REQ_F_BL_NO_RECYCLE_BIT),
+
+	REQ_F_UNPREPPED_FAIL	= IO_REQ_FLAG(REQ_F_UNPREPPED_FAIL_BIT),
  };
  
  typedef void (*io_req_tw_func_t)(struct io_kiocb *req, struct io_tw_state *ts);
diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 846d67a9c72e..1231f8c53014 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -993,7 +993,7 @@ void io_req_defer_failed(struct io_kiocb *req, s32 res)
  
  	req_set_fail(req);
  	io_req_set_res(req, res, io_put_kbuf(req, IO_URING_F_UNLOCKED));
-	if (def->fail)
+	if (!(req->flags & REQ_F_UNPREPPED_FAIL) && def->fail)
  		def->fail(req);
  	io_req_complete_defer(req);
  }
@@ -2201,8 +2201,7 @@ static int io_init_req(struct io_ring_ctx *ctx, struct io_kiocb *req,
  		}
  		req->flags |= REQ_F_CREDS;
  	}
-
-	return def->prep(req, sqe);
+	return 0;
  }
  
  static __cold int io_submit_fail_init(const struct io_uring_sqe *sqe,
@@ -2250,7 +2249,13 @@ static inline int io_submit_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
  	int ret;
  
  	ret = io_init_req(ctx, req, sqe);
-	if (unlikely(ret))
+	if (unlikely(ret)) {
+		req->flags |= REQ_F_UNPREPPED_FAIL;
+		return io_submit_fail_init(sqe, req, ret);
+	}
+
+	ret = io_issue_defs[req->opcode].prep(req, sqe);
+	if (ret)
  		return io_submit_fail_init(sqe, req, ret);
  
  	trace_io_uring_submit_req(req);


-- 
Pavel Begunkov