From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54894) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XgZBL-0000D2-GR for qemu-devel@nongnu.org; Tue, 21 Oct 2014 09:09:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XgZBG-0007OB-VV for qemu-devel@nongnu.org; Tue, 21 Oct 2014 09:08:59 -0400 Date: Tue, 21 Oct 2014 21:08:46 +0800 From: "Zhang Haoyu" References: <201410211934299401424@sangfor.com>, <54464B0F.5070500@redhat.com> Message-ID: <201410212108441493881@sangfor.com> Mime-Version: 1.0 Content-Type: text/plain; charset="gb2312" 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: Max Reitz , qemu-devel Cc: Kevin Wolf , qemu-stable , Stefan Hajnoczi >> 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)). >(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; >> } >>