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-sta...@nongnu.org> > Signed-off-by: Max Reitz <mre...@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.) Stefan
pgp7LjNaaoemZ.pgp
Description: PGP signature
