[Qemu-devel] Re: [PATCH] qemu: work around for "posix-aio-compat"

[Michael S. Tsirkin]

On Thu, Oct 08, 2009 at 10:37:40PM +0200, Michael S. Tsirkin wrote:
> With commit ee3993069ff55fa6f1c64daf1e09963e340db8e4,
> "posix-aio-compat: avoid signal race when spawning a thread"
> winxp installation on a raw format file fails
> during disk format, with a message "your
> disk may be damaged".
> 
> This commit moved signal mask from aio thread to creating thread.
> It turns out if we keep the mask in aio thread as well, the problem
> disappears. It should not be needed, but since this is harmless, let's
> keep it around until someone inclined to debug pthread library internals
> can check this issue.
> 
> While we are at it, convert sigprocmask to pthread_sigmask
> as per posix.
> 
> Signed-off-by: Michael S. Tsirkin <address@hidden>
> 
> 
> For the benefit of whoever tries to debug this any deeper here's
> documentation on how to reproduce the issue to be saved in git logs
> (need to tweak paths obviously):
> 
> The test was done on FC11, 32 bit userspace, 64 bit kernel 2.6.31.

Vanilla kernel from FC11 does the same btw.

> 
> ./configure --prefix=/scm/qemu-kvm-debug --target-list=x86_64-softmmu \
>   --enable-kvm
> make -j 8 && make install
> rm /scm/images/winxp-bisect.raw
> 
> /scm/qemu-kvm-debug/bin/qemu-img create -f raw \
> /scm/images/winxp-bisect.raw 2G
> 
> /scm/qemu-kvm-debug/bin/qemu-system-x86_64 -enable-kvm -cdrom \
> /home/mst/en_windows_xp_professional_with_service_pack_3_x86_cd_x14-80428.iso 
> \
> /scm/images/winxp-bisect.raw
> 
> Select format to NTFS.
> ---
> 
> OK, I'm out of time with this issue, let's apply a work-around
> until someone interested comes around?
> 
>  posix-aio-compat.c |   14 ++++++++++++--
> 
> diff --git a/posix-aio-compat.c b/posix-aio-compat.c
> index 400d898..4abbe3a 100644
> --- a/posix-aio-compat.c
> +++ b/posix-aio-compat.c
> @@ -301,6 +301,16 @@ static size_t handle_aiocb_rw(struct qemu_paiocb *aiocb)
>  static void *aio_thread(void *unused)
>  {
>      pid_t pid;
> +    sigset_t set;
> +
> +    /* block all signals */
> +    /* Should not be necessary as we should inherit mask
> +     * from creating thread. However, without this,
> +     * on FC11, using raw file, WinXP installation fails during disk format
> +     * saying disk was damaged. pthread library bug?
> +     * */
> +    if (sigfillset(&set)) die("sigfillset");
> +    if (pthread_sigmask(SIG_BLOCK, &set, NULL)) die("pthread_sigmask");

By the way, things seem to work even when I comment out pthread_sigmask:
just sigfillset does it. Seems pretty wild.

>  
>      pid = getpid();
>  
> @@ -371,11 +381,11 @@ static void spawn_thread(void)
>  
>      /* block all signals */
>      if (sigfillset(&set)) die("sigfillset");
> -    if (sigprocmask(SIG_SETMASK, &set, &oldset)) die("sigprocmask");
> +    if (pthread_sigmask(SIG_SETMASK, &set, &oldset)) die("pthread_sigmask");
>  
>      thread_create(&thread_id, &attr, aio_thread, NULL);
>  
> -    if (sigprocmask(SIG_SETMASK, &oldset, NULL)) die("sigprocmask restore");
> +    if (pthread_sigmask(SIG_SETMASK, &oldset, NULL)) die("pthread_sigmask 
> restore");
>  }

As I said before, just this second hunk alone does not help.

>  static void qemu_paio_submit(struct qemu_paiocb *aiocb)
> -- 
> 1.6.5.rc2