From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56427) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YROvx-0003o5-HJ for qemu-devel@nongnu.org; Fri, 27 Feb 2015 12:42:42 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YROvt-0004dz-Ao for qemu-devel@nongnu.org; Fri, 27 Feb 2015 12:42:41 -0500 Sender: Paolo Bonzini Message-ID: <54F0AC87.3040707@redhat.com> Date: Fri, 27 Feb 2015 18:42:31 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1425045947-9271-1-git-send-email-mreitz@redhat.com> In-Reply-To: <1425045947-9271-1-git-send-email-mreitz@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v2] block/vdi: Add locking for parallel requests List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu-block@nongnu.org Cc: Kevin Wolf , qemu-stable@nongnu.org, Stefan Hajnoczi , qemu-devel@nongnu.org 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 > Signed-off-by: Max Reitz > --- > 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; > } > >