Fix block I/O hang.

PATCH In review Gerd Hoffmann 3 participants 13 messages Nov 11, 2008 – Nov 13, 2008

lore

Share & tools

lore

Gerd Hoffmann

author

2008-11-11 16:36 UTC #0 lore raw

PATCH 1/1 Fix block I/O hang.

Hi,

$subject says all.

please apply,
Gerd

Quoted 1 line

From db0f41a843004157fd1181082716f83879f518ca Mon Sep 17 00:00:00 2001

From: Gerd Hoffmann <[email protected]>
Date: Tue, 11 Nov 2008 17:31:55 +0100
Subject: [PATCH] Fix qemu hang in block layer.

Without s->rfd being non-blocking qemu hangs in posix_aio_read() due to the read() syscall blocking.

Signed-off-by Gerd Hoffmann <[email protected]>

---
 block-raw-posix.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

block-raw-posix.c +1 -0

index c06e38d..a08a773 100644
@@ -591,6 +591,7 @@ static int posix_aio_init(void)
     s->rfd = fds[0];
     s->wfd = fds[1];
 
+    fcntl(s->rfd, F_SETFL, O_NONBLOCK);
     fcntl(s->wfd, F_SETFL, O_NONBLOCK);
 
     qemu_aio_set_fd_handler(s->rfd, posix_aio_read, NULL, posix_aio_flush, s);

Signature · 1 line

1.5.6.5

Anthony Liguori

2008-11-11 16:49 UTC #1 lore raw

Quoted 7 lines · Gerd Hoffmann

Hi,
$subject says all.
please apply,
Gerd

Under what circumstances? posix_aio_read() is only invoked from a select callback. This means there should be data available to be read.

Regards,

Anthony Liguori

Gerd Hoffmann

author

2008-11-11 17:48 UTC #2 lore raw

Quoted 10 lines · Anthony Liguori

Quoted 6 lines · Gerd Hoffmann
Hi,
$subject says all.
please apply,
Gerd
Under what circumstances? posix_aio_read() is only invoked from a select callback. This means there should be data available to be read.

Hmm, looked at the loop again a bit closer.

First, the check for errno == EAGAIN is superfluous when the filehandle isn't in non-blocking mode. It will not happen.

Second, the check for errno == EINTR is superfluous too because the loop will retry the read() syscall anyway for *any* error.

Third, when called from a select callback it shouldn't block indeed. It does though for me now and then when booting xen guests (with a big stack of xenner patches). Doesn't reproduce reliable though. Sprinkled in a printk (with rfd being non-blocking) and got a EAGAIN once, so it got called with rfd not having data.

cheers,
Gerd

Anthony Liguori

2008-11-11 17:55 UTC #3 lore raw

Quoted 15 lines · Gerd Hoffmann

Quoted 4 lines · Anthony Liguori
Quoted 2 lines · Gerd Hoffmann
Hi,
Quoted 8 lines
Quoted 5 lines
$subject says all.
please apply,
Gerd
Under what circumstances? posix_aio_read() is only invoked from a select callback. This means there should be data available to be read.

Quoted 8 lines

Hmm, looked at the loop again a bit closer.
First, the check for errno == EAGAIN is superfluous when the filehandle isn't in non-blocking mode. It will not happen.
Second, the check for errno == EINTR is superfluous too because the loop will retry the read() syscall anyway for *any* error.

Yes, this all is. I think the intention was to make it non-blocking so that the full queue could be drained to avoid superfluous calls to posix_aio_read() since it handles all outstanding ops at once. However,

Quoted 6 lines

Third, when called from a select callback it shouldn't block indeed. It does though for me now and then when booting xen guests (with a big stack of xenner patches). Doesn't reproduce reliable though. Sprinkled in a printk (with rfd being non-blocking) and got a EAGAIN once, so it got called with rfd not having data.

I think it would be good to root cause this. AFAICT, it shouldn't block.

Regards,

Anthony Liguori

Quoted 5 lines

cheers,
Gerd

Gerd Hoffmann

author

2008-11-11 20:51 UTC #4 lore raw

Quoted 3 lines · Anthony Liguori

Yes, this all is. I think the intention was to make it non-blocking so that the full queue could be drained to avoid superfluous calls to posix_aio_read() since it handles all outstanding ops at once.

That is what I concluded after a brief look, without realizing that the loop doesn't actually do that (yet?), thus went for the "oops, this fd must be in non-blocking mode" fix.

Quoted 9 lines

However,
Quoted 5 lines
Third, when called from a select callback it shouldn't block indeed. It does though for me now and then when booting xen guests (with a big stack of xenner patches). Doesn't reproduce reliable though. Sprinkled in a printk (with rfd being non-blocking) and got a EAGAIN once, so it got called with rfd not having data.
I think it would be good to root cause this. AFAICT, it shouldn't block.

Yes, something is fishy here. Will try to pin it. Ideas where to look are welcome in case you have any ;)

cheers,
Gerd

Anthony Liguori

2008-11-11 21:07 UTC #5 lore raw

Quoted 10 lines · Gerd Hoffmann

Quoted 10 lines
However,
Quoted 6 lines
Third, when called from a select callback it shouldn't block indeed. It does though for me now and then when booting xen guests (with a big stack of xenner patches). Doesn't reproduce reliable though. Sprinkled in a printk (with rfd being non-blocking) and got a EAGAIN once, so it got called with rfd not having data.

Quoted 6 lines

Quoted 2 lines
I think it would be good to root cause this. AFAICT, it shouldn't block.
Yes, something is fishy here. Will try to pin it. Ideas where to look are welcome in case you have any ;)

Are you doing something funky with threads in xenner?

Regards,

Anthony Liguori

Quoted 6 lines

cheers,
Gerd

Gerd Hoffmann

author

2008-11-11 21:34 UTC #6 lore raw

Quoted 17 lines · Anthony Liguori

Quoted 16 lines · Gerd Hoffmann
Quoted 15 lines
However,
Quoted 8 lines
Third, when called from a select callback it shouldn't block indeed. It
does though for me now and then when booting xen guests (with a big stack of xenner patches). Doesn't reproduce reliable though. Sprinkled
in a printk (with rfd being non-blocking) and got a EAGAIN once, so it got called with rfd not having data.
I think it would be good to root cause this. AFAICT, it shouldn't block.

Quoted 5 lines

Quoted 3 lines
Yes, something is fishy here. Will try to pin it. Ideas where to look are welcome in case you have any ;)
Are you doing something funky with threads in xenner?

No, I don't create any.

cheers,
Gerd

Johannes Stezenbach

2008-11-12 13:49 UTC #7 lore raw

In reply to Gerd Hoffmann

Hi,

Quoted 4 lines · Gerd Hoffmann

Quoted 3 lines · Anthony Liguori
Under what circumstances? posix_aio_read() is only invoked from a select callback. This means there should be data available to be read.

...

Quoted 6 lines

Third, when called from a select callback it shouldn't block indeed. It does though for me now and then when booting xen guests (with a big stack of xenner patches). Doesn't reproduce reliable though. Sprinkled in a printk (with rfd being non-blocking) and got a EAGAIN once, so it got called with rfd not having data.

I don't know what kind of fd you're talking about, but the Linux select man page says:

BUGS
Under Linux, select() may report a socket file descriptor as "ready for reading", while nevertheless a sub‐ sequent read blocks. This could for example happen when data has arrived but upon examination has wrong
checksum and is discarded. There may be other circumstances in which
a file descriptor is spuriously reported as ready. Thus it may be safer to use O_NONBLOCK on sockets that should not block.

HTH
Johannes

Anthony Liguori

2008-11-12 14:58 UTC #8 lore raw

Quoted 18 lines · Johannes Stezenbach

Hi,
Quoted 5 lines · Gerd Hoffmann
Quoted 3 lines · Anthony Liguori
Under what circumstances? posix_aio_read() is only invoked from a select callback. This means there should be data available to be read.
...
Quoted 6 lines
Third, when called from a select callback it shouldn't block indeed. It does though for me now and then when booting xen guests (with a big stack of xenner patches). Doesn't reproduce reliable though. Sprinkled in a printk (with rfd being non-blocking) and got a EAGAIN once, so it got called with rfd not having data.

Quoted 3 lines

I don't know what kind of fd you're talking about, but the Linux select man page says:

It's a pipe. The socket issue has bit me a number of times in the past though :-)

Regards,

Anthony Liguori

Gerd Hoffmann

author

2008-11-12 15:01 UTC #9 lore raw

In reply to Johannes Stezenbach

Quoted 2 lines · Johannes Stezenbach

I don't know what kind of fd you're talking about, but the Linux select man page says:

pipe.

Quoted 7 lines

BUGS
Under Linux, select() may report a socket file descriptor as "ready for reading", while nevertheless a sub‐ sequent read blocks. This could for example happen when data has arrived but upon examination has wrong
checksum and is discarded. There may be other circumstances in which
a file descriptor is spuriously reported as ready. Thus it may be safer to use O_NONBLOCK on sockets that should not block.

Unlikely to apply here, I've also found the real problem (see other mail in this thread).

cheers,
Gerd

Gerd Hoffmann

author

2008-11-12 13:04 UTC #10 lore raw

In reply to Anthony Liguori

Quoted 10 lines · Anthony Liguori

Quoted 6 lines · Gerd Hoffmann
Hi,
$subject says all.
please apply,
Gerd
Under what circumstances? posix_aio_read() is only invoked from a select callback. This means there should be data available to be read.

Well, there are *two* select loops: main_loop_wait() and qemu_aio_wait(). Calling sync block i/o functions from a i/o handler causes the two select loops run nested => boom.

cheers,
Gerd

Anthony Liguori

2008-11-12 14:57 UTC #11 lore raw

Quoted 13 lines · Gerd Hoffmann

Quoted 9 lines · Anthony Liguori
Quoted 7 lines · Gerd Hoffmann
Hi,
$subject says all.
please apply,
Gerd

Quoted 8 lines

Quoted 3 lines
Under what circumstances? posix_aio_read() is only invoked from a select callback. This means there should be data available to be read.
Well, there are *two* select loops: main_loop_wait() and qemu_aio_wait(). Calling sync block i/o functions from a i/o handler causes the two select loops run nested => boom.

Yeah, qemu_aio_wait needs to die. Can you resubmit your patch with a better description, and change the read() look in posix_aio_read() to consume as much data as possible before hitting EAGAIN?

Regards,

Anthony Liguori

Quoted 5 lines

cheers,
Gerd

Gerd Hoffmann

author

2008-11-13 09:14 UTC #12 lore raw

Quoted 6 lines · Anthony Liguori

Quoted 5 lines · Gerd Hoffmann
Quoted 4 lines
Under what circumstances? posix_aio_read() is only invoked from a select callback. This means there should be data available to be read.

Quoted 7 lines

Quoted 3 lines
Well, there are *two* select loops: main_loop_wait() and qemu_aio_wait(). Calling sync block i/o functions from a i/o handler causes the two select loops run nested => boom.
Yeah, qemu_aio_wait needs to die. Can you resubmit your patch with a better description, and change the read() look in posix_aio_read() to consume as much data as possible before hitting EAGAIN?

I've fixed my problem by changing xen_disk to use a bottom half for actual work, so the block read/write calls are moved out of the select loop anyway. Which turned out to be useful for aio support too.

So I'm fine again with the current state. I can create such a patch nevertheless though.

cheers,
Gerd