| Message ID | f168b4f24181942f3614dd8ff648221736f572e6.1652433740.git.asml.silence@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | io_uring: avoid iowq again trap | expand |
On 5/13/22 4:24 AM, Pavel Begunkov wrote: > If an opcode handler semi-reliably returns -EAGAIN, io_wq_submit_work() > might continue busily hammer the same handler over and over again, which > is not ideal. The -EAGAIN handling in question was put there only for > IOPOLL, so restrict it to IOPOLL mode only. Looks good, needs: Fixes: 90fa02883f06 ("io_uring: implement async hybrid mode for pollable requests") unless I'm mistaken.
On 5/13/22 13:31, Jens Axboe wrote: > On 5/13/22 4:24 AM, Pavel Begunkov wrote: >> If an opcode handler semi-reliably returns -EAGAIN, io_wq_submit_work() >> might continue busily hammer the same handler over and over again, which >> is not ideal. The -EAGAIN handling in question was put there only for >> IOPOLL, so restrict it to IOPOLL mode only. > > Looks good, needs: > > Fixes: 90fa02883f06 ("io_uring: implement async hybrid mode for pollable requests") > > unless I'm mistaken. It's probably more of Fixes: def596e9557c9 ("io_uring: support for IO polling")
On 5/13/22 6:49 AM, Pavel Begunkov wrote: > On 5/13/22 13:31, Jens Axboe wrote: >> On 5/13/22 4:24 AM, Pavel Begunkov wrote: >>> If an opcode handler semi-reliably returns -EAGAIN, io_wq_submit_work() >>> might continue busily hammer the same handler over and over again, which >>> is not ideal. The -EAGAIN handling in question was put there only for >>> IOPOLL, so restrict it to IOPOLL mode only. >> >> Looks good, needs: >> >> Fixes: 90fa02883f06 ("io_uring: implement async hybrid mode for pollable requests") >> >> unless I'm mistaken. > > It's probably more of > > Fixes: def596e9557c9 ("io_uring: support for IO polling") Yes, I think you are right and it goes back further. I'll get it queued up, thanks!
On Fri, 13 May 2022 11:24:56 +0100, Pavel Begunkov wrote: > If an opcode handler semi-reliably returns -EAGAIN, io_wq_submit_work() > might continue busily hammer the same handler over and over again, which > is not ideal. The -EAGAIN handling in question was put there only for > IOPOLL, so restrict it to IOPOLL mode only. > > Applied, thanks! [1/1] io_uring: avoid iowq again trap (no commit info) Best regards,
On 5/13/22 18:24, Pavel Begunkov wrote: > If an opcode handler semi-reliably returns -EAGAIN, io_wq_submit_work() Hi Pavel, When would it return -EAGAIN in non-IOPOLL mode? > might continue busily hammer the same handler over and over again, which > is not ideal. The -EAGAIN handling in question was put there only for > IOPOLL, so restrict it to IOPOLL mode only. > > Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> > --- > fs/io_uring.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/io_uring.c b/fs/io_uring.c > index e01f595f5b7d..3af1905efc78 100644 > --- a/fs/io_uring.c > +++ b/fs/io_uring.c > @@ -7319,6 +7319,8 @@ static void io_wq_submit_work(struct io_wq_work *work) > * wait for request slots on the block side. > */ > if (!needs_poll) { > + if (!(req->ctx->flags & IORING_SETUP_IOPOLL)) > + break; > cond_resched(); > continue; > }
On 5/15/22 08:31, Hao Xu wrote: > On 5/13/22 18:24, Pavel Begunkov wrote: >> If an opcode handler semi-reliably returns -EAGAIN, io_wq_submit_work() > > Hi Pavel, > When would it return -EAGAIN in non-IOPOLL mode? I didn't see it in the wild but stumbled upon while preparing some future patches. I hope it's not a real issue, but it's better to not leave a way for some driver/etc. to abuse it. >> might continue busily hammer the same handler over and over again, which >> is not ideal. The -EAGAIN handling in question was put there only for >> IOPOLL, so restrict it to IOPOLL mode only. >> >> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> >> --- >> fs/io_uring.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/fs/io_uring.c b/fs/io_uring.c >> index e01f595f5b7d..3af1905efc78 100644 >> --- a/fs/io_uring.c >> +++ b/fs/io_uring.c >> @@ -7319,6 +7319,8 @@ static void io_wq_submit_work(struct io_wq_work *work) >> * wait for request slots on the block side. >> */ >> if (!needs_poll) { >> + if (!(req->ctx->flags & IORING_SETUP_IOPOLL)) >> + break; >> cond_resched(); >> continue; >> } >
On 5/15/22 22:21, Pavel Begunkov wrote: > On 5/15/22 08:31, Hao Xu wrote: >> On 5/13/22 18:24, Pavel Begunkov wrote: >>> If an opcode handler semi-reliably returns -EAGAIN, io_wq_submit_work() >> >> Hi Pavel, >> When would it return -EAGAIN in non-IOPOLL mode? > > I didn't see it in the wild but stumbled upon while preparing some > future patches. I hope it's not a real issue, but it's better to not > leave a way for some driver/etc. to abuse it. > > Gotcha, thanks.
diff --git a/fs/io_uring.c b/fs/io_uring.c index e01f595f5b7d..3af1905efc78 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -7319,6 +7319,8 @@ static void io_wq_submit_work(struct io_wq_work *work) * wait for request slots on the block side. */ if (!needs_poll) { + if (!(req->ctx->flags & IORING_SETUP_IOPOLL)) + break; cond_resched(); continue; }
If an opcode handler semi-reliably returns -EAGAIN, io_wq_submit_work() might continue busily hammer the same handler over and over again, which is not ideal. The -EAGAIN handling in question was put there only for IOPOLL, so restrict it to IOPOLL mode only. Signed-off-by: Pavel Begunkov <asml.silence@gmail.com> --- fs/io_uring.c | 2 ++ 1 file changed, 2 insertions(+)