[Qemu-devel] [PATCH 1/2] Use fd signal trick to break us out of select; do not sigwait

[Qemu-devel] [PATCH 1/2] Use fd signal trick to break us out of select; do not sigwait

[Ian Jackson]

* Use of SIGUSR2 to interrupt select() does not work because signals
  which arrive just before entry to select() do not interrupt it.

* The sigwait approach to detecting aio does not work properly because
  some versions of glibc forget to block signals on the private aio
  thread under some hard-to-reproduce conditions.  This means that
  blocking SIGUSR2 is ineffective; the signals can be lost and the
  program can block in sigwait (!)

So instead we use the time-honoured self-pipe trick: in the signal
handler we write to a pipe, which we select on when we want to wait
for the signal, and which we read from (to empty out) just before
actually doing the `top half' processing which deals with the condition
to which the signal relates.

We use the existing fd handler infrastructure to run the desired
completion code out of the main event loop; in the aio completion wait
we use a cut-down version of the same arrangements.

(Cherry picked from qemu-xen ef9633e1290d055543136e136b6b7f59d863a601
 Conflicts: block-raw-posix.c)

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 block-raw-posix.c |   50 ++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 38 insertions(+), 12 deletions(-)

diff --git a/block-raw-posix.c b/block-raw-posix.c
index 96edab4..94928c0 100644
--- a/block-raw-posix.c
+++ b/block-raw-posix.c
@@ -23,6 +23,8 @@
  */
 #include "qemu-common.h"
 #if !defined(QEMU_IMG) && !defined(QEMU_NBD)
+#include "qemu-char.h"
+#include "qemu_socket.h"
 #include "qemu-timer.h"
 #include "exec-all.h"
 #endif
@@ -441,10 +443,14 @@ typedef struct RawAIOCB {
 static int aio_sig_num = SIGUSR2;
 static RawAIOCB *first_aio; /* AIO issued */
 static int aio_initialized = 0;
+static int aio_sig_pipe[2];
 
 static void aio_signal_handler(int signum)
 {
 #if !defined(QEMU_IMG) && !defined(QEMU_NBD)
+    int e;
+    e = errno;
+    write(aio_sig_pipe[1],"",1); /* ignore errors as they should be EAGAIN */
     CPUState *env = cpu_single_env;
     if (env) {
         /* stop the currently executing cpu because a timer occured */
@@ -455,12 +461,34 @@ static void aio_signal_handler(int signum)
         }
 #endif
     }
+    errno = e;
 #endif
 }
 
+#if !defined(QEMU_IMG) && !defined(QEMU_NBD)
+static void qemu_aio_sig_pipe_read(void *opaque_ignored) {
+    qemu_aio_poll();
+}
+#endif
+
 void qemu_aio_init(void)
 {
     struct sigaction act;
+    int ret;
+
+    ret = pipe(aio_sig_pipe);
+    if (ret) { perror("qemu_aio_init pipe failed"); exit(-1); }
+    fcntl(aio_sig_pipe[0], F_SETFL, O_NONBLOCK);
+    fcntl(aio_sig_pipe[1], F_SETFL, O_NONBLOCK);
+
+#if !defined(QEMU_IMG) && !defined(QEMU_NBD)
+    ret = qemu_set_fd_handler2(aio_sig_pipe[0], NULL,
+                               qemu_aio_sig_pipe_read, NULL, NULL);
+    if (ret) {
+        fputs("qemu_aio_init set_fd_handler failed\n",stderr);
+        exit(-1);
+    }
+#endif
 
     aio_initialized = 1;
 
@@ -488,6 +516,12 @@ void qemu_aio_poll(void)
     RawAIOCB *acb, **pacb;
     int ret;
 
+    /* eat any pending signal notifications */
+    {
+        char dummy_buf[16];
+        read(aio_sig_pipe[0],dummy_buf,sizeof(dummy_buf));
+    }
+
     for(;;) {
         pacb = &first_aio;
         for(;;) {
@@ -536,37 +570,29 @@ void qemu_aio_flush(void)
 }
 
 /* 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;
+    fd_set check;
 
 #if !defined(QEMU_IMG) && !defined(QEMU_NBD)
     if (qemu_bh_poll())
         return;
 #endif
-    sigemptyset(&set);
-    sigaddset(&set, aio_sig_num);
-    sigwait(&set, &nb_sigs);
+    FD_ZERO(&check);
+    FD_SET(aio_sig_pipe[0], &check);
+    select(aio_sig_pipe[0]+1, &check,0,&check, 0);
     qemu_aio_poll();
 }
 
 void qemu_aio_wait_end(void)
 {
-    sigprocmask(SIG_SETMASK, &wait_oset, NULL);
 }
 
 static RawAIOCB *raw_aio_setup(BlockDriverState *bs,
-- 
1.4.4.4

[Qemu-devel] [PATCH 2/2] Introduce #define QEMU_ASYNC_EVENTLOOP to simplify #ifdefs

[Ian Jackson]

In various places we have
  #if !defined(QEMU_IMG) && !defined(QEMU_NBD)
because those standalone tools do not have the whole qemu event loop
and asynchronous IO machinery.  To reduce the number of places where
these facts are encoded, and to make it easier to introduce any new
helper program, we replace this with
  #ifdef QEMU_ASYNC_EVENTLOOP
which is defined in qemu-common.h.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 block-raw-posix.c |   14 +++++++-------
 qemu-common.h     |    5 +++++
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/block-raw-posix.c b/block-raw-posix.c
index 94928c0..ef1c993 100644
--- a/block-raw-posix.c
+++ b/block-raw-posix.c
@@ -22,7 +22,7 @@
  * THE SOFTWARE.
  */
 #include "qemu-common.h"
-#if !defined(QEMU_IMG) && !defined(QEMU_NBD)
+#ifdef QEMU_ASYNC_EVENTLOOP
 #include "qemu-char.h"
 #include "qemu_socket.h"
 #include "qemu-timer.h"
@@ -447,7 +447,7 @@ static int aio_sig_pipe[2];
 
 static void aio_signal_handler(int signum)
 {
-#if !defined(QEMU_IMG) && !defined(QEMU_NBD)
+#ifdef QEMU_ASYNC_EVENTLOOP
     int e;
     e = errno;
     write(aio_sig_pipe[1],"",1); /* ignore errors as they should be EAGAIN */
@@ -465,7 +465,7 @@ static void aio_signal_handler(int signum)
 #endif
 }
 
-#if !defined(QEMU_IMG) && !defined(QEMU_NBD)
+#ifdef QEMU_ASYNC_EVENTLOOP
 static void qemu_aio_sig_pipe_read(void *opaque_ignored) {
     qemu_aio_poll();
 }
@@ -481,7 +481,7 @@ void qemu_aio_init(void)
     fcntl(aio_sig_pipe[0], F_SETFL, O_NONBLOCK);
     fcntl(aio_sig_pipe[1], F_SETFL, O_NONBLOCK);
 
-#if !defined(QEMU_IMG) && !defined(QEMU_NBD)
+#ifdef QEMU_ASYNC_EVENTLOOP
     ret = qemu_set_fd_handler2(aio_sig_pipe[0], NULL,
                                qemu_aio_sig_pipe_read, NULL, NULL);
     if (ret) {
@@ -581,7 +581,7 @@ void qemu_aio_wait(void)
 {
     fd_set check;
 
-#if !defined(QEMU_IMG) && !defined(QEMU_NBD)
+#ifdef QEMU_ASYNC_EVENTLOOP
     if (qemu_bh_poll())
         return;
 #endif
@@ -622,7 +622,7 @@ static RawAIOCB *raw_aio_setup(BlockDriverState *bs,
     return acb;
 }
 
-#if !defined(QEMU_IMG) && !defined(QEMU_NBD)
+#ifdef QEMU_ASYNC_EVENTLOOP
 static void raw_aio_em_cb(void* opaque)
 {
     RawAIOCB *acb = opaque;
@@ -744,7 +744,7 @@ void qemu_aio_wait_start(void)
 
 void qemu_aio_wait(void)
 {
-#if !defined(QEMU_IMG) && !defined(QEMU_NBD)
+#ifdef QEMU_ASYNC_EVENTLOOP
     qemu_bh_poll();
 #endif
 }
diff --git a/qemu-common.h b/qemu-common.h
index 23d1444..04c38fd 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -49,6 +49,11 @@ static inline char *realpath(const char *path, char *resolved_path)
 #define PRIo64 "I64o"
 #endif
 
+#if !defined(QEMU_IMG) && !defined(QEMU_NBD)
+# define QEMU_ASYNC_EVENTLOOP
+  /* The standalone utilities do not use or support asynchronous IO */
+#endif
+
 /* FIXME: Remove NEED_CPU_H.  */
 #ifndef NEED_CPU_H
 
-- 
1.4.4.4

Re: [Qemu-devel] [PATCH 1/2] Use fd signal trick to break us out of select; do not sigwait

[Anthony Liguori]

Ian Jackson wrote:
> * Use of SIGUSR2 to interrupt select() does not work because signals
>   which arrive just before entry to select() do not interrupt it.
>
> * The sigwait approach to detecting aio does not work properly because
>   some versions of glibc forget to block signals on the private aio
>   thread under some hard-to-reproduce conditions.  This means that
>   blocking SIGUSR2 is ineffective; the signals can be lost and the
>   program can block in sigwait (!)
>
> So instead we use the time-honoured self-pipe trick: in the signal
> handler we write to a pipe, which we select on when we want to wait
> for the signal, and which we read from (to empty out) just before
> actually doing the `top half' processing which deals with the condition
> to which the signal relates.
>
> We use the existing fd handler infrastructure to run the desired
> completion code out of the main event loop; in the aio completion wait
> we use a cut-down version of the same arrangements.
>   

Hi Ian,

Somehow, I didn't see this until today--sorry for the delayed response.  
I personally prefer the patch I posted.  I've also got another patch on 
top of it that refactors the aio API so that multiple aio 
implementations can be used.  I notice you leave the qemu_aio_poll() in 
place in the main loop, is there a reason you left it there?

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 2/2] Introduce #define QEMU_ASYNC_EVENTLOOP to simplify #ifdefs

[Anthony Liguori]

Ian Jackson wrote:
> In various places we have
>   #if !defined(QEMU_IMG) && !defined(QEMU_NBD)
> because those standalone tools do not have the whole qemu event loop
> and asynchronous IO machinery.  To reduce the number of places where
> these facts are encoded, and to make it easier to introduce any new
> helper program, we replace this with
>   #ifdef QEMU_ASYNC_EVENTLOOP
> which is defined in qemu-common.h.
>   

I don't understand why we need two defines in the first place.  I 
noticed in the Makefile that we have rules to build qemu-nbd- objects 
and qemu-img- objects, but I don't see why that should be necessary.

Regards,

Anthony Liguori

> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
>  block-raw-posix.c |   14 +++++++-------
>  qemu-common.h     |    5 +++++
>  2 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/block-raw-posix.c b/block-raw-posix.c
> index 94928c0..ef1c993 100644
> --- a/block-raw-posix.c
> +++ b/block-raw-posix.c
> @@ -22,7 +22,7 @@
>   * THE SOFTWARE.
>   */
>  #include "qemu-common.h"
> -#if !defined(QEMU_IMG) && !defined(QEMU_NBD)
> +#ifdef QEMU_ASYNC_EVENTLOOP
>  #include "qemu-char.h"
>  #include "qemu_socket.h"
>  #include "qemu-timer.h"
> @@ -447,7 +447,7 @@ static int aio_sig_pipe[2];
>  
>  static void aio_signal_handler(int signum)
>  {
> -#if !defined(QEMU_IMG) && !defined(QEMU_NBD)
> +#ifdef QEMU_ASYNC_EVENTLOOP
>      int e;
>      e = errno;
>      write(aio_sig_pipe[1],"",1); /* ignore errors as they should be EAGAIN */
> @@ -465,7 +465,7 @@ static void aio_signal_handler(int signum)
>  #endif
>  }
>  
> -#if !defined(QEMU_IMG) && !defined(QEMU_NBD)
> +#ifdef QEMU_ASYNC_EVENTLOOP
>  static void qemu_aio_sig_pipe_read(void *opaque_ignored) {
>      qemu_aio_poll();
>  }
> @@ -481,7 +481,7 @@ void qemu_aio_init(void)
>      fcntl(aio_sig_pipe[0], F_SETFL, O_NONBLOCK);
>      fcntl(aio_sig_pipe[1], F_SETFL, O_NONBLOCK);
>  
> -#if !defined(QEMU_IMG) && !defined(QEMU_NBD)
> +#ifdef QEMU_ASYNC_EVENTLOOP
>      ret = qemu_set_fd_handler2(aio_sig_pipe[0], NULL,
>                                 qemu_aio_sig_pipe_read, NULL, NULL);
>      if (ret) {
> @@ -581,7 +581,7 @@ void qemu_aio_wait(void)
>  {
>      fd_set check;
>  
> -#if !defined(QEMU_IMG) && !defined(QEMU_NBD)
> +#ifdef QEMU_ASYNC_EVENTLOOP
>      if (qemu_bh_poll())
>          return;
>  #endif
> @@ -622,7 +622,7 @@ static RawAIOCB *raw_aio_setup(BlockDriverState *bs,
>      return acb;
>  }
>  
> -#if !defined(QEMU_IMG) && !defined(QEMU_NBD)
> +#ifdef QEMU_ASYNC_EVENTLOOP
>  static void raw_aio_em_cb(void* opaque)
>  {
>      RawAIOCB *acb = opaque;
> @@ -744,7 +744,7 @@ void qemu_aio_wait_start(void)
>  
>  void qemu_aio_wait(void)
>  {
> -#if !defined(QEMU_IMG) && !defined(QEMU_NBD)
> +#ifdef QEMU_ASYNC_EVENTLOOP
>      qemu_bh_poll();
>  #endif
>  }
> diff --git a/qemu-common.h b/qemu-common.h
> index 23d1444..04c38fd 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -49,6 +49,11 @@ static inline char *realpath(const char *path, char *resolved_path)
>  #define PRIo64 "I64o"
>  #endif
>  
> +#if !defined(QEMU_IMG) && !defined(QEMU_NBD)
> +# define QEMU_ASYNC_EVENTLOOP
> +  /* The standalone utilities do not use or support asynchronous IO */
> +#endif
> +
>  /* FIXME: Remove NEED_CPU_H.  */
>  #ifndef NEED_CPU_H
>  
>   

Re: [Qemu-devel] [PATCH 1/2] Use fd signal trick to break us out of select; do not sigwait

[Ian Jackson]

Anthony Liguori writes ("Re: [Qemu-devel] [PATCH 1/2] Use fd signal trick to break us out of select; do not sigwait"):
> Somehow, I didn't see this until today--sorry for the delayed response.  
> I personally prefer the patch I posted.

Did you see Jamie Lokier's message about the behaviour of glibc 2.3.1
with respect to signal blocking and the glibc aio helper threads ?
That is, it doesn't always block them.

I think the result is that with your patch you risk having the signal
delivered on the aio helper thread.

>  I've also got another patch on top of it that refactors the aio API
> so that multiple aio implementations can be used.

That doesn't seem unreasonable.  But we have to have an answer which
works around the glibc bug.

>  I notice you leave the qemu_aio_poll() in place in the main loop,
> is there a reason you left it there?

This is essential as it does the actual work for AIO completion.

Ian.

Re: [Qemu-devel] [PATCH 2/2] Introduce #define QEMU_ASYNC_EVENTLOOP to simplify #ifdefs

[Ian Jackson]

Anthony Liguori writes ("Re: [Qemu-devel] [PATCH 2/2] Introduce #define QEMU_ASYNC_EVENTLOOP to simplify #ifdefs"):
> I don't understand why we need two defines in the first place.  I 
> noticed in the Makefile that we have rules to build qemu-nbd- objects 
> and qemu-img- objects, but I don't see why that should be necessary.

Well, the original #define was called QEMU_IMG and the objects were
called qemu-img-*.  Surely we didn't expect Laurent to reuse those ?
The names clearly indicated not to.

Perhaps it would be better to reorganise this some more and have
qemu-syncioutil-*.o but I think to avoid the best being the enemy of
the good I would argue that my patch should be applied in the
meantime.

Ian.

Re: [Qemu-devel] [PATCH 2/2] Introduce #define QEMU_ASYNC_EVENTLOOP to simplify #ifdefs

[Anthony Liguori]

Ian Jackson wrote:
> Anthony Liguori writes ("Re: [Qemu-devel] [PATCH 2/2] Introduce #define QEMU_ASYNC_EVENTLOOP to simplify #ifdefs"):
>   
>> I don't understand why we need two defines in the first place.  I 
>> noticed in the Makefile that we have rules to build qemu-nbd- objects 
>> and qemu-img- objects, but I don't see why that should be necessary.
>>     
>
> Well, the original #define was called QEMU_IMG and the objects were
> called qemu-img-*.  Surely we didn't expect Laurent to reuse those ?
> The names clearly indicated not to.
>   

Right, I can't see a reason why we would need to compile a single object 
different for qemu-img and qemu-nbd.  I think there's something fowl there.

> Perhaps it would be better to reorganise this some more and have
> qemu-syncioutil-*.o but I think to avoid the best being the enemy of
> the good I would argue that my patch should be applied in the
> meantime.
>   

Half solutions often mask the underlying problem because people think 
it's fixed.  I only think it's appropriate to do this sort of thing if 
the amount of work to do it right is prohibitively high.  In this case, 
someone just has to do a quick audit of the Makefile and respective .c 
files or Laurent just needs to explain why he did it that way :-)

Regards,

Anthony Liguori

> Ian.
>
>
>