[Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_sync L1 table

[Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_sync L1 table

[Zhang Haoyu]

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) {
+        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);
         }
     }
-    if (l1_allocated)
-        g_free(l1_table);
+    g_free(l1_table);
     return ret;
 }
 
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_sync L1 table

[Gonglei]

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?

Best regards,
-Gonglei

> +        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);
>          }
>      }
> -    if (l1_allocated)
> -        g_free(l1_table);
> +    g_free(l1_table);
>      return ret;
>  }
>  

Re: [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_sync L1 table

[Max Reitz]

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...?

Max

> Best regards,
> -Gonglei
>
>> +        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);
>>           }
>>       }
>> -    if (l1_allocated)
>> -        g_free(l1_table);
>> +    g_free(l1_table);
>>       return ret;
>>   }
>>   
>
>

Re: [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_sync L1 table

[Max Reitz]

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. Therefore, just "if (l1_table 
== NULL)" is sufficient.

Max

Re: [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_syncL1 table

[Zhang Haoyu]

>> 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?
>
If the condition of "if (l1_size2 && l1_table == NULL)" is true,
below code will be performed,
s->cache_discards = false;
qcow2_process_discards(bs, ret);
g_free(l1_table);

What's your question?

Thanks,
Zhang Haoyu
>Best regards,
>-Gonglei
>
>> +        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);
>>          }
>>      }
>> -    if (l1_allocated)
>> -        g_free(l1_table);
>> +    g_free(l1_table);
>>      return ret;
>>  }
>>  

Re: [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_sync L1 table

[Gonglei]

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

Re: [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_sync L1 table

[Max Reitz]

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".

> 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.

Max

Re: [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_sync L1 table

[Gonglei]

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

Re: [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_sync L1 table

[Max Reitz]

On 2014-10-22 at 14:30, Gonglei wrote:
> 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?

We can just omit the "l1_size2 && " from the "if (l1_size2 && l1_table 
== NULL)".

Max

Re: [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_sync L1 table

[Gonglei]

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

> On 2014-10-22 at 14:30, Gonglei wrote:
>> 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?
> 
> We can just omit the "l1_size2 && " from the "if (l1_size2 && l1_table 
> == NULL)".
> 

Ack.

Best regards,
-Gonglei