From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53522) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VEHeP-0006hM-K1 for qemu-devel@nongnu.org; Tue, 27 Aug 2013 07:41:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VEHeJ-0006c7-FX for qemu-devel@nongnu.org; Tue, 27 Aug 2013 07:41:33 -0400 Received: from mx1.redhat.com ([209.132.183.28]:14826) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VEHeJ-0006bv-7Z for qemu-devel@nongnu.org; Tue, 27 Aug 2013 07:41:27 -0400 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r7RBfQSF029275 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 27 Aug 2013 07:41:26 -0400 Message-ID: <521C9064.2030101@redhat.com> Date: Tue, 27 Aug 2013 13:41:24 +0200 From: Max Reitz MIME-Version: 1.0 References: <1377522260-16676-1-git-send-email-mreitz@redhat.com> <1377522260-16676-4-git-send-email-mreitz@redhat.com> <20130827113205.GD648@dhcp-200-207.str.redhat.com> In-Reply-To: <20130827113205.GD648@dhcp-200-207.str.redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/5] qcow2: Employ metadata overlap checks List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: qemu-devel@nongnu.org, Stefan Hajnoczi Am 27.08.2013 13:32, schrieb Kevin Wolf: > Am 26.08.2013 um 15:04 hat Max Reitz geschrieben: >> The pre-write overlap check function is now called before most of the >> qcow2 writes (aborting it on collision or other error). >> >> Signed-off-by: Max Reitz >> --- >> block/qcow2-cache.c | 17 +++++++++++++++++ >> block/qcow2-cluster.c | 23 +++++++++++++++++++++++ >> block/qcow2-snapshot.c | 24 ++++++++++++++++++++++++ >> block/qcow2.c | 38 +++++++++++++++++++++++++++++++++++++- >> 4 files changed, 101 insertions(+), 1 deletion(-) >> @@ -368,6 +384,13 @@ static int coroutine_fn copy_sectors(BlockDriverState *bs, >> &s->aes_encrypt_key); >> } >> >> + ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_DEFAULT, >> + ((cluster_offset >> 9) + n_start) << 9, n * BDRV_SECTOR_SIZE); > Looks a bit overcomplicated, I'd like something like this better: > > cluster_offset + n_start * BDRV_SECTOR_SIZE Yes, but this wouldn't correspond with the write call if (cluster_offset & ((1 << 9) - 1)) != 0. ;-) Basically, I just wanted it to match exactly the write command. > >> + if (ret) { >> + ret = (ret < 0) ? ret : -EIO; > I wonder whether the -EIO logic should be moved into > qcow2_pre_write_overlap_check(). Currently each single caller seems to > have this check. Seems reasonable. I didn't want to prevent the caller from receiving information about the exact overlap, but that could be achieved through an optional result pointer as well, I think. > >> + goto out; >> + } >> + >> BLKDBG_EVENT(bs->file, BLKDBG_COW_WRITE); >> ret = bdrv_co_writev(bs->file, (cluster_offset >> 9) + n_start, n, &qiov); >> if (ret < 0) { >> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c >> index 0caac90..6f69ecc 100644 >> --- a/block/qcow2-snapshot.c >> +++ b/block/qcow2-snapshot.c >> @@ -189,6 +189,15 @@ static int qcow2_write_snapshots(BlockDriverState *bs) >> return ret; >> } >> >> + /* The snapshot list position has not yet been updated, so these clusters >> + * must indeed be completely free */ >> + ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_DEFAULT, >> + offset, s->nb_snapshots * sizeof(h)); >> + if (ret) { >> + return (ret < 0) ? ret : -EIO; >> + } > This doesn't check the full size. snapshots_size should have the right > value. Yes, you're right. Max