From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43637) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XoDMN-0001ir-UY for qemu-devel@nongnu.org; Tue, 11 Nov 2014 10:28:04 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XoDMJ-0006RG-Dh for qemu-devel@nongnu.org; Tue, 11 Nov 2014 10:27:59 -0500 Received: from mx1.redhat.com ([209.132.183.28]:47043) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XoDMJ-0006RA-71 for qemu-devel@nongnu.org; Tue, 11 Nov 2014 10:27:55 -0500 From: Max Reitz Date: Tue, 11 Nov 2014 16:27:51 +0100 Message-Id: <1415719671-16257-1-git-send-email-mreitz@redhat.com> Subject: [Qemu-devel] [PATCH v6] qcow2: Buffer L1 table in snapshot refcount update List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: Kevin Wolf , Zhang Haoyu , Stefan Hajnoczi , Max Reitz From: Zhang Haoyu Buffer the active L1 table in qcow2_update_snapshot_refcount() in order to prevent in-place conversion of the L1 table buffer in the BDRVQcowState to big endian and back, which would lead to data corruption if that buffer was accessed concurrently. This should not happen but better being safe than sorry. Signed-off-by: Zhang Haoyu Signed-off-by: Max Reitz --- v6 for "snapshot: use local variable to bdrv_pwrite_sync L1 table" (I changed the commit message wording to make it more clear what this patch does and why we want it). Changes in v6: - Only copy the local buffer back into s->l1_table if we are indeed accessing the local L1 table - Use qemu_vfree() instead of g_free() --- block/qcow2-refcount.c | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c index 9afdb40..c0c4a50 100644 --- a/block/qcow2-refcount.c +++ b/block/qcow2-refcount.c @@ -877,14 +877,18 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, { BDRVQcowState *s = bs->opaque; uint64_t *l1_table, *l2_table, l2_offset, offset, l1_size2; - bool l1_allocated = false; + bool active_l1 = false; int64_t old_offset, old_l2_offset; int i, j, l1_modified = 0, nb_csectors, refcount; int ret; l2_table = NULL; - l1_table = NULL; l1_size2 = l1_size * sizeof(uint64_t); + l1_table = qemu_try_blockalign(bs->file, l1_size2); + if (l1_table == NULL) { + ret = -ENOMEM; + goto fail; + } s->cache_discards = true; @@ -892,13 +896,6 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, * 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) { - l1_table = g_try_malloc0(align_offset(l1_size2, 512)); - if (l1_size2 && l1_table == NULL) { - ret = -ENOMEM; - goto fail; - } - l1_allocated = true; - ret = bdrv_pread(bs->file, l1_table_offset, l1_table, l1_size2); if (ret < 0) { goto fail; @@ -908,8 +905,8 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs, be64_to_cpus(&l1_table[i]); } else { assert(l1_size == s->l1_size); - l1_table = s->l1_table; - l1_allocated = false; + memcpy(l1_table, s->l1_table, l1_size2); + active_l1 = true; } for(i = 0; i < l1_size; i++) { @@ -1051,13 +1048,14 @@ fail: } ret = bdrv_pwrite_sync(bs->file, l1_table_offset, l1_table, l1_size2); - - for (i = 0; i < l1_size; i++) { - be64_to_cpus(&l1_table[i]); + if (active_l1 && ret == 0) { + for (i = 0; i < l1_size; i++) { + be64_to_cpus(&l1_table[i]); + } + memcpy(s->l1_table, l1_table, l1_size2); } } - if (l1_allocated) - g_free(l1_table); + qemu_vfree(l1_table); return ret; } -- 1.9.3