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; > } >