From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1XgwJg-00089A-Vm for mharc-qemu-trivial@gnu.org; Wed, 22 Oct 2014 09:51:08 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43098) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XgwJb-00086h-0M for qemu-trivial@nongnu.org; Wed, 22 Oct 2014 09:51:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XgwJW-0000Gm-Qu for qemu-trivial@nongnu.org; Wed, 22 Oct 2014 09:51:02 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41767) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XgwJW-0000Gh-K1; Wed, 22 Oct 2014 09:50:58 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s9MDouhT004502 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Wed, 22 Oct 2014 09:50:56 -0400 Received: from dresden.str.redhat.com (dhcp-192-247.str.redhat.com [10.33.192.247]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s9MDoqBg032272 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Wed, 22 Oct 2014 09:50:53 -0400 Message-ID: <5447B63C.70603@redhat.com> Date: Wed, 22 Oct 2014 15:50:52 +0200 From: Max Reitz User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: Zhang Haoyu , qemu-devel References: <201410222039495960337@sangfor.com> In-Reply-To: <201410222039495960337@sangfor.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: qemu-trivial , Kevin Wolf , Stefan Hajnoczi Subject: Re: [Qemu-trivial] [PATCH v5] snapshot: use local variable to bdrv_pwrite_sync L1 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 13:51:07 -0000 On 2014-10-22 at 14:39, 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 > Reviewed-by: Max Reitz > --- > v4 -> v5: > - delete superfluous check of "l1_size2 != 0" > after qemu_try_blockalign(l1_size2) > > 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..4cf6639 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_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,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); Well, "for (i = 0; i < l1_size; i++) { s->l1_table[i] = be64_to_cpu(l1_table[i]; }" would have saved us this memcpy(). But this function is not critical for performance, so it doesn't really matter (if it was about performance, we could get rid of the first memcpy() as well by the same means). Oh, and by the way: For your future patches, if you create a new version which has non-trivial changes (I normally consider only whitespace and comment changes trivial, and not even all of those), please drop the Reviewed-by. There are exceptions to this where a reviewer explicitly states "If you do $foo, then: Reviewed-by: Foo Bar ", though. Also, please don't include a Reviewed-by if the reviewer did not explicitly state "Reviewed-by: Foo Bar ". As far as I remember, I never explicitly stated "Reviewed-by: Max Reitz " in regards to this series and that was intended. Anyway, thanks for your patch, I applied it to my block branch: https://github.com/XanClic/qemu/commits/block Max > } > } > - 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]:43172) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XgwJj-0008CY-NF for qemu-devel@nongnu.org; Wed, 22 Oct 2014 09:51:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XgwJf-0000I9-Fg for qemu-devel@nongnu.org; Wed, 22 Oct 2014 09:51:11 -0400 Message-ID: <5447B63C.70603@redhat.com> Date: Wed, 22 Oct 2014 15:50:52 +0200 From: Max Reitz MIME-Version: 1.0 References: <201410222039495960337@sangfor.com> In-Reply-To: <201410222039495960337@sangfor.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v5] snapshot: use local variable to bdrv_pwrite_sync L1 table List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Zhang Haoyu , qemu-devel Cc: qemu-trivial , Kevin Wolf , Stefan Hajnoczi On 2014-10-22 at 14:39, 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 > Reviewed-by: Max Reitz > --- > v4 -> v5: > - delete superfluous check of "l1_size2 != 0" > after qemu_try_blockalign(l1_size2) > > 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..4cf6639 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_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,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); Well, "for (i = 0; i < l1_size; i++) { s->l1_table[i] = be64_to_cpu(l1_table[i]; }" would have saved us this memcpy(). But this function is not critical for performance, so it doesn't really matter (if it was about performance, we could get rid of the first memcpy() as well by the same means). Oh, and by the way: For your future patches, if you create a new version which has non-trivial changes (I normally consider only whitespace and comment changes trivial, and not even all of those), please drop the Reviewed-by. There are exceptions to this where a reviewer explicitly states "If you do $foo, then: Reviewed-by: Foo Bar ", though. Also, please don't include a Reviewed-by if the reviewer did not explicitly state "Reviewed-by: Foo Bar ". As far as I remember, I never explicitly stated "Reviewed-by: Max Reitz " in regards to this series and that was intended. Anyway, thanks for your patch, I applied it to my block branch: https://github.com/XanClic/qemu/commits/block Max > } > } > - if (l1_allocated) > - g_free(l1_table); > + g_free(l1_table); > return ret; > } >