Re: [Qemu-devel] [PATCH v2] block: avoid SIGUSR2

[Kevin Wolf]

Am 19.09.2011 16:37, schrieb Frediano Ziglio:
> Now that iothread is always compiled sending a signal seems only an
> additional step. This patch also avoid writing to two pipe (one from signal
> and one in qemu_service_io).
> 
> Work with kvm enabled or disabled. strace output is more readable (less 
> syscalls).
> 
> Signed-off-by: Frediano Ziglio <address@hidden>

Something in this change has bad effects, in the sense that it seems to
break bdrv_read_em.

To be precise, what I'm testing is booting from a DOS installation
floppy (interestingly, on my laptop it just works, but on my other test
box it fails). The first attempt of git bisect pointed at the commit
where we converted bdrv_read/write to coroutines.

However, it turned out that the conversion commit only caused problems
because instead of using a synchronous read() it now goes through
posix-aio-compat.c. The problem is reproducible in pre-coroutine
versions by just commenting out .bdrv_read/.bdrv_write in raw-posix.

Going back a bit more showed that this did work fine a while ago, and
the removal of SIGUSR2 is the first commit in which bdrv_read_em didn't
provide the same behaviour as bdrv_read any more.

I have no idea yet what's really going wrong, but maybe it rings a bell
for one of you?

Kevin

> ---
>  cpus.c             |    5 -----
>  posix-aio-compat.c |   29 +++++++++--------------------
>  2 files changed, 9 insertions(+), 25 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index 54c188c..d0cfe91 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -380,11 +380,6 @@ static int qemu_signal_init(void)
>      int sigfd;
>      sigset_t set;
>  
> -    /* SIGUSR2 used by posix-aio-compat.c */
> -    sigemptyset(&set);
> -    sigaddset(&set, SIGUSR2);
> -    pthread_sigmask(SIG_UNBLOCK, &set, NULL);
> -
>      /*
>       * SIG_IPI must be blocked in the main thread and must not be caught
>       * by sigwait() in the signal thread. Otherwise, the cpu thread will
> diff --git a/posix-aio-compat.c b/posix-aio-compat.c
> index 3193dbf..185d5b2 100644
> --- a/posix-aio-compat.c
> +++ b/posix-aio-compat.c
> @@ -42,7 +42,6 @@ struct qemu_paiocb {
>      int aio_niov;
>      size_t aio_nbytes;
>  #define aio_ioctl_cmd   aio_nbytes /* for QEMU_AIO_IOCTL */
> -    int ev_signo;
>      off_t aio_offset;
>  
>      QTAILQ_ENTRY(qemu_paiocb) node;
> @@ -309,6 +308,8 @@ static ssize_t handle_aiocb_rw(struct qemu_paiocb *aiocb)
>      return nbytes;
>  }
>  
> +static void posix_aio_notify_event(void);
> +
>  static void *aio_thread(void *unused)
>  {
>      pid_t pid;
> @@ -381,7 +382,7 @@ static void *aio_thread(void *unused)
>          aiocb->ret = ret;
>          mutex_unlock(&lock);
>  
> -        if (kill(pid, aiocb->ev_signo)) die("kill failed");
> +        posix_aio_notify_event();
>      }
>  
>      cur_threads--;
> @@ -548,18 +549,14 @@ static int posix_aio_flush(void *opaque)
>  
>  static PosixAioState *posix_aio_state;
>  
> -static void aio_signal_handler(int signum)
> +static void posix_aio_notify_event(void)
>  {
> -    if (posix_aio_state) {
> -        char byte = 0;
> -        ssize_t ret;
> -
> -        ret = write(posix_aio_state->wfd, &byte, sizeof(byte));
> -        if (ret < 0 && errno != EAGAIN)
> -            die("write()");
> -    }
> +    char byte = 0;
> +    ssize_t ret;
>  
> -    qemu_service_io();
> +    ret = write(posix_aio_state->wfd, &byte, sizeof(byte));
> +    if (ret < 0 && errno != EAGAIN)
> +        die("write()");
>  }
>  
>  static void paio_remove(struct qemu_paiocb *acb)
> @@ -623,7 +620,6 @@ BlockDriverAIOCB *paio_submit(BlockDriverState *bs, int 
> fd,
>          return NULL;
>      acb->aio_type = type;
>      acb->aio_fildes = fd;
> -    acb->ev_signo = SIGUSR2;
>  
>      if (qiov) {
>          acb->aio_iov = qiov->iov;
> @@ -651,7 +647,6 @@ BlockDriverAIOCB *paio_ioctl(BlockDriverState *bs, int fd,
>          return NULL;
>      acb->aio_type = QEMU_AIO_IOCTL;
>      acb->aio_fildes = fd;
> -    acb->ev_signo = SIGUSR2;
>      acb->aio_offset = 0;
>      acb->aio_ioctl_buf = buf;
>      acb->aio_ioctl_cmd = req;
> @@ -665,7 +660,6 @@ BlockDriverAIOCB *paio_ioctl(BlockDriverState *bs, int fd,
>  
>  int paio_init(void)
>  {
> -    struct sigaction act;
>      PosixAioState *s;
>      int fds[2];
>      int ret;
> @@ -675,11 +669,6 @@ int paio_init(void)
>  
>      s = g_malloc(sizeof(PosixAioState));
>  
> -    sigfillset(&act.sa_mask);
> -    act.sa_flags = 0; /* do not restart syscalls to interrupt select() */
> -    act.sa_handler = aio_signal_handler;
> -    sigaction(SIGUSR2, &act, NULL);
> -
>      s->first_aio = NULL;
>      if (qemu_pipe(fds) == -1) {
>          fprintf(stderr, "failed to create pipe\n");