On 3/15/24 5:19 PM, Pavel Begunkov wrote: > On 3/15/24 23:13, Pavel Begunkov wrote: >> On 3/15/24 23:09, Pavel Begunkov wrote: >>> On 3/15/24 22:48, Jens Axboe wrote: >>>> If we get a request with IOSQE_ASYNC set, then we first run the prep >>>> async handlers. But if we then fail setting it up and want to post >>>> a CQE with -EINVAL, we use ->done_io. This was previously guarded with >>>> REQ_F_PARTIAL_IO, and the normal setup handlers do set it up before any >>>> potential errors, but we need to cover the async setup too. >>> >>> You can hit io_req_defer_failed() { opdef->fail(); } >>> off of an early submission failure path where def->prep has >>> not yet been called, I don't think the patch will fix the >>> problem. >>> >>> ->fail() handlers are fragile, maybe we should skip them >>> if def->prep() wasn't called. Not even compile tested: >>> >>> >>> diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c >>> index 846d67a9c72e..56eed1490571 100644 >>> --- a/io_uring/io_uring.c >>> +++ b/io_uring/io_uring.c > [...] >>> 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,8 +2249,15 @@ 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)) { >>> +fail: > > Obvious the diff is crap, but still bugging me enough to write > that the label should've been one line below, otherwise we'd > flag after ->prep as well. It certainly needs testing :-) We can go either way - patch up the net thing, or do a proper EARLY_FAIL and hopefully not have to worry about it again. Do you want to clean it up, test it, and send it out? -- Jens Axboe