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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2120 bytes --]

On 2015-02-27 at 12:25, Stefan Weil wrote:
> 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 <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]
> [...]
>>> 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.

I have one for you. See the attached ruby script.

(If there are no "Pattern verification failed" messages, everything is good)

> 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 :-)).

Will a macro defined in vdi.c be enough?

Max

[-- Attachment #2: test.rb --]
[-- Type: application/x-ruby, Size: 677 bytes --]