From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38605) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XgY8E-0002Vr-R8 for qemu-devel@nongnu.org; Tue, 21 Oct 2014 08:01:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XgY8A-00073R-Ai for qemu-devel@nongnu.org; Tue, 21 Oct 2014 08:01:42 -0400 Message-ID: <54464B0F.5070500@redhat.com> Date: Tue, 21 Oct 2014 14:01:19 +0200 From: Max Reitz MIME-Version: 1.0 References: <201410211934299401424@sangfor.com> In-Reply-To: <201410211934299401424@sangfor.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2] 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: Kevin Wolf , qemu-stable , Stefan Hajnoczi On 2014-10-21 at 13:34, 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. (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. Sorry for having missed these in my previous review. Max > } > - if (l1_allocated) > - g_free(l1_table); > + g_free(l1_table); > return ret; > } >