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

[Qemu-devel] [PATCH] 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>
---
 block/qcow2-refcount.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 2bcaaf9..8b318e8 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -881,7 +881,6 @@ 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;
@@ -889,6 +888,11 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
     l2_table = NULL;
     l1_table = NULL;
     l1_size2 = l1_size * sizeof(uint64_t);
+    l1_table = g_try_malloc0(align_offset(l1_size2, 512));
+    if (l1_size2 && l1_table == NULL) {
+        ret = -ENOMEM;
+        goto fail;
+    }
 
     s->cache_discards = true;
 
@@ -896,13 +900,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 +909,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,12 +1051,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]);
-        }
     }
-    if (l1_allocated)
+    if (l1_table)
         g_free(l1_table);
     return ret;
 }
-- 
1.7.12.4

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

[Max Reitz]

On 2014-10-21 at 10:04, 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>
> ---
>   block/qcow2-refcount.c | 22 +++++++---------------
>   1 file changed, 7 insertions(+), 15 deletions(-)
>
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 2bcaaf9..8b318e8 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -881,7 +881,6 @@ 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;
> @@ -889,6 +888,11 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
>       l2_table = NULL;
>       l1_table = NULL;

Please remove this assignment; thanks to this hunk we don't need it anymore.

>       l1_size2 = l1_size * sizeof(uint64_t);
> +    l1_table = g_try_malloc0(align_offset(l1_size2, 512));

I wanted to propose using qemu_try_blockalign(), but since it'd require 
a memset() afterwards, it gets rather ugly.

Could you at least replace 512 by BDRV_SECTOR_SIZE, and maybe even 
align_offset() by ROUND_UP()? We should probably do the latter in all of 
the qcow2 code, though, I think it's just there because it has been 
around since before there was a ROUND_UP()...

> +    if (l1_size2 && l1_table == NULL) {
> +        ret = -ENOMEM;
> +        goto fail;
> +    }
>   
>       s->cache_discards = true;
>   
> @@ -896,13 +900,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 +909,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,12 +1051,8 @@ fail:

I don't think it will change a lot, but could you wrap the 
"s->cache_discards = false; qcow2_process_discards(bs, ret);" in an "if 
(s->cache_discards)"? You have introduced a case where s->cache_discards 
was still false, so we don't need to call qcow2_process_discards() then 
(which will hopefully return immediately, but well...).

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

Just drop the condition. g_free(l1_table); is enough.

>       return ret;
>   }

The change itself is good, it just needs some polishing.

Max

Re: [Qemu-devel] [PATCH] 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>
>> ---
>>   block/qcow2-refcount.c | 22 +++++++---------------
>>   1 file changed, 7 insertions(+), 15 deletions(-)
>>
>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>> index 2bcaaf9..8b318e8 100644
>> --- a/block/qcow2-refcount.c
>> +++ b/block/qcow2-refcount.c
>> @@ -881,7 +881,6 @@ 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;
>> @@ -889,6 +888,11 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
>>       l2_table = NULL;
>>       l1_table = NULL;
>
>Please remove this assignment; thanks to this hunk we don't need it anymore.
OK.
>
>>       l1_size2 = l1_size * sizeof(uint64_t);
>> +    l1_table = g_try_malloc0(align_offset(l1_size2, 512));
>
>I wanted to propose using qemu_try_blockalign(), but since it'd require 
>a memset() afterwards, it gets rather ugly.
>
>Could you at least replace 512 by BDRV_SECTOR_SIZE, and maybe even 
>align_offset() by ROUND_UP()? We should probably do the latter in all of 
>the qcow2 code, though, I think it's just there because it has been 
>around since before there was a ROUND_UP()...
>
Good, I will replace 512 with BDRV_SECTOR_SIZE, and replace align_offset with ROUND_UP.
>> +    if (l1_size2 && l1_table == NULL) {
>> +        ret = -ENOMEM;
>> +        goto fail;
>> +    }
>>   
>>       s->cache_discards = true;
>>   
>> @@ -896,13 +900,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 +909,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,12 +1051,8 @@ fail:
>
>I don't think it will change a lot, but could you wrap the 
>"s->cache_discards = false; qcow2_process_discards(bs, ret);" in an "if 
>(s->cache_discards)"? You have introduced a case where s->cache_discards 
>was still false, so we don't need to call qcow2_process_discards() then 
>(which will hopefully return immediately, but well...).
s->cache_discards's initial value is true in qcow2_update_snapshot_refcount(),
where s->cache_discards is set to false?
Or you means s->cache_discards should be set to false
after g_try_malloc0(align_offset(l1_size2, 512)) failed.

>
>>           }
>>   
>>           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 (l1_allocated)
>> +    if (l1_table)
>>           g_free(l1_table);
>
>Just drop the condition. g_free(l1_table); is enough.
>
OK.
>>       return ret;
>>   }
>
>The change itself is good, it just needs some polishing.
>
>Max

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

[Max Reitz]

On 2014-10-21 at 12:49, 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>
>>> ---
>>>    block/qcow2-refcount.c | 22 +++++++---------------
>>>    1 file changed, 7 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>>> index 2bcaaf9..8b318e8 100644
>>> --- a/block/qcow2-refcount.c
>>> +++ b/block/qcow2-refcount.c
>>> @@ -881,7 +881,6 @@ 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;
>>> @@ -889,6 +888,11 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
>>>        l2_table = NULL;
>>>        l1_table = NULL;
>> Please remove this assignment; thanks to this hunk we don't need it anymore.
> OK.
>>>        l1_size2 = l1_size * sizeof(uint64_t);
>>> +    l1_table = g_try_malloc0(align_offset(l1_size2, 512));
>> I wanted to propose using qemu_try_blockalign(), but since it'd require
>> a memset() afterwards, it gets rather ugly.
>>
>> Could you at least replace 512 by BDRV_SECTOR_SIZE, and maybe even
>> align_offset() by ROUND_UP()? We should probably do the latter in all of
>> the qcow2 code, though, I think it's just there because it has been
>> around since before there was a ROUND_UP()...
>>
> Good, I will replace 512 with BDRV_SECTOR_SIZE, and replace align_offset with ROUND_UP.
>>> +    if (l1_size2 && l1_table == NULL) {
>>> +        ret = -ENOMEM;
>>> +        goto fail;
>>> +    }
>>>    
>>>        s->cache_discards = true;
>>>    
>>> @@ -896,13 +900,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 +909,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,12 +1051,8 @@ fail:
>> I don't think it will change a lot, but could you wrap the
>> "s->cache_discards = false; qcow2_process_discards(bs, ret);" in an "if
>> (s->cache_discards)"? You have introduced a case where s->cache_discards
>> was still false, so we don't need to call qcow2_process_discards() then
>> (which will hopefully return immediately, but well...).
> s->cache_discards's initial value is true in qcow2_update_snapshot_refcount(),
> where s->cache_discards is set to false?

It is? I only see it set to true after the "if (l1_size2 && l1_table == 
NULL)" conditional block.

Well, okay, I don't know about the callers of qcow2_process_discards(), 
so they may have set s->cache_discards to true and then expect this 
function to always call qcow2_process_discards() and set 
s->cache_discards to false. Okay, then let's just keep it as it is.

Max

> Or you means s->cache_discards should be set to false
> after g_try_malloc0(align_offset(l1_size2, 512)) failed.
>
>>>            }
>>>    
>>>            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 (l1_allocated)
>>> +    if (l1_table)
>>>            g_free(l1_table);
>> Just drop the condition. g_free(l1_table); is enough.
>>
> OK.
>>>        return ret;
>>>    }
>> The change itself is good, it just needs some polishing.
>>
>> Max

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 947 bytes --]

On 10/21/2014 03:24 AM, Max Reitz wrote:
> On 2014-10-21 at 10:04, 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>
>> ---
>>   block/qcow2-refcount.c | 22 +++++++---------------
>>   1 file changed, 7 insertions(+), 15 deletions(-)
>>

I know we're up to v5 and that Max already took it into his branch, but...


>>       l1_size2 = l1_size * sizeof(uint64_t);
>> +    l1_table = g_try_malloc0(align_offset(l1_size2, 512));
> 
> I wanted to propose using qemu_try_blockalign(), but since it'd require
> a memset() afterwards, it gets rather ugly.

Not after this recent patch:

https://lists.gnu.org/archive/html/qemu-devel/2014-10/msg02499.html

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

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

[Gonglei]

On 2014/10/23 6:18, Eric Blake wrote:

> On 10/21/2014 03:24 AM, Max Reitz wrote:
>> On 2014-10-21 at 10:04, 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>
>>> ---
>>>   block/qcow2-refcount.c | 22 +++++++---------------
>>>   1 file changed, 7 insertions(+), 15 deletions(-)
>>>
> 
> I know we're up to v5 and that Max already took it into his branch, but...
> 
> 
>>>       l1_size2 = l1_size * sizeof(uint64_t);
>>> +    l1_table = g_try_malloc0(align_offset(l1_size2, 512));
>>
>> I wanted to propose using qemu_try_blockalign(), but since it'd require
>> a memset() afterwards, it gets rather ugly.
> 
> Not after this recent patch:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2014-10/msg02499.html
> 

Good catch :)

Best regards,
-Gonglei

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

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 1094 bytes --]

Am 23.10.2014 um 00:18 hat Eric Blake geschrieben:
> On 10/21/2014 03:24 AM, Max Reitz wrote:
> > On 2014-10-21 at 10:04, 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>
> >> ---
> >>   block/qcow2-refcount.c | 22 +++++++---------------
> >>   1 file changed, 7 insertions(+), 15 deletions(-)
> >>
> 
> I know we're up to v5 and that Max already took it into his branch, but...
> 
> 
> >>       l1_size2 = l1_size * sizeof(uint64_t);
> >> +    l1_table = g_try_malloc0(align_offset(l1_size2, 512));
> > 
> > I wanted to propose using qemu_try_blockalign(), but since it'd require
> > a memset() afterwards, it gets rather ugly.
> 
> Not after this recent patch:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2014-10/msg02499.html

We can switch to qemu_try_blockalign0() in a follow-up patch.

But this is far from being a fast path, so there is very little to gain
anyway.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

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

[Max Reitz]

On 2014-10-23 at 09:03, Kevin Wolf wrote:
> Am 23.10.2014 um 00:18 hat Eric Blake geschrieben:
>> On 10/21/2014 03:24 AM, Max Reitz wrote:
>>> On 2014-10-21 at 10:04, 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>
>>>> ---
>>>>    block/qcow2-refcount.c | 22 +++++++---------------
>>>>    1 file changed, 7 insertions(+), 15 deletions(-)
>>>>
>> I know we're up to v5 and that Max already took it into his branch, but...
>>
>>
>>>>        l1_size2 = l1_size * sizeof(uint64_t);
>>>> +    l1_table = g_try_malloc0(align_offset(l1_size2, 512));
>>> I wanted to propose using qemu_try_blockalign(), but since it'd require
>>> a memset() afterwards, it gets rather ugly.
>> Not after this recent patch:
>>
>> https://lists.gnu.org/archive/html/qemu-devel/2014-10/msg02499.html
> We can switch to qemu_try_blockalign0() in a follow-up patch.
>
> But this is far from being a fast path, so there is very little to gain
> anyway.

We don't need the 0 variant anyway. In v5, the one I applied, it's just 
qemu_try_blockalign(). The size does not have to be aligned to 
BDRV_SECTOR_SIZE, since any access index is below l1_size and only 
bdrv_pread() and bdrv_pwrite() are used, so we can first omit the 
align_offset(). Second, all the rest (0 .. l1_size - 1) is overwritten 
either by memcpy() or bdrv_pread(). Therefore, no 0-ing required and 
qemu_try_blockalign() was fine.

There are things we can do in follow-up patches to optimize v5 (getting 
read of the memcpy()s), but as Kevin said (and myself before that in 
reply to v5), this function is not performance-critical, so there's no 
real point in doing so (other than "Clearly unoptimized code makes my 
fingers twitch").

Max