The io_uring_enter(2) syscall returns with errno=EINTR when interrupted
by a signal. Retry the syscall in this case.
It's essential to do this in the io_uring_submit_and_wait() case. My
interpretation of the Linux v5.5 io_uring_enter(2) code is that it
shouldn't affect the io_uring_submit() case, but there is no guarantee
this will always be the case. Let's check for -EINTR around both APIs.
Note that the liburing APIs have -errno return values.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
util/fdmon-io_uring.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/util/fdmon-io_uring.c b/util/fdmon-io_uring.c
index b4d6109f20..d5a80ed6fb 100644
--- a/util/fdmon-io_uring.c
+++ b/util/fdmon-io_uring.c
@@ -88,7 +88,10 @@ static struct io_uring_sqe *get_sqe(AioContext *ctx)
}
/* No free sqes left, submit pending sqes first */
- ret = io_uring_submit(ring);
+ do {
+ ret = io_uring_submit(ring);
+ } while (ret == -EINTR);
+
assert(ret > 1);
sqe = io_uring_get_sqe(ring);
assert(sqe);
@@ -282,7 +285,10 @@ static int fdmon_io_uring_wait(AioContext *ctx, AioHandlerList *ready_list,
fill_sq_ring(ctx);
- ret = io_uring_submit_and_wait(&ctx->fdmon_io_uring, wait_nr);
+ do {
+ ret = io_uring_submit_and_wait(&ctx->fdmon_io_uring, wait_nr);
+ } while (ret == -EINTR);
+
assert(ret >= 0);
return process_cq_ring(ctx, ready_list);
--
2.25.1
On 4/8/20 11:11 AM, Stefan Hajnoczi wrote: > The io_uring_enter(2) syscall returns with errno=EINTR when interrupted > by a signal. Retry the syscall in this case. > > It's essential to do this in the io_uring_submit_and_wait() case. My > interpretation of the Linux v5.5 io_uring_enter(2) code is that it > shouldn't affect the io_uring_submit() case, but there is no guarantee > this will always be the case. This is how io_uring_enter() is documented indeed (no EINTR). Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > Let's check for -EINTR around both APIs. > > Note that the liburing APIs have -errno return values. > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > util/fdmon-io_uring.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/util/fdmon-io_uring.c b/util/fdmon-io_uring.c > index b4d6109f20..d5a80ed6fb 100644 > --- a/util/fdmon-io_uring.c > +++ b/util/fdmon-io_uring.c > @@ -88,7 +88,10 @@ static struct io_uring_sqe *get_sqe(AioContext *ctx) > } > > /* No free sqes left, submit pending sqes first */ > - ret = io_uring_submit(ring); > + do { > + ret = io_uring_submit(ring); > + } while (ret == -EINTR); > + > assert(ret > 1); > sqe = io_uring_get_sqe(ring); > assert(sqe); > @@ -282,7 +285,10 @@ static int fdmon_io_uring_wait(AioContext *ctx, AioHandlerList *ready_list, > > fill_sq_ring(ctx); > > - ret = io_uring_submit_and_wait(&ctx->fdmon_io_uring, wait_nr); > + do { > + ret = io_uring_submit_and_wait(&ctx->fdmon_io_uring, wait_nr); > + } while (ret == -EINTR); > + > assert(ret >= 0); > > return process_cq_ring(ctx, ready_list); >
On Wed, Apr 08, 2020 at 10:11:39AM +0100, Stefan Hajnoczi wrote: > The io_uring_enter(2) syscall returns with errno=EINTR when interrupted > by a signal. Retry the syscall in this case. > > It's essential to do this in the io_uring_submit_and_wait() case. My > interpretation of the Linux v5.5 io_uring_enter(2) code is that it > shouldn't affect the io_uring_submit() case, but there is no guarantee > this will always be the case. Let's check for -EINTR around both APIs. > > Note that the liburing APIs have -errno return values. > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > util/fdmon-io_uring.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) Thanks, applied to my block tree: https://github.com/stefanha/qemu/commits/block Stefan
On Wed, Apr 08, 2020 at 10:11:39AM +0100, Stefan Hajnoczi wrote: > The io_uring_enter(2) syscall returns with errno=EINTR when interrupted > by a signal. Retry the syscall in this case. > > It's essential to do this in the io_uring_submit_and_wait() case. My > interpretation of the Linux v5.5 io_uring_enter(2) code is that it > shouldn't affect the io_uring_submit() case, but there is no guarantee > this will always be the case. Let's check for -EINTR around both APIs. > > Note that the liburing APIs have -errno return values. > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > util/fdmon-io_uring.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) The patch LGTM: Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> Not related to this patch, looking at block/io_uring.c, should we retry if the io_uring_submit() fails with EINTR? I mean something like this: diff --git a/block/io_uring.c b/block/io_uring.c index a3142ca989..9765681f7c 100644 --- a/block/io_uring.c +++ b/block/io_uring.c @@ -231,7 +231,7 @@ static int ioq_submit(LuringState *s) trace_luring_io_uring_submit(s, ret); /* Prevent infinite loop if submission is refused */ if (ret <= 0) { - if (ret == -EAGAIN) { + if (ret == -EAGAIN || ret == -EINTR) { continue; } break; Thanks, Stefano
On Wed, Apr 08, 2020 at 12:06:03PM +0200, Stefano Garzarella wrote: > On Wed, Apr 08, 2020 at 10:11:39AM +0100, Stefan Hajnoczi wrote: > > The io_uring_enter(2) syscall returns with errno=EINTR when interrupted > > by a signal. Retry the syscall in this case. > > > > It's essential to do this in the io_uring_submit_and_wait() case. My > > interpretation of the Linux v5.5 io_uring_enter(2) code is that it > > shouldn't affect the io_uring_submit() case, but there is no guarantee > > this will always be the case. Let's check for -EINTR around both APIs. > > > > Note that the liburing APIs have -errno return values. > > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > --- > > util/fdmon-io_uring.c | 10 ++++++++-- > > 1 file changed, 8 insertions(+), 2 deletions(-) > > The patch LGTM: > > Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> > > Not related to this patch, looking at block/io_uring.c, should we retry > if the io_uring_submit() fails with EINTR? > > I mean something like this: > > diff --git a/block/io_uring.c b/block/io_uring.c > index a3142ca989..9765681f7c 100644 > --- a/block/io_uring.c > +++ b/block/io_uring.c > @@ -231,7 +231,7 @@ static int ioq_submit(LuringState *s) > trace_luring_io_uring_submit(s, ret); > /* Prevent infinite loop if submission is refused */ > if (ret <= 0) { > - if (ret == -EAGAIN) { > + if (ret == -EAGAIN || ret == -EINTR) { > continue; > } > break; Thanks! I didn't do that for the reason described in the commit message. Philippe also mentioned that EINTR isn't listed on the io_uring_enter(2) man page, although that is a bug because it does occur with IORING_ENTER_GETEVENTS and min_complete > 0. Feel free to send a separate patch. It's probably not something that needs to go into QEMU 5.0. Stefan
On Thu, Apr 09, 2020 at 03:57:09PM +0100, Stefan Hajnoczi wrote: > On Wed, Apr 08, 2020 at 12:06:03PM +0200, Stefano Garzarella wrote: > > On Wed, Apr 08, 2020 at 10:11:39AM +0100, Stefan Hajnoczi wrote: > > > The io_uring_enter(2) syscall returns with errno=EINTR when interrupted > > > by a signal. Retry the syscall in this case. > > > > > > It's essential to do this in the io_uring_submit_and_wait() case. My > > > interpretation of the Linux v5.5 io_uring_enter(2) code is that it > > > shouldn't affect the io_uring_submit() case, but there is no guarantee > > > this will always be the case. Let's check for -EINTR around both APIs. > > > > > > Note that the liburing APIs have -errno return values. > > > > > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > > > --- > > > util/fdmon-io_uring.c | 10 ++++++++-- > > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > The patch LGTM: > > > > Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> > > > > Not related to this patch, looking at block/io_uring.c, should we retry > > if the io_uring_submit() fails with EINTR? > > > > I mean something like this: > > > > diff --git a/block/io_uring.c b/block/io_uring.c > > index a3142ca989..9765681f7c 100644 > > --- a/block/io_uring.c > > +++ b/block/io_uring.c > > @@ -231,7 +231,7 @@ static int ioq_submit(LuringState *s) > > trace_luring_io_uring_submit(s, ret); > > /* Prevent infinite loop if submission is refused */ > > if (ret <= 0) { > > - if (ret == -EAGAIN) { > > + if (ret == -EAGAIN || ret == -EINTR) { > > continue; > > } > > break; > > Thanks! > > I didn't do that for the reason described in the commit message. Yeah, I agree and it was not well documented in io_uring_enter(2)! > Philippe also mentioned that EINTR isn't listed on the io_uring_enter(2) > man page, FYI there was a discussion on io-uring list just few days ago [1] about that and now it is documented [2]. :-) > although that is a bug because it does occur with > IORING_ENTER_GETEVENTS and min_complete > 0. I think we are safe for now. IIUC io_uring_submit() sets min_complete to 0 and IORING_ENTER_GETEVENTS is set only if IOPOLL is enabled (or min_complete > 0), but it is not our case (for now). > > Feel free to send a separate patch. It's probably not something that > needs to go into QEMU 5.0. Sure, I'll send it after the freeze. Thanks, Stefano [1] https://lore.kernel.org/io-uring/43b339d3dc0c4b6ab15652faf12afa30@cert.org/t/#u [2] https://github.com/axboe/liburing/commit/344355ec6619de8f4e64584c9736530b5346e4f4
© 2016 - 2024 Red Hat, Inc.