From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38791) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YNzRL-0004GL-Vy for qemu-devel@nongnu.org; Wed, 18 Feb 2015 02:53:01 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YNzRG-0003ny-Sz for qemu-devel@nongnu.org; Wed, 18 Feb 2015 02:52:59 -0500 Received: from mail-we0-f176.google.com ([74.125.82.176]:43261) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YNzRG-0003nu-Mo for qemu-devel@nongnu.org; Wed, 18 Feb 2015 02:52:54 -0500 Received: by wesu56 with SMTP id u56so2994536wes.10 for ; Tue, 17 Feb 2015 23:52:54 -0800 (PST) Sender: Paolo Bonzini Message-ID: <54E444D1.4090704@redhat.com> Date: Wed, 18 Feb 2015 08:52:49 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1424208819-10306-1-git-send-email-mreitz@redhat.com> In-Reply-To: <1424208819-10306-1-git-send-email-mreitz@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH] block/vdi: Fix locking for parallel requests List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz , qemu-devel@nongnu.org Cc: Kevin Wolf , Stefan Weil , Stefan Hajnoczi On 17/02/2015 22:33, Max Reitz wrote: > Concurrently modifying the bmap is not a good idea; 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 > --- > block/vdi.c | 23 ++++++++++++++++++++--- > 1 file changed, 20 insertions(+), 3 deletions(-) > > diff --git a/block/vdi.c b/block/vdi.c > index 74030c6..c5ff428 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, > @@ -619,6 +624,9 @@ static int vdi_co_write(BlockDriverState *bs, > n_sectors, sector_num); > > /* prepare next AIO request */ > + if (!block) { > + qemu_co_mutex_lock(&s->bmap_lock); > + } > bmap_entry = le32_to_cpu(s->bmap[block_index]); > if (!VDI_IS_ALLOCATED(bmap_entry)) { > /* Allocate new block and write to it. */ > @@ -641,9 +649,13 @@ static int vdi_co_write(BlockDriverState *bs, > (s->block_sectors - n_sectors - sector_in_block) * SECTOR_SIZE); > ret = bdrv_write(bs->file, offset, block, s->block_sectors); > } else { > - uint64_t offset = s->header.offset_data / SECTOR_SIZE + > - (uint64_t)bmap_entry * s->block_sectors + > - sector_in_block; > + uint64_t offset; > + > + qemu_co_mutex_unlock(&s->bmap_lock); This qemu_co_mutex_unlock should only be done "if (!block)", because you can have two iterations of the loop, the first going down the "then" and the second going down the "else". In that case you must not give away the lock, because the lock will be needed when you write the bitmap. > + > + offset = s->header.offset_data / SECTOR_SIZE + > + (uint64_t)bmap_entry * s->block_sectors + > + sector_in_block; > ret = bdrv_write(bs->file, offset, buf, n_sectors); > } > > @@ -656,6 +668,9 @@ static int vdi_co_write(BlockDriverState *bs, > > logout("finished data write\n"); > if (ret < 0) { > + if (block) { > + qemu_co_mutex_unlock(&s->bmap_lock); > + } > return ret; > } > > @@ -688,6 +703,8 @@ static int vdi_co_write(BlockDriverState *bs, > logout("will write %u block map sectors starting from entry %u\n", > n_sectors, bmap_first); > ret = bdrv_write(bs->file, offset, base, n_sectors); > + > + qemu_co_mutex_unlock(&s->bmap_lock); > } > > return ret; >