From 78cd014231b0f781643fb980f5cd2103659da49c Mon Sep 17 00:00:00 2001 Message-Id: <78cd014231b0f781643fb980f5cd2103659da49c.1368098699.git.minovotn@redhat.com> In-Reply-To: <618a4b91ddb04b21f9dc0c1defe7693fb7cc1748.1368098699.git.minovotn@redhat.com> References: <618a4b91ddb04b21f9dc0c1defe7693fb7cc1748.1368098699.git.minovotn@redhat.com> From: Kevin Wolf Date: Fri, 5 Apr 2013 19:44:48 +0200 Subject: [PATCH 09/24] qcow2: Fix order of refcount updates in qcow2_snapshot_goto RH-Author: Kevin Wolf Message-id: <1365191091-25631-10-git-send-email-kwolf@redhat.com> Patchwork-id: 50169 O-Subject: [RHEL-6.5 qemu-kvm PATCH 09/12] qcow2: Fix order of refcount updates in qcow2_snapshot_goto Bugzilla: 796011 RH-Acked-by: Stefan Hajnoczi RH-Acked-by: Laszlo Ersek RH-Acked-by: Fam Zheng Bugzilla: 796011 The refcount updates must be moved so that in the worst case we can get cluster leaks, but refcounts may never be too low. Signed-off-by: Kevin Wolf Reviewed-by: Stefan Hajnoczi (cherry picked from commit 43a0cac4658bbee9c9e84554712a94daa092c1cd) Signed-off-by: Kevin Wolf --- block/qcow2-refcount.c | 7 +++++- block/qcow2-snapshot.c | 61 ++++++++++++++++++++++++++++++++++++-------------- 2 files changed, 50 insertions(+), 18 deletions(-) Signed-off-by: Michal Novotny --- block/qcow2-refcount.c | 7 +++++- block/qcow2-snapshot.c | 61 ++++++++++++++++++++++++++++++++++++-------------- 2 files changed, 50 insertions(+), 18 deletions(-) diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c index 69edb81..82b747b 100644 --- a/block/qcow2-refcount.c +++ b/block/qcow2-refcount.c @@ -718,6 +718,10 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, l2_table = NULL; l1_table = NULL; l1_size2 = l1_size * sizeof(uint64_t); + + /* WARNING: qcow2_snapshot_goto relies on this function not using the + * l1_table_offset when it is the current s->l1_table_offset! Be careful + * when changing this! */ if (l1_table_offset != s->l1_table_offset) { if (l1_size2 != 0) { l1_table = g_malloc0(align_offset(l1_size2, 512)); @@ -837,7 +841,8 @@ fail: qcow2_cache_set_writethrough(bs, s->refcount_block_cache, old_refcount_writethrough); - if (l1_modified) { + /* Update L1 only if it isn't deleted anyway (addend = -1) */ + if (addend >= 0 && l1_modified) { for(i = 0; i < l1_size; i++) cpu_to_be64s(&l1_table[i]); if (bdrv_pwrite_sync(bs->file, l1_table_offset, l1_table, diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c index 445efbe..da60e9f 100644 --- a/block/qcow2-snapshot.c +++ b/block/qcow2-snapshot.c @@ -384,6 +384,7 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id) int i, snapshot_index; int cur_l1_bytes, sn_l1_bytes; int ret; + uint64_t *sn_l1_table = NULL; /* Search the snapshot */ snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_id); @@ -392,14 +393,6 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id) } sn = &s->snapshots[snapshot_index]; - /* Decrease refcount of clusters of current L1 table. - * FIXME This is too early! */ - ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, - s->l1_size, -1); - if (ret < 0) { - goto fail; - } - /* * Make sure that the current L1 table is big enough to contain the whole * L1 table of the snapshot. If the snapshot L1 table is smaller, the @@ -413,33 +406,66 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id) cur_l1_bytes = s->l1_size * sizeof(uint64_t); sn_l1_bytes = sn->l1_size * sizeof(uint64_t); - if (cur_l1_bytes > sn_l1_bytes) { - memset(s->l1_table + sn->l1_size, 0, cur_l1_bytes - sn_l1_bytes); - } - /* * Copy the snapshot L1 table to the current L1 table. * * Before overwriting the old current L1 table on disk, make sure to * increase all refcounts for the clusters referenced by the new one. + * Decrease the refcount referenced by the old one only when the L1 + * table is overwritten. */ - ret = bdrv_pread(bs->file, sn->l1_table_offset, s->l1_table, sn_l1_bytes); + sn_l1_table = g_malloc0(cur_l1_bytes); + + ret = bdrv_pread(bs->file, sn->l1_table_offset, sn_l1_table, sn_l1_bytes); + if (ret < 0) { + goto fail; + } + + ret = qcow2_update_snapshot_refcount(bs, sn->l1_table_offset, + sn->l1_size, 1); if (ret < 0) { goto fail; } - ret = bdrv_pwrite_sync(bs->file, s->l1_table_offset, s->l1_table, + ret = bdrv_pwrite_sync(bs->file, s->l1_table_offset, sn_l1_table, cur_l1_bytes); if (ret < 0) { goto fail; } + /* + * Decrease refcount of clusters of current L1 table. + * + * At this point, the in-memory s->l1_table points to the old L1 table, + * whereas on disk we already have the new one. + * + * qcow2_update_snapshot_refcount special cases the current L1 table to use + * the in-memory data instead of really using the offset to load a new one, + * which is why this works. + */ + ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, + s->l1_size, -1); + + /* + * Now update the in-memory L1 table to be in sync with the on-disk one. We + * need to do this even if updating refcounts failed. + */ for(i = 0;i < s->l1_size; i++) { - be64_to_cpus(&s->l1_table[i]); + s->l1_table[i] = be64_to_cpu(sn_l1_table[i]); } - /* FIXME This is too late! */ - ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 1); + if (ret < 0) { + goto fail; + } + + g_free(sn_l1_table); + sn_l1_table = NULL; + + /* + * Update QCOW_OFLAG_COPIED in the active L1 table (it may have changed + * when we decreased the refcount of the old snapshot. + */ + ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 0); if (ret < 0) { goto fail; } @@ -450,6 +476,7 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id) return 0; fail: + g_free(sn_l1_table); return ret; } -- 1.7.11.7