Re: [Qemu-devel] [Qemu-stable] [PATCH v2] snapshot: use local variable tobdrv_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 <address@hidden>
>> ---
>> 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 | 24 +++++++-----------------
>>   1 file changed, 7 insertions(+), 17 deletions(-)
>>
>> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
>> index 2bcaaf9..29a916a 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 = g_try_malloc0(ROUND_UP(l1_size2, BDRV_SECTOR_SIZE));
>
>I'm sorry, but I just now realized that we don't even need the 0 variant 
>here. We are never accessing any element beyond l1_table[l1_size - 1], 
>and all elements from 0 until l1_size - 1 are overwritten by 
>bdrv_pread() or memcpy(). Therefore we can simply use 
>qemu_try_blockalign(bs->file, l1_size2) here.
>
OK, I will replace g_try_malloc0(ROUND_UP(l1_size2, BDRV_SECTOR_SIZE)) with
qemu_try_blockalign(bs, ROUND_UP(l1_size2, BDRV_SECTOR_SIZE)).

>(and we should be using qemu_{try_,}blockalign() instead of 
>g_{try_,}malloc{0,}() whenever possible for performance reasons)
>
>> +    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,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]);
>> -        }
>
>But I realized something even more important as well: If you are 
>modifying the current L1 table, you need to copy the result back to 
>s->l1_table.
>
My fault.
>Sorry for having missed these in my previous review.
>
>Max
>
>>       }
>> -    if (l1_allocated)
>> -        g_free(l1_table);
>> +    g_free(l1_table);
>>       return ret;
>>   }
>>