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