* [PATCH for-5.0] aio-posix: signal-proof fdmon-io_uring
@ 2020-04-08 9:11 Stefan Hajnoczi
2020-04-08 10:06 ` Stefano Garzarella
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Stefan Hajnoczi @ 2020-04-08 9:11 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, qemu-block, Julia Suvorova, Stefan Hajnoczi,
Paolo Bonzini, sgarzare
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH for-5.0] aio-posix: signal-proof fdmon-io_uring
2020-04-08 9:11 [PATCH for-5.0] aio-posix: signal-proof fdmon-io_uring Stefan Hajnoczi
@ 2020-04-08 10:06 ` Stefano Garzarella
2020-04-09 14:57 ` Stefan Hajnoczi
2020-04-08 11:47 ` Philippe Mathieu-Daudé
2020-04-09 14:55 ` Stefan Hajnoczi
2 siblings, 1 reply; 6+ messages in thread
From: Stefano Garzarella @ 2020-04-08 10:06 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Fam Zheng, Paolo Bonzini, Julia Suvorova, qemu-devel, qemu-block
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH for-5.0] aio-posix: signal-proof fdmon-io_uring
2020-04-08 9:11 [PATCH for-5.0] aio-posix: signal-proof fdmon-io_uring Stefan Hajnoczi
2020-04-08 10:06 ` Stefano Garzarella
@ 2020-04-08 11:47 ` Philippe Mathieu-Daudé
2020-04-09 14:55 ` Stefan Hajnoczi
2 siblings, 0 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-04-08 11:47 UTC (permalink / raw)
To: Stefan Hajnoczi, qemu-devel
Cc: Fam Zheng, Paolo Bonzini, Julia Suvorova, qemu-block, sgarzare
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);
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH for-5.0] aio-posix: signal-proof fdmon-io_uring
2020-04-08 9:11 [PATCH for-5.0] aio-posix: signal-proof fdmon-io_uring Stefan Hajnoczi
2020-04-08 10:06 ` Stefano Garzarella
2020-04-08 11:47 ` Philippe Mathieu-Daudé
@ 2020-04-09 14:55 ` Stefan Hajnoczi
2 siblings, 0 replies; 6+ messages in thread
From: Stefan Hajnoczi @ 2020-04-09 14:55 UTC (permalink / raw)
To: qemu-devel; +Cc: Fam Zheng, Paolo Bonzini, Julia Suvorova, qemu-block, sgarzare
[-- Attachment #1: Type: text/plain, Size: 804 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH for-5.0] aio-posix: signal-proof fdmon-io_uring
2020-04-08 10:06 ` Stefano Garzarella
@ 2020-04-09 14:57 ` Stefan Hajnoczi
2020-04-09 15:31 ` Stefano Garzarella
0 siblings, 1 reply; 6+ messages in thread
From: Stefan Hajnoczi @ 2020-04-09 14:57 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Fam Zheng, Paolo Bonzini, Julia Suvorova, qemu-devel, qemu-block
[-- Attachment #1: Type: text/plain, Size: 1938 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH for-5.0] aio-posix: signal-proof fdmon-io_uring
2020-04-09 14:57 ` Stefan Hajnoczi
@ 2020-04-09 15:31 ` Stefano Garzarella
0 siblings, 0 replies; 6+ messages in thread
From: Stefano Garzarella @ 2020-04-09 15:31 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Fam Zheng, qemu-block, Julia Suvorova, qemu-devel, Paolo Bonzini, philmd
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-04-09 15:38 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-08 9:11 [PATCH for-5.0] aio-posix: signal-proof fdmon-io_uring Stefan Hajnoczi
2020-04-08 10:06 ` Stefano Garzarella
2020-04-09 14:57 ` Stefan Hajnoczi
2020-04-09 15:31 ` Stefano Garzarella
2020-04-08 11:47 ` Philippe Mathieu-Daudé
2020-04-09 14:55 ` Stefan Hajnoczi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).