[Qemu-devel] [5187] Use signalfd() to work around signal/select race

[Qemu-devel] [5187] Use signalfd() to work around signal/select race

[Anthony Liguori]

Revision: 5187
          http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=5187
Author:   aliguori
Date:     2008-09-10 15:45:19 +0000 (Wed, 10 Sep 2008)

Log Message:
-----------
Use signalfd() to work around signal/select race

This patch introduces signalfd() to work around the signal/select race in
checking for AIO completions.  For platforms that don't support signalfd(), we
emulate it with threads.

There was a long discussion about this approach.  I don't believe there are any
fundamental problems with this approach and I believe eliminating the use of
signals is a good thing.

I've tested Windows and Linux using Windows and Linux guests.  I've also checked
for disk IO performance regressions.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

Modified Paths:
--------------
    trunk/Makefile
    trunk/Makefile.target
    trunk/block-raw-posix.c
    trunk/block-raw-win32.c
    trunk/block.c
    trunk/block.h
    trunk/vl.c

Modified: trunk/Makefile
===================================================================
--- trunk/Makefile	2008-09-10 15:23:19 UTC (rev 5186)
+++ trunk/Makefile	2008-09-10 15:45:19 UTC (rev 5187)
@@ -177,7 +177,7 @@
 ifdef CONFIG_WIN32
 QEMU_IMG_BLOCK_OBJS += qemu-img-block-raw-win32.o
 else
-QEMU_IMG_BLOCK_OBJS += nbd.o qemu-img-block-raw-posix.o
+QEMU_IMG_BLOCK_OBJS += nbd.o qemu-img-block-raw-posix.o compatfd.o
 endif
 
 ######################################################################
@@ -195,7 +195,7 @@
 	$(CC) $(CFLAGS) $(CPPFLAGS) -DQEMU_NBD -c -o $@ $<
 
 qemu-nbd$(EXESUF):  qemu-nbd.o qemu-nbd-nbd.o qemu-img-block.o \
-		    osdep.o qemu-nbd-block-raw-posix.o $(BLOCK_OBJS)
+		    osdep.o qemu-nbd-block-raw-posix.o compatfd.o $(BLOCK_OBJS)
 	$(CC) $(LDFLAGS) -o $@ $^ -lz $(LIBS)
 
 # dyngen host tool

Modified: trunk/Makefile.target
===================================================================
--- trunk/Makefile.target	2008-09-10 15:23:19 UTC (rev 5186)
+++ trunk/Makefile.target	2008-09-10 15:45:19 UTC (rev 5187)
@@ -476,7 +476,7 @@
 ifdef CONFIG_WIN32
 OBJS+=block-raw-win32.o
 else
-OBJS+=block-raw-posix.o
+OBJS+=block-raw-posix.o compatfd.o
 endif
 
 LIBS+=-lz

Modified: trunk/block-raw-posix.c
===================================================================
--- trunk/block-raw-posix.c	2008-09-10 15:23:19 UTC (rev 5186)
+++ trunk/block-raw-posix.c	2008-09-10 15:45:19 UTC (rev 5187)
@@ -25,8 +25,10 @@
 #if !defined(QEMU_IMG) && !defined(QEMU_NBD)
 #include "qemu-timer.h"
 #include "exec-all.h"
+#include "qemu-char.h"
 #endif
 #include "block_int.h"
+#include "compatfd.h"
 #include <assert.h>
 #ifdef CONFIG_AIO
 #include <aio.h>
@@ -438,53 +440,13 @@
     int ret;
 } RawAIOCB;
 
+static int aio_sig_fd = -1;
 static int aio_sig_num = SIGUSR2;
 static RawAIOCB *first_aio; /* AIO issued */
 static int aio_initialized = 0;
 
-static void aio_signal_handler(int signum)
+static void qemu_aio_poll(void *opaque)
 {
-#if !defined(QEMU_IMG) && !defined(QEMU_NBD)
-    CPUState *env = cpu_single_env;
-    if (env) {
-        /* stop the currently executing cpu because a timer occured */
-        cpu_interrupt(env, CPU_INTERRUPT_EXIT);
-#ifdef USE_KQEMU
-        if (env->kqemu_enabled) {
-            kqemu_cpu_interrupt(env);
-        }
-#endif
-    }
-#endif
-}
-
-void qemu_aio_init(void)
-{
-    struct sigaction act;
-
-    aio_initialized = 1;
-
-    sigfillset(&act.sa_mask);
-    act.sa_flags = 0; /* do not restart syscalls to interrupt select() */
-    act.sa_handler = aio_signal_handler;
-    sigaction(aio_sig_num, &act, NULL);
-
-#if defined(__GLIBC__) && defined(__linux__)
-    {
-        /* XXX: aio thread exit seems to hang on RedHat 9 and this init
-           seems to fix the problem. */
-        struct aioinit ai;
-        memset(&ai, 0, sizeof(ai));
-        ai.aio_threads = 1;
-        ai.aio_num = 1;
-        ai.aio_idle_time = 365 * 100000;
-        aio_init(&ai);
-    }
-#endif
-}
-
-void qemu_aio_poll(void)
-{
     RawAIOCB *acb, **pacb;
     int ret;
 
@@ -524,49 +486,66 @@
  the_end: ;
 }
 
+void qemu_aio_init(void)
+{
+    sigset_t mask;
+
+    aio_initialized = 1;
+
+    /* Make sure to block AIO signal */
+    sigemptyset(&mask);
+    sigaddset(&mask, aio_sig_num);
+    sigprocmask(SIG_BLOCK, &mask, NULL);
+    
+    aio_sig_fd = qemu_signalfd(&mask);
+#if !defined(QEMU_IMG) && !defined(QEMU_NBD)
+    qemu_set_fd_handler2(aio_sig_fd, NULL, qemu_aio_poll, NULL, NULL);
+#endif
+
+#if defined(__GLIBC__) && defined(__linux__)
+    {
+        /* XXX: aio thread exit seems to hang on RedHat 9 and this init
+           seems to fix the problem. */
+        struct aioinit ai;
+        memset(&ai, 0, sizeof(ai));
+        ai.aio_threads = 1;
+        ai.aio_num = 1;
+        ai.aio_idle_time = 365 * 100000;
+        aio_init(&ai);
+    }
+#endif
+}
+
 /* Wait for all IO requests to complete.  */
 void qemu_aio_flush(void)
 {
-    qemu_aio_wait_start();
-    qemu_aio_poll();
+    qemu_aio_poll(NULL);
     while (first_aio) {
         qemu_aio_wait();
     }
-    qemu_aio_wait_end();
 }
 
-/* wait until at least one AIO was handled */
-static sigset_t wait_oset;
-
-void qemu_aio_wait_start(void)
-{
-    sigset_t set;
-
-    if (!aio_initialized)
-        qemu_aio_init();
-    sigemptyset(&set);
-    sigaddset(&set, aio_sig_num);
-    sigprocmask(SIG_BLOCK, &set, &wait_oset);
-}
-
 void qemu_aio_wait(void)
 {
-    sigset_t set;
-    int nb_sigs;
+    int ret;
 
 #if !defined(QEMU_IMG) && !defined(QEMU_NBD)
     if (qemu_bh_poll())
         return;
 #endif
-    sigemptyset(&set);
-    sigaddset(&set, aio_sig_num);
-    sigwait(&set, &nb_sigs);
-    qemu_aio_poll();
-}
 
-void qemu_aio_wait_end(void)
-{
-    sigprocmask(SIG_SETMASK, &wait_oset, NULL);
+    do {
+        fd_set rdfds;
+
+        FD_ZERO(&rdfds);
+        FD_SET(aio_sig_fd, &rdfds);
+
+        ret = select(aio_sig_fd + 1, &rdfds, NULL, NULL, NULL);
+        if (ret == -1 && errno == EINTR)
+            continue;
+    } while (ret == 0);
+
+    qemu_aio_poll(NULL);
 }
 
 static RawAIOCB *raw_aio_setup(BlockDriverState *bs,
@@ -704,18 +683,10 @@
 {
 }
 
-void qemu_aio_poll(void)
-{
-}
-
 void qemu_aio_flush(void)
 {
 }
 
-void qemu_aio_wait_start(void)
-{
-}
-
 void qemu_aio_wait(void)
 {
 #if !defined(QEMU_IMG) && !defined(QEMU_NBD)
@@ -723,10 +694,6 @@
 #endif
 }
 
-void qemu_aio_wait_end(void)
-{
-}
-
 #endif /* CONFIG_AIO */
 
 static void raw_close(BlockDriverState *bs)

Modified: trunk/block-raw-win32.c
===================================================================
--- trunk/block-raw-win32.c	2008-09-10 15:23:19 UTC (rev 5186)
+++ trunk/block-raw-win32.c	2008-09-10 15:45:19 UTC (rev 5187)
@@ -350,18 +350,10 @@
 {
 }
 
-void qemu_aio_poll(void)
-{
-}
-
 void qemu_aio_flush(void)
 {
 }
 
-void qemu_aio_wait_start(void)
-{
-}
-
 void qemu_aio_wait(void)
 {
 #ifndef QEMU_IMG
@@ -369,10 +361,6 @@
 #endif
 }
 
-void qemu_aio_wait_end(void)
-{
-}
-
 BlockDriver bdrv_raw = {
     "raw",
     sizeof(BDRVRawState),

Modified: trunk/block.c
===================================================================
--- trunk/block.c	2008-09-10 15:23:19 UTC (rev 5186)
+++ trunk/block.c	2008-09-10 15:45:19 UTC (rev 5187)
@@ -1280,17 +1280,15 @@
     BlockDriverAIOCB *acb;
 
     async_ret = NOT_DONE;
-    qemu_aio_wait_start();
     acb = bdrv_aio_read(bs, sector_num, buf, nb_sectors,
                         bdrv_rw_em_cb, &async_ret);
-    if (acb == NULL) {
-        qemu_aio_wait_end();
+    if (acb == NULL)
         return -1;
-    }
+
     while (async_ret == NOT_DONE) {
         qemu_aio_wait();
     }
-    qemu_aio_wait_end();
+
     return async_ret;
 }
 
@@ -1301,17 +1299,13 @@
     BlockDriverAIOCB *acb;
 
     async_ret = NOT_DONE;
-    qemu_aio_wait_start();
     acb = bdrv_aio_write(bs, sector_num, buf, nb_sectors,
                          bdrv_rw_em_cb, &async_ret);
-    if (acb == NULL) {
-        qemu_aio_wait_end();
+    if (acb == NULL)
         return -1;
-    }
     while (async_ret == NOT_DONE) {
         qemu_aio_wait();
     }
-    qemu_aio_wait_end();
     return async_ret;
 }
 

Modified: trunk/block.h
===================================================================
--- trunk/block.h	2008-09-10 15:23:19 UTC (rev 5186)
+++ trunk/block.h	2008-09-10 15:45:19 UTC (rev 5187)
@@ -90,11 +90,8 @@
 void bdrv_aio_cancel(BlockDriverAIOCB *acb);
 
 void qemu_aio_init(void);
-void qemu_aio_poll(void);
 void qemu_aio_flush(void);
-void qemu_aio_wait_start(void);
 void qemu_aio_wait(void);
-void qemu_aio_wait_end(void);
 
 int qemu_key_check(BlockDriverState *bs, const char *name);
 

Modified: trunk/vl.c
===================================================================
--- trunk/vl.c	2008-09-10 15:23:19 UTC (rev 5186)
+++ trunk/vl.c	2008-09-10 15:45:19 UTC (rev 5187)
@@ -7482,7 +7482,6 @@
         slirp_select_poll(&rfds, &wfds, &xfds);
     }
 #endif
-    qemu_aio_poll();
 
     if (vm_running) {
         if (likely(!(cur_cpu->singlestep_enabled & SSTEP_NOTIMER)))

Re: [Qemu-devel] [5187] Use signalfd() to work around signal/select race

[Blue Swirl]

On 9/10/08, Anthony Liguori <anthony@codemonkey.ws> wrote:
> Revision: 5187
>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=5187
>  Author:   aliguori
>  Date:     2008-09-10 15:45:19 +0000 (Wed, 10 Sep 2008)
>
>  Log Message:
>  -----------
>  Use signalfd() to work around signal/select race
>
>  This patch introduces signalfd() to work around the signal/select race in
>  checking for AIO completions.  For platforms that don't support signalfd(), we
>  emulate it with threads.
>
>  There was a long discussion about this approach.  I don't believe there are any
>  fundamental problems with this approach and I believe eliminating the use of
>  signals is a good thing.
>
>  I've tested Windows and Linux using Windows and Linux guests.  I've also checked
>  for disk IO performance regressions.
>
>  Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>
>  Modified Paths:
>  --------------
>     trunk/Makefile
>     trunk/Makefile.target
>     trunk/block-raw-posix.c
>     trunk/block-raw-win32.c
>     trunk/block.c
>     trunk/block.h
>     trunk/vl.c

The file compatfd.h is missing.

Re: [Qemu-devel] [5187] Use signalfd() to work around signal/select race

[C.W. Betts]

[-- Attachment #1: Type: text/plain, Size: 1234 bytes --]


On Sep 10, 2008, at 10:55 AM, Blue Swirl wrote:

> On 9/10/08, Anthony Liguori <anthony@codemonkey.ws> wrote:
>> Revision: 5187
>>          http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=5187
>> Author:   aliguori
>> Date:     2008-09-10 15:45:19 +0000 (Wed, 10 Sep 2008)
>>
>> Log Message:
>> -----------
>> Use signalfd() to work around signal/select race
>>
>> This patch introduces signalfd() to work around the signal/select  
>> race in
>> checking for AIO completions.  For platforms that don't support  
>> signalfd(), we
>> emulate it with threads.
>>
>> There was a long discussion about this approach.  I don't believe  
>> there are any
>> fundamental problems with this approach and I believe eliminating  
>> the use of
>> signals is a good thing.
>>
>> I've tested Windows and Linux using Windows and Linux guests.  I've  
>> also checked
>> for disk IO performance regressions.
>>
>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>>
>> Modified Paths:
>> --------------
>>    trunk/Makefile
>>    trunk/Makefile.target
>>    trunk/block-raw-posix.c
>>    trunk/block-raw-win32.c
>>    trunk/block.c
>>    trunk/block.h
>>    trunk/vl.c
>
> The file compatfd.h is missing.
>
And possible compatfd.c


[-- Attachment #2: Type: text/html, Size: 3010 bytes --]

Re: [Qemu-devel] [5187] Use signalfd() to work around signal/select race

[Anthony Liguori]

Blue Swirl wrote:
> On 9/10/08, Anthony Liguori <anthony@codemonkey.ws> wrote:
>   
> The file compatfd.h is missing.
>   

Ugh, sorry about that!

Regards,

Anthony Liguori

Re: [Qemu-devel] [5187] Use signalfd() to work around signal/select race

[Laurent Vivier]

Le mercredi 10 septembre 2008 à 15:45 +0000, Anthony Liguori a écrit :
> Revision: 5187
>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=5187
> Author:   aliguori
> Date:     2008-09-10 15:45:19 +0000 (Wed, 10 Sep 2008)
> 
> Log Message:
> -----------
> Use signalfd() to work around signal/select race

This commit breaks qemu-nbd:

# ./qemu-nbd --port 1024 --persistent ../disk.qcow2 
generates "Bus error" when I connect the client:
# ./x86_64-softmmu/qemu-system-x86_64 -hda ../etch64.qcow2 -hdb
nbd:localhost:1024

* gdb output:

Program received signal SIGBUS, Bus error.
[Switching to Thread 0x7f3accdf76e0 (LWP 13146)]
qemu_aio_wait () at block-raw-posix.c:541
541	        FD_SET(aio_sig_fd, &rdfds);
(gdb) bt
#0  qemu_aio_wait () at block-raw-posix.c:541
#1  0x0000000000406245 in bdrv_read_em (bs=0x0,
sector_num=140736764918416, 
    buf=0x8000000000000000 <Address 0x8000000000000000 out of bounds>, 
    nb_sectors=0) at block.c:1289
#2  0x000000000040437a in nbd_trip (bs=0x622010, csock=8,
size=6442450944, 
    dev_offset=0, offset=0x7fffd4e13c78, readonly=false, 
    data=0x7f3acccd5200 "�H\220м", data_size=1048576) at nbd.c:573
#3  0x0000000000402d11 in main (argc=1, argv=0x1) at qemu-nbd.c:444

Laurent
-- 
----------------- Laurent.Vivier@bull.net  ------------------
  "La perfection est atteinte non quand il ne reste rien à
ajouter mais quand il ne reste rien à enlever." Saint Exupéry

Re: [Qemu-devel] [5187] Use signalfd() to work around signal/select race

[Anthony Liguori]

Laurent Vivier wrote:
> Le mercredi 10 septembre 2008 à 15:45 +0000, Anthony Liguori a écrit :
>   
>> Revision: 5187
>>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=5187
>> Author:   aliguori
>> Date:     2008-09-10 15:45:19 +0000 (Wed, 10 Sep 2008)
>>
>> Log Message:
>> -----------
>> Use signalfd() to work around signal/select race
>>     
>
> This commit breaks qemu-nbd:
>
> # ./qemu-nbd --port 1024 --persistent ../disk.qcow2 
> generates "Bus error" when I connect the client:
> # ./x86_64-softmmu/qemu-system-x86_64 -hda ../etch64.qcow2 -hdb
> nbd:localhost:1024
>
> * gdb output:
>
> Program received signal SIGBUS, Bus error.
> [Switching to Thread 0x7f3accdf76e0 (LWP 13146)]
> qemu_aio_wait () at block-raw-posix.c:541
> 541	        FD_SET(aio_sig_fd, &rdfds);
> (gdb) bt
> #0  qemu_aio_wait () at block-raw-posix.c:541
> #1  0x0000000000406245 in bdrv_read_em (bs=0x0,
> sector_num=140736764918416, 
>     buf=0x8000000000000000 <Address 0x8000000000000000 out of bounds>, 
>     nb_sectors=0) at block.c:1289
> #2  0x000000000040437a in nbd_trip (bs=0x622010, csock=8,
> size=6442450944, 
>     dev_offset=0, offset=0x7fffd4e13c78, readonly=false, 
>     data=0x7f3acccd5200 "�H\220м", data_size=1048576) at nbd.c:573
> #3  0x0000000000402d11 in main (argc=1, argv=0x1) at qemu-nbd.c:444
>   

That's because qemu-nbd isn't doing an qemu_aio_init().  There still 
appears to be an issue though because qemu_aio_wait() doesn't ever 
returning.  I'm looking into it right now.

Regards,

Anthony Liguori

> Laurent
>   

Re: [Qemu-devel] [5187] Use signalfd() to work around signal/select race

[Anthony Liguori]

Laurent Vivier wrote:
> Le mercredi 10 septembre 2008 à 15:45 +0000, Anthony Liguori a écrit :
>   
>> Revision: 5187
>>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=5187
>> Author:   aliguori
>> Date:     2008-09-10 15:45:19 +0000 (Wed, 10 Sep 2008)
>>
>> Log Message:
>> -----------
>> Use signalfd() to work around signal/select race
>>     
>
> This commit breaks qemu-nbd:
>
> # ./qemu-nbd --port 1024 --persistent ../disk.qcow2 
> generates "Bus error" when I connect the client:
> # ./x86_64-softmmu/qemu-system-x86_64 -hda ../etch64.qcow2 -hdb
> nbd:localhost:1024
>
> * gdb output:
>
> Program received signal SIGBUS, Bus error.
> [Switching to Thread 0x7f3accdf76e0 (LWP 13146)]
> qemu_aio_wait () at block-raw-posix.c:541
> 541	        FD_SET(aio_sig_fd, &rdfds);
> (gdb) bt
> #0  qemu_aio_wait () at block-raw-posix.c:541
> #1  0x0000000000406245 in bdrv_read_em (bs=0x0,
> sector_num=140736764918416, 
>     buf=0x8000000000000000 <Address 0x8000000000000000 out of bounds>, 
>     nb_sectors=0) at block.c:1289
> #2  0x000000000040437a in nbd_trip (bs=0x622010, csock=8,
> size=6442450944, 
>     dev_offset=0, offset=0x7fffd4e13c78, readonly=false, 
>     data=0x7f3acccd5200 "�H\220м", data_size=1048576) at nbd.c:573
> #3  0x0000000000402d11 in main (argc=1, argv=0x1) at qemu-nbd.c:444
>   

Should be fixed now.  But the following is broken:

qemu-system-x86_64 -hda nbd:localhost:1024

That was broken before the signalfd stuff.  My guest hangs after loading 
grub.  You're command line probably suggests you knew that.  Do you know 
why this is?

Regards,

Anthony Liguori

> Laurent
>   

Re: [Qemu-devel] [5187] Use signalfd() to work around signal/select race

[Laurent Vivier]

Le jeudi 11 septembre 2008 à 13:10 -0500, Anthony Liguori a écrit :
> Laurent Vivier wrote:
> > Le mercredi 10 septembre 2008 à 15:45 +0000, Anthony Liguori a écrit :
> >   
> >> Revision: 5187
> >>           http://svn.sv.gnu.org/viewvc/?view=rev&root=qemu&revision=5187
> >> Author:   aliguori
> >> Date:     2008-09-10 15:45:19 +0000 (Wed, 10 Sep 2008)
> >>
> >> Log Message:
> >> -----------
> >> Use signalfd() to work around signal/select race
> >>     
> >
> > This commit breaks qemu-nbd:
> >
> > # ./qemu-nbd --port 1024 --persistent ../disk.qcow2 
> > generates "Bus error" when I connect the client:
> > # ./x86_64-softmmu/qemu-system-x86_64 -hda ../etch64.qcow2 -hdb
> > nbd:localhost:1024
> >
> > * gdb output:
> >
> > Program received signal SIGBUS, Bus error.
> > [Switching to Thread 0x7f3accdf76e0 (LWP 13146)]
> > qemu_aio_wait () at block-raw-posix.c:541
> > 541	        FD_SET(aio_sig_fd, &rdfds);
> > (gdb) bt
> > #0  qemu_aio_wait () at block-raw-posix.c:541
> > #1  0x0000000000406245 in bdrv_read_em (bs=0x0,
> > sector_num=140736764918416, 
> >     buf=0x8000000000000000 <Address 0x8000000000000000 out of bounds>, 
> >     nb_sectors=0) at block.c:1289
> > #2  0x000000000040437a in nbd_trip (bs=0x622010, csock=8,
> > size=6442450944, 
> >     dev_offset=0, offset=0x7fffd4e13c78, readonly=false, 
> >     data=0x7f3acccd5200 "�H\220м", data_size=1048576) at nbd.c:573
> > #3  0x0000000000402d11 in main (argc=1, argv=0x1) at qemu-nbd.c:444
> >   
> 
> Should be fixed now.  But the following is broken:

Yes

> qemu-system-x86_64 -hda nbd:localhost:1024
> 
> That was broken before the signalfd stuff.  My guest hangs after loading 
> grub.  You're command line probably suggests you knew that.  Do you know 
> why this is?

It doesn't hang: it's just incredibly slow. Try "-socket /tmp/foo"
instead of "-port 1024" (you can add "--verbose" to see it is alive...)
or move the qemu-nbd to another server (with gigabit it's nice).

Laurent
-- 
----------------- Laurent.Vivier@bull.net  ------------------
  "La perfection est atteinte non quand il ne reste rien à
ajouter mais quand il ne reste rien à enlever." Saint Exupéry