On 27/02/2015 15:05, 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]
I think I know what the bug is. Suppose you have two concurrent writes to a non-allocated block, one at 16K...32K (in bytes) and one at 32K...48K. The first write is enlarged to contain zeros, the second is not. Then you have two writes in flight: 0 zeros ... zeros 16K data1 ... data1 32K zeros data2 ... zeros data2 48K zeros ... zeros 64K And the contents of 32K...48K are undefined. If the above diagnosis is correct, I'm not even sure why Max's v1 patch worked... An optimized fix could be to use a CoRwLock, then: - take it shared (read) around the write in the "VDI_IS_ALLOCATED(bmap_entry)" path - take it exclusive (write) around the write in the "!VDI_IS_ALLOCATED(bmap_entry)" path Paolo > --- > 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); > + > while (ret >= 0 && nb_sectors > 0) { > block_index = sector_num / s->block_sectors; > sector_in_block = sector_num % s->block_sectors; > @@ -656,6 +664,7 @@ static int vdi_co_write(BlockDriverState *bs, > > logout("finished data write\n"); > if (ret < 0) { > + qemu_co_mutex_unlock(&s->bmap_lock); > return ret; > } > > @@ -674,6 +683,7 @@ static int vdi_co_write(BlockDriverState *bs, > block = NULL; > > if (ret < 0) { > + qemu_co_mutex_unlock(&s->bmap_lock); > return ret; > } > > @@ -690,6 +700,7 @@ static int vdi_co_write(BlockDriverState *bs, > ret = bdrv_write(bs->file, offset, base, n_sectors); > } > > + qemu_co_mutex_unlock(&s->bmap_lock); > return ret; > } > >