[PATCH for-5.0] aio-posix: signal-proof fdmon-io_uring

[PATCH for-5.0] aio-posix: signal-proof fdmon-io_uring

[Stefan Hajnoczi]

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

Re: [PATCH for-5.0] aio-posix: signal-proof fdmon-io_uring

[Stefano Garzarella]

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

Re: [PATCH for-5.0] aio-posix: signal-proof fdmon-io_uring

[Stefan Hajnoczi]

[-- 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 --]

Re: [PATCH for-5.0] aio-posix: signal-proof fdmon-io_uring

[Stefano Garzarella]

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

Re: [PATCH for-5.0] aio-posix: signal-proof fdmon-io_uring

[Philippe Mathieu-Daudé]

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);
> 

Re: [PATCH for-5.0] aio-posix: signal-proof fdmon-io_uring

[Stefan Hajnoczi]

[-- 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 --]