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

[Max Reitz]

On 2015-02-18 at 07:39, Kevin Wolf wrote:
> Am 17.02.2015 um 22:33 hat Max Reitz geschrieben:
> > Concurrently modifying the bmap is not a good idea;
> Why? I mean, the fact that this fixes something for you probably means
> that there really is some piece of local state that is invalidated by
> concurrent writes, but it's not obvious to me what it is.

One thing I can see is the following: The write operations for header 
and bmap are separate. Therefore, writing the header may succeed; then, 
when writing the bmap, something may yield and another instance of 
vdi_co_write() gets to run. That instance is still in the while loop, 
allocating a new block.

It modifies s->bmap[] and increases s->header.blocks_allocated; then 
tries to write. That yields and the first coroutine wakes up. It writes 
the bmap to the disk, which is now inconsistent with 
s->header.blocks_allocated, though. So that looks to me like it may 
break, although it in fact doesn't seem to be responsible for the 
problem at hand.

For that problem, always doing the unlock() after the bdrv_write() in 
the "if (!VDI_IS_ALLOCATED(bmap_entry))" path seems enough; although I 
cannot really explain how coroutines yielding from writing the data 
block interferes with other coroutines allocating new blocks.

However, in case you agree with my reasoning for the possible corruption 
(which I did not try to produce), this patch (which would fix that) 
would also happen to prevent the really appearing VDI problems, so...

Max

> What could obviously happen is that metadata is written before the data
> is on the disk, but as we don't support backing files for VDI, this is
> irrelvant. (And if it were relevant, it stil wouldn't be fixed by your
> patch because the driver never flushes.)
>
> > this patch adds a
> > lock for it. See https://bugs.launchpad.net/qemu/+bug/1422307 for what
> > can go wrong without.
> >
> > Signed-off-by: Max Reitz <address@hidden>
> Kevin