Re: [Qemu-devel] [PATCH v2] block/vdi: Add locking for parallel requests

[Max Reitz <mreitz@redhat.com>]

On 2015-02-27 at 11:57, Stefan Hajnoczi wrote:
> On Fri, Feb 27, 2015 at 09:05:47AM -0500, Max Reitz wrote:
>> Concurrently modifying the bmap does not seem to be a good idea; this patch adds
>> a lock for it. See https://bugs.launchpad.net/qemu/+bug/1422307 for what
>> can go wrong without.
>>
>> Cc: qemu-stable <qemu-stable@nongnu.org>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> v2:
>> - Make the mutex cover vdi_co_write() completely [Kevin]
>> - Add a TODO comment [Kevin]
>> ---
>>   block/vdi.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/block/vdi.c b/block/vdi.c
>> index 74030c6..f5f42ef 100644
>> --- a/block/vdi.c
>> +++ b/block/vdi.c
>> @@ -51,6 +51,7 @@
>>   
>>   #include "qemu-common.h"
>>   #include "block/block_int.h"
>> +#include "block/coroutine.h"
>>   #include "qemu/module.h"
>>   #include "migration/migration.h"
>>   
>> @@ -196,6 +197,8 @@ typedef struct {
>>       /* VDI header (converted to host endianness). */
>>       VdiHeader header;
>>   
>> +    CoMutex bmap_lock;
>> +
>>       Error *migration_blocker;
>>   } BDRVVdiState;
>>   
>> @@ -498,6 +501,8 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
>>           goto fail_free_bmap;
>>       }
>>   
>> +    qemu_co_mutex_init(&s->bmap_lock);
>> +
>>       /* Disable migration when vdi images are used */
>>       error_set(&s->migration_blocker,
>>                 QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
>> @@ -607,6 +612,9 @@ static int vdi_co_write(BlockDriverState *bs,
>>   
>>       logout("\n");
>>   
>> +    /* TODO: Figure out why this is necessary */
>> +    qemu_co_mutex_lock(&s->bmap_lock);
> If we don't know why bmap_lock works, it would be more approprate to
> take the same approach as VMDK and VHDX where there is a simply s->lock
> that protects all reads and writes.  That way we know for sure there is
> no parallel I/O going on.
>
> (Since the problem is not understood, maybe reads in parallel with
> writes could also cause problems.  Better to really do a coarse lock
> instead of just bmap_lock in write.)

OK, will do.

Max