From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55622) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XgZDL-0002So-Ci for qemu-devel@nongnu.org; Tue, 21 Oct 2014 09:11:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XgZDG-0008Qr-RW for qemu-devel@nongnu.org; Tue, 21 Oct 2014 09:11:03 -0400 Message-ID: <54465B52.90208@redhat.com> Date: Tue, 21 Oct 2014 15:10:42 +0200 From: Max Reitz MIME-Version: 1.0 References: <201410211934299401424@sangfor.com>, <54464B0F.5070500@redhat.com> <201410212108441493881@sangfor.com> <54465B2C.1070603@redhat.com> In-Reply-To: <54465B2C.1070603@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed 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: Zhang Haoyu , qemu-devel Cc: Kevin Wolf , qemu-stable , Stefan Hajnoczi On 2014-10-21 at 15:10, Max Reitz wrote: > On 2014-10-21 at 15:08, 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. >>> >> 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)). > > You can actually omit the ROUND_UP(). There are no accesses beyond > l1_table[l1_size - 1], if I'm not mistaken. Oh, and it should be qemu_try_blockalign(bs->file, l1_size2), not qemu_try_blockalign(bs, l1_size2). > Max > >>> (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; >>>> } >