Re: [Qemu-devel] [PATCH] block: Fix race in gluster_finish_aiocb

[Asias He <asias@redhat.com>]

On Thu, Aug 22, 2013 at 11:51:00AM +0200, Paolo Bonzini wrote:
> Il 22/08/2013 11:50, Asias He ha scritto:
> > On Wed, Aug 21, 2013 at 10:16:02AM +0200, Paolo Bonzini wrote:
> >> Il 21/08/2013 04:02, Asias He ha scritto:
> >>> In block/gluster.c, we have
> >>>
> >>> gluster_finish_aiocb
> >>> {
> >>>    if (retval != sizeof(acb)) {
> >>>       qemu_mutex_lock_iothread(); /* We are in gluster thread context */
> >>>       ...
> >>>       qemu_mutex_unlock_iothread();
> >>>    }
> >>> }
> >>>
> >>> qemu tools, e.g. qemu-img, might race here because
> >>> qemu_mutex_{lock,unlock}_iothread are a nop operation and
> >>> gluster_finish_aiocb is in the gluster thread context.
> >>>
> >>> To fix, we introduce our own mutex for qemu tools.
> >>>
> >>> Signed-off-by: Asias He <asias@redhat.com>
> >>> ---
> >>>  stubs/iothread-lock.c | 11 +++++++++++
> >>>  1 file changed, 11 insertions(+)
> >>>
> >>> diff --git a/stubs/iothread-lock.c b/stubs/iothread-lock.c
> >>> index 5d8aca1..d5c6dec 100644
> >>> --- a/stubs/iothread-lock.c
> >>> +++ b/stubs/iothread-lock.c
> >>> @@ -1,10 +1,21 @@
> >>>  #include "qemu-common.h"
> >>>  #include "qemu/main-loop.h"
> >>>  
> >>> +static QemuMutex qemu_tools_mutex;
> >>> +static pthread_once_t qemu_tools_once = PTHREAD_ONCE_INIT;
> >> 
> >> Doesn't work on Windows, but you can just add
> > 
> > Hmm, Any reasons, why it does not work on Windows?
> 
> There are no pthreads on Windows.
> 
> There is an emulation library, but we're not using it.

This one: http://www.sourceware.org/pthreads-win32 ?

I found other places use pthread_create. e.g.

[qemu]$ git grep pthread_create
audio/audio_pt_int.c:    err = pthread_create (&p->thread, NULL, func,
opaque);
audio/audio_pt_int.c:        efunc = "pthread_create";
clone_func, &info);
qemu-nbd.c:    pthread_create(&show_parts_thread, NULL, show_parts,
device);
qemu-nbd.c:        ret = pthread_create(&client_thread, NULL,
nbd_client_thread, device);

These bits are also not working on Windows?

> >> __attribute__((__constructor__)) to qemu_tools_mutex_init.
> > 
> > __attribute__((__constructor__)) works on Windows?
> 
> Yes, it is part of the runtime library's support for C++.  We use it
> already, see include/qemu/module.h.

Hmm, nice. 

> Paolo
> 
> >> Paolo
> >>
> >>> +static void qemu_tools_mutex_init(void)
> >>> +{
> >>> +    qemu_mutex_init(&qemu_tools_mutex);
> >>> +}
> >>> +
> >>>  void qemu_mutex_lock_iothread(void)
> >>>  {
> >>> +    pthread_once(&qemu_tools_once, qemu_tools_mutex_init);
> >>> +    qemu_mutex_lock(&qemu_tools_mutex);
> >>>  }
> >>>  
> >>>  void qemu_mutex_unlock_iothread(void)
> >>>  {
> >>> +    qemu_mutex_unlock(&qemu_tools_mutex);
> >>>  }
> >>>
> >>
> > 
> 

-- 
Asias