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

[Max Reitz]

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

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

test.rb
Description: application/ruby