Message-ID: <5447A374.4070309@huawei.com>
Date: Wed, 22 Oct 2014 20:30:44 +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>
	<5447A151.2050000@huawei.com> <5447A1E1.7030102@redhat.com>
In-Reply-To: <5447A1E1.7030102@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:24, Max Reitz wrote:

> On 2014-10-22 at 14:21, Gonglei wrote:
>> 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.
> 
> Okay, so you meant "if l1_size2 == 0 and l1_table != NULL".
> 

Hum, sorry for my typo ;)

>> I don't know whether l1_size2 can be zero or not.
> 
> Probably not, but if it cannot be zero, testing for that case makes even 
> less sense.
> 

So, can we add a check at the begin of this function?

Best regards,
-Gonglei