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 <address@hidden>
> > > ---
> > >  util/fdmon-io_uring.c | 10 ++++++++--
> > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > The patch LGTM:
> > 
> > Reviewed-by: Stefano Garzarella <address@hidden>
> > 
> > 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/address@hidden/t/#u
[2] 
https://github.com/axboe/liburing/commit/344355ec6619de8f4e64584c9736530b5346e4f4