Message-ID: <5447A151.2050000@huawei.com>
Date: Wed, 22 Oct 2014 20:21:37 +0800
From: Gonglei <arei.gonglei@huawei.com>
MIME-Version: 1.0
References: <201410221945053926307@sangfor.com> <54479C3E.1070803@huawei.com>
	<54479C8A.9000407@redhat.com> <54479CE5.7040406@redhat.com>
In-Reply-To: <54479CE5.7040406@redhat.com>
Content-Type: text/plain; charset="windows-1252"
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH v4] snapshot: use local variable to
 bdrv_pwrite_sync L1 table
To: Max Reitz <mreitz@redhat.com>
Cc: qemu-trivial <qemu-trivial@nongnu.org>, Kevin Wolf <kwolf@redhat.com>, Zhang Haoyu <zhanghy@sangfor.com>, qemu-devel <qemu-devel@nongnu.org>, Stefan Hajnoczi <stefanha@redhat.com>

On 2014/10/22 20:02, Max Reitz wrote:

> On 2014-10-22 at 14:01, Max Reitz wrote:
>> On 2014-10-22 at 13:59, Gonglei wrote:
>>> On 2014/10/22 19:45, 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 <zhanghy@sangfor.com>
>>>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>>> ---
>>>> 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?
>>
>> Then this condition is met and you return with -ENOMEM...?
> 
> Oh, but I see something different: qemu_try_blockalign() never returns 
> NULL, not even when you request 0 bytes. 


Yes. But if l1_size2 is zero, will waste memory or some other problems.
Please see below:

the original code:
l1_table = g_try_malloc0(align_offset(0, 512));
 -> l1_table = g_try_malloc0(0)
so  l1_table == NULL.

after this patch:
l1_table = qemu_try_blockalign(bs->file, 0)
l1_table will not be NULL.

I don't know whether l1_size2 can be zero or not.

Best regards,
-Gonglei

> Therefore, just "if (l1_table  == NULL)" is sufficient.
> 


> Max