From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55331) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XgZCf-0001pR-3D for qemu-devel@nongnu.org; Tue, 21 Oct 2014 09:10:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XgZCa-00088H-JL for qemu-devel@nongnu.org; Tue, 21 Oct 2014 09:10:21 -0400 Message-ID: <54465B2C.1070603@redhat.com> Date: Tue, 21 Oct 2014 15:10:04 +0200 From: Max Reitz MIME-Version: 1.0 References: <201410211934299401424@sangfor.com>, <54464B0F.5070500@redhat.com> <201410212108441493881@sangfor.com> In-Reply-To: <201410212108441493881@sangfor.com> Content-Type: text/plain; charset=gbk; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [Qemu-stable] [PATCH v2] snapshot: use local variable tobdrv_pwrite_sync L1 table List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Zhang Haoyu , qemu-devel Cc: Kevin Wolf , qemu-stable , Stefan Hajnoczi On 2014-10-21 at 15:08, Zhang Haoyu wrote: >>> Use local variable to bdrv_pwrite_sync L1 table, >>> needless to make conversion of cached L1 table between >>> big-endian and host style. >>> >>> Signed-off-by: Zhang Haoyu >>> --- >>> v1 -> v2: >>> - remove the superflous assignment, l1_table = NULL; >>> - replace 512 with BDRV_SECTOR_SIZE, and align_offset with ROUND_UP >>> - remove needless check of if (l1_table) before g_free(l1_table) >>> >>> block/qcow2-refcount.c | 24 +++++++----------------- >>> 1 file changed, 7 insertions(+), 17 deletions(-) >>> >>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c >>> index 2bcaaf9..29a916a 100644 >>> --- a/block/qcow2-refcount.c >>> +++ b/block/qcow2-refcount.c >>> @@ -881,14 +881,17 @@ 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; >>> 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 = g_try_malloc0(ROUND_UP(l1_size2, BDRV_SECTOR_SIZE)); >> I'm sorry, but I just now realized that we don't even need the 0 variant >> here. We are never accessing any element beyond l1_table[l1_size - 1], >> and all elements from 0 until l1_size - 1 are overwritten by >> bdrv_pread() or memcpy(). Therefore we can simply use >> qemu_try_blockalign(bs->file, l1_size2) here. >> > OK, I will replace g_try_malloc0(ROUND_UP(l1_size2, BDRV_SECTOR_SIZE)) with > qemu_try_blockalign(bs, ROUND_UP(l1_size2, BDRV_SECTOR_SIZE)). You can actually omit the ROUND_UP(). There are no accesses beyond l1_table[l1_size - 1], if I'm not mistaken. Max >> (and we should be using qemu_{try_,}blockalign() instead of >> g_{try_,}malloc{0,}() whenever possible for performance reasons) >> >>> + if (l1_size2 && l1_table == NULL) { >>> + ret = -ENOMEM; >>> + goto fail; >>> + } >>> >>> s->cache_discards = true; >>> >>> @@ -896,13 +899,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; >>> @@ -912,8 +908,7 @@ 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); >>> } >>> >>> for(i = 0; i < l1_size; i++) { >>> @@ -1055,13 +1050,8 @@ 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]); >>> - } >> But I realized something even more important as well: If you are >> modifying the current L1 table, you need to copy the result back to >> s->l1_table. >> > My fault. >> Sorry for having missed these in my previous review. >> >> Max >> >>> } >>> - if (l1_allocated) >>> - g_free(l1_table); >>> + g_free(l1_table); >>> return ret; >>> } >>>