From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37738) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YRPLf-00026W-51 for qemu-devel@nongnu.org; Fri, 27 Feb 2015 13:09:19 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YRPLa-00066w-HC for qemu-devel@nongnu.org; Fri, 27 Feb 2015 13:09:14 -0500 Message-ID: <54F0B2C3.1010506@redhat.com> Date: Fri, 27 Feb 2015 13:09:07 -0500 From: Max Reitz MIME-Version: 1.0 References: <1425045947-9271-1-git-send-email-mreitz@redhat.com> <54F0AC87.3040707@redhat.com> In-Reply-To: <54F0AC87.3040707@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit 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: Paolo Bonzini , qemu-block@nongnu.org Cc: Kevin Wolf , qemu-stable@nongnu.org, Stefan Hajnoczi , qemu-devel@nongnu.org On 2015-02-27 at 12:42, Paolo Bonzini wrote: > > 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... Maybe that's an issue, too; but the test case I sent out does 1 MB requests (and it fails), so this shouldn't matter there. > An optimized fix could be to use a CoRwLock, then: Yes, I'm actually already working on that. Max > - 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; >> } >> >>