From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:36085) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QpF8h-0002yY-5x for qemu-devel@nongnu.org; Fri, 05 Aug 2011 03:48:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QpF8g-0006PA-57 for qemu-devel@nongnu.org; Fri, 05 Aug 2011 03:48:15 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55539) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QpF8f-0006P5-QK for qemu-devel@nongnu.org; Fri, 05 Aug 2011 03:48:14 -0400 Message-ID: <4E3BA0EB.9050604@redhat.com> Date: Fri, 05 Aug 2011 09:51:07 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <1312474673-2705-1-git-send-email-kwolf@redhat.com> <1312475099-3323-1-git-send-email-kwolf@redhat.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 0.15.0] qcow2: Fix L1 table size after bdrv_snapshot_goto List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Frediano Ziglio Cc: stefanha@gmail.com, qemu-devel@nongnu.org, hahn@univention.de Am 05.08.2011 08:35, schrieb Frediano Ziglio: > 2011/8/4 Kevin Wolf : >> When loading an internal snapshot whose L1 table is smaller than the current L1 >> table, the size of the current L1 would be shrunk to the snapshot's L1 size in >> memory, but not on disk. This lead to incorrect refcount updates and eventuelly >> to image corruption. >> >> Instead of writing the new L1 size to disk, this simply retains the bigger L1 >> size that is currently in use and makes sure that the unused part is zeroed. >> >> Signed-off-by: Kevin Wolf >> --- >> >> And the moment you send it out, you notice that it's wrong... *sigh* >> >> v2: >> - Check for s->l1_size > sn->l1_size in order to avoid disasters... >> >> Philipp, I think this should fix your corruption. Please give it a try. >> >> Anthony, this must go into 0.15. Given the short time until -rc2, do you prefer >> to pick it up directly or should I send a pull request tomorrow? The patch >> looks obvious, is tested with the given testcase and survives a basic >> qemu-iotests run (though qemu-iotests doesn't exercise snapshots a lot) >> >> Stefan, please review :-) >> >> block/qcow2-snapshot.c | 5 ++++- >> 1 files changed, 4 insertions(+), 1 deletions(-) >> >> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c >> index 74823a5..6972e66 100644 >> --- a/block/qcow2-snapshot.c >> +++ b/block/qcow2-snapshot.c >> @@ -330,8 +330,11 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id) >> if (qcow2_grow_l1_table(bs, sn->l1_size, true) < 0) >> goto fail; >> >> - s->l1_size = sn->l1_size; >> + if (s->l1_size > sn->l1_size) { >> + memset(s->l1_table + sn->l1_size, 0, s->l1_size - sn->l1_size); >> + } >> l1_size2 = s->l1_size * sizeof(uint64_t); >> + >> /* copy the snapshot l1 table to the current l1 table */ >> if (bdrv_pread(bs->file, sn->l1_table_offset, >> s->l1_table, l1_size2) != l1_size2) >> -- >> 1.7.6 >> > > This patch looked odd at first sight. First a qcow2_grow_l1_table is > called to shrink L1 so perhaps should be qcow2_resize_l1_table. No, it doesn't shrink the table: if (min_size <= s->l1_size) return 0; > Perhaps also it would be better to clean entries in > qcow2_grow_l1_table instead of qcow2_snapshot_goto to avoid same > problem in different calls to qcow2_grow_l1_table. The other oddity > (still to understand) is: why does some code use l1_table above > l1_size ?? Which code do you mean specifically? Kevin