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

[Stefan Weil]

Am 27.02.2015 um 17:57 schrieb Stefan Hajnoczi:
> 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 <address@hidden>
>> Signed-off-by: Max Reitz <address@hidden>
>> ---
>> v2:
>> - Make the mutex cover vdi_co_write() completely [Kevin]
>> - Add a TODO comment [Kevin]
[...]
>> 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.)
>>
>> Stefan

block/vdi.c was never written for multi-threaded access, see my comment
in the header of block/vdi.c:

 * The code is not thread safe (missing locks for changes in header and
 * block table, no problem with current QEMU).

This was true in the past, but obviously later multi-threaded access was
introduced for QEMU. Locking was added for qcow2 and other drivers in
2012 and 2013, but never for vdi.

I must admit that I don't know which parts of the block filesystem
drivers potentially run in parallel threads. Ideally there would be one
or more test cases which test multi-threaded operations and which
trigger a failure with the current vdi code.

If I had a simple test scenario, I could have a look on the problem.

The VMDK approach is fine as an intermediate work around, but please use
conditional compilation to allow easy tests without coarse locks (and
update the comments :-)).

Regards
Stefan (Weil)