From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55229) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YO6H5-0002ux-Ml for qemu-devel@nongnu.org; Wed, 18 Feb 2015 10:10:52 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YO6H4-0004g0-C5 for qemu-devel@nongnu.org; Wed, 18 Feb 2015 10:10:51 -0500 Received: from mx1.redhat.com ([209.132.183.28]:42289) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YO6H4-0004fp-5G for qemu-devel@nongnu.org; Wed, 18 Feb 2015 10:10:50 -0500 Message-ID: <54E4AB77.2010706@redhat.com> Date: Wed, 18 Feb 2015 10:10:47 -0500 From: Max Reitz MIME-Version: 1.0 References: <1424208819-10306-1-git-send-email-mreitz@redhat.com> <54E444D1.4090704@redhat.com> In-Reply-To: <54E444D1.4090704@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit 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: Paolo Bonzini , qemu-devel@nongnu.org Cc: Kevin Wolf , Stefan Weil , Stefan Hajnoczi On 2015-02-18 at 02:52, Paolo Bonzini wrote: > > 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. Oh, right, and also there's an if (ret < 0) { return ret; } between the last two hunks of this patch which needs an additional unlock(). Max > >> + >> + 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; >>