From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1XguiF-0005TK-Ai for mharc-qemu-trivial@gnu.org; Wed, 22 Oct 2014 08:08:23 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42017) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XguiA-0005Mk-0h for qemu-trivial@nongnu.org; Wed, 22 Oct 2014 08:08:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xgui5-0005UZ-FF for qemu-trivial@nongnu.org; Wed, 22 Oct 2014 08:08:17 -0400 Received: from [58.251.49.30] (port=40554 helo=mail.sangfor.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xguhu-0005Rg-W8; Wed, 22 Oct 2014 08:08:03 -0400 Received: from localhost (mail.sangfor.com [127.0.0.1]) by mail.sangfor.com (Postfix) with ESMTP id 04A5417C0292; Wed, 22 Oct 2014 19:48:46 +0800 (CST) Received: from mail.sangfor.com ([127.0.0.1]) by localhost (mail.sangfor.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id gAiywWaGAyN5; Wed, 22 Oct 2014 19:48:45 +0800 (CST) Received: from vv-PC (unknown [10.10.6.254]) by mail.sangfor.com (Postfix) with ESMTPA id E5DBA17C0296; Wed, 22 Oct 2014 19:48:43 +0800 (CST) Date: Wed, 22 Oct 2014 20:07:53 +0800 From: "Zhang Haoyu" To: "Gonglei" References: <201410221945053926307@sangfor.com>, <54479C3E.1070803@huawei.com> Message-ID: <201410222007512815199@sangfor.com> X-mailer: Foxmail 6, 15, 201, 20 [cn] Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x (no timestamps) [generic] X-Received-From: 58.251.49.30 Cc: qemu-trivial , Kevin Wolf , qemu-devel , Stefan Hajnoczi , Max Reitz Subject: Re: [Qemu-trivial] [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_syncL1 table X-BeenThere: qemu-trivial@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 22 Oct 2014 12:08:22 -0000 >> 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 >> Reviewed-by: Max Reitz >> --- >> v3 -> v4: >> - convert local L1 table to host-style before copy it >> back to s->l1_table >> >> v2 -> v3: >> - replace g_try_malloc0 with qemu_try_blockalign >> - copy the latest local L1 table back to s->l1_table >> after successfully bdrv_pwrite_sync L1 table >> >> 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 | 28 ++++++++++++---------------- >> 1 file changed, 12 insertions(+), 16 deletions(-) >> >> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c >> index 2bcaaf9..3e4050a 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 = qemu_try_blockalign(bs->file, l1_size2); >> + if (l1_size2 && l1_table == NULL) { > >I think this check has a logic problem. If l1_size2 != 0 and l1_table == NULL, >What will happen? > If the condition of "if (l1_size2 && l1_table == NULL)" is true, below code will be performed, s->cache_discards = false; qcow2_process_discards(bs, ret); g_free(l1_table); What's your question? Thanks, Zhang Haoyu >Best regards, >-Gonglei > >> + 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,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 (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); >> + g_free(l1_table); >> return ret; >> } >> From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41984) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xgui0-0005BA-UA for qemu-devel@nongnu.org; Wed, 22 Oct 2014 08:08:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xguhv-0005S1-Q0 for qemu-devel@nongnu.org; Wed, 22 Oct 2014 08:08:08 -0400 Date: Wed, 22 Oct 2014 20:07:53 +0800 From: "Zhang Haoyu" References: <201410221945053926307@sangfor.com>, <54479C3E.1070803@huawei.com> Message-ID: <201410222007512815199@sangfor.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_syncL1 table List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gonglei Cc: qemu-trivial , Kevin Wolf , qemu-devel , Stefan Hajnoczi , Max Reitz >> 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 >> Reviewed-by: Max Reitz >> --- >> v3 -> v4: >> - convert local L1 table to host-style before copy it >> back to s->l1_table >> >> v2 -> v3: >> - replace g_try_malloc0 with qemu_try_blockalign >> - copy the latest local L1 table back to s->l1_table >> after successfully bdrv_pwrite_sync L1 table >> >> 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 | 28 ++++++++++++---------------- >> 1 file changed, 12 insertions(+), 16 deletions(-) >> >> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c >> index 2bcaaf9..3e4050a 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 = qemu_try_blockalign(bs->file, l1_size2); >> + if (l1_size2 && l1_table == NULL) { > >I think this check has a logic problem. If l1_size2 != 0 and l1_table == NULL, >What will happen? > If the condition of "if (l1_size2 && l1_table == NULL)" is true, below code will be performed, s->cache_discards = false; qcow2_process_discards(bs, ret); g_free(l1_table); What's your question? Thanks, Zhang Haoyu >Best regards, >-Gonglei > >> + 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,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 (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); >> + g_free(l1_table); >> return ret; >> } >>