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

[Paolo Bonzini <pbonzini@redhat.com>]

Il 23/08/2013 10:32, Asias He ha scritto:
> 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 ?

Yes.

> 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?

They are not used on Windows, no.  qemu-nbd is not compiled, audio uses
audio/audio_win_int.c.

Paolo

>>>> __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);
>>>>>  }
>>>>>
>>>>
>>>
>>
>