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