| Message ID | ed30899d479bf40c6d386cac5d9401892836c3b5.1573077364.git.asml.silence@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Inline sqe_submit | expand |
This one changes behaviour a bit. If we haven't been able to allocate req before, it would post an completion event with -EAGAIN. Now it will break imidiately without consuming sqe. So the user will see, that 0 sqes was submitted/consumed. Is that ok or we need to do something about it? On 07/11/2019 01:00, Pavel Begunkov wrote: > Let io_submit_sqes() to allocate io_kiocb before fetching an sqe. > > Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> > --- > fs/io_uring.c | 25 ++++++++++++------------- > 1 file changed, 12 insertions(+), 13 deletions(-) > > diff --git a/fs/io_uring.c b/fs/io_uring.c > index 6524898831e0..0289bb3cc697 100644 > --- a/fs/io_uring.c > +++ b/fs/io_uring.c > @@ -2551,30 +2551,23 @@ static int io_queue_link_head(struct io_ring_ctx *ctx, struct io_kiocb *req, > > #define SQE_VALID_FLAGS (IOSQE_FIXED_FILE|IOSQE_IO_DRAIN|IOSQE_IO_LINK) > > -static void io_submit_sqe(struct io_ring_ctx *ctx, struct sqe_submit *s, > - struct io_submit_state *state, struct io_kiocb **link) > +static void io_submit_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req, > + struct sqe_submit *s, struct io_submit_state *state, > + struct io_kiocb **link) > { > struct io_uring_sqe *sqe_copy; > - struct io_kiocb *req; > int ret; > > /* enforce forwards compatibility on users */ > if (unlikely(s->sqe->flags & ~SQE_VALID_FLAGS)) { > ret = -EINVAL; > - goto err; > - } > - > - req = io_get_req(ctx, state); > - if (unlikely(!req)) { > - ret = -EAGAIN; > - goto err; > + goto err_req; > } > > ret = io_req_set_file(ctx, s, state, req); > if (unlikely(ret)) { > err_req: > io_free_req(req, NULL); > -err: > io_cqring_add_event(ctx, s->sqe->user_data, ret); > return; > } > @@ -2710,9 +2703,15 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr, > > for (i = 0; i < nr; i++) { > struct sqe_submit s; > + struct io_kiocb *req; > > - if (!io_get_sqring(ctx, &s)) > + req = io_get_req(ctx, statep); > + if (unlikely(!req)) > break; > + if (!io_get_sqring(ctx, &s)) { > + __io_free_req(req); > + break; > + } > > if (io_sqe_needs_user(s.sqe) && !*mm) { > mm_fault = mm_fault || !mmget_not_zero(ctx->sqo_mm); > @@ -2740,7 +2739,7 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr, > s.in_async = async; > s.needs_fixed_file = async; > trace_io_uring_submit_sqe(ctx, s.sqe->user_data, true, async); > - io_submit_sqe(ctx, &s, statep, &link); > + io_submit_sqe(ctx, req, &s, statep, &link); > submitted++; > > /* >
On 11/6/19 3:05 PM, Pavel Begunkov wrote: > This one changes behaviour a bit. If we haven't been able to allocate > req before, it would post an completion event with -EAGAIN. Now it will > break imidiately without consuming sqe. So the user will see, that 0 > sqes was submitted/consumed. > > Is that ok or we need to do something about it? At the very least we need to return -EAGAIN to the application. So something ala: return submitted ? submitted : ret; where ret is 0 or -EAGAIN if we failed to get a request.
On 07/11/2019 01:18, Jens Axboe wrote: > On 11/6/19 3:05 PM, Pavel Begunkov wrote: >> This one changes behaviour a bit. If we haven't been able to allocate >> req before, it would post an completion event with -EAGAIN. Now it will >> break imidiately without consuming sqe. So the user will see, that 0 >> sqes was submitted/consumed. >> >> Is that ok or we need to do something about it? > > At the very least we need to return -EAGAIN to the application. So > something ala: > > return submitted ? submitted : ret; > > where ret is 0 or -EAGAIN if we failed to get a request. > This one is even better, as the old one didn't care about links
diff --git a/fs/io_uring.c b/fs/io_uring.c index 6524898831e0..0289bb3cc697 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -2551,30 +2551,23 @@ static int io_queue_link_head(struct io_ring_ctx *ctx, struct io_kiocb *req, #define SQE_VALID_FLAGS (IOSQE_FIXED_FILE|IOSQE_IO_DRAIN|IOSQE_IO_LINK) -static void io_submit_sqe(struct io_ring_ctx *ctx, struct sqe_submit *s, - struct io_submit_state *state, struct io_kiocb **link) +static void io_submit_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req, + struct sqe_submit *s, struct io_submit_state *state, + struct io_kiocb **link) { struct io_uring_sqe *sqe_copy; - struct io_kiocb *req; int ret; /* enforce forwards compatibility on users */ if (unlikely(s->sqe->flags & ~SQE_VALID_FLAGS)) { ret = -EINVAL; - goto err; - } - - req = io_get_req(ctx, state); - if (unlikely(!req)) { - ret = -EAGAIN; - goto err; + goto err_req; } ret = io_req_set_file(ctx, s, state, req); if (unlikely(ret)) { err_req: io_free_req(req, NULL); -err: io_cqring_add_event(ctx, s->sqe->user_data, ret); return; } @@ -2710,9 +2703,15 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr, for (i = 0; i < nr; i++) { struct sqe_submit s; + struct io_kiocb *req; - if (!io_get_sqring(ctx, &s)) + req = io_get_req(ctx, statep); + if (unlikely(!req)) break; + if (!io_get_sqring(ctx, &s)) { + __io_free_req(req); + break; + } if (io_sqe_needs_user(s.sqe) && !*mm) { mm_fault = mm_fault || !mmget_not_zero(ctx->sqo_mm); @@ -2740,7 +2739,7 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr, s.in_async = async; s.needs_fixed_file = async; trace_io_uring_submit_sqe(ctx, s.sqe->user_data, true, async); - io_submit_sqe(ctx, &s, statep, &link); + io_submit_sqe(ctx, req, &s, statep, &link); submitted++; /*
Let io_submit_sqes() to allocate io_kiocb before fetching an sqe. Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> --- fs/io_uring.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-)