[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_
From: |
Zhang Haoyu |
Subject: |
Re: [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_syncL1 table |
Date: |
Wed, 22 Oct 2014 20:07:53 +0800 |
>> 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>
>> Reviewed-by: Max Reitz <address@hidden>
>> ---
>> 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;
>> }
>>
- [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_sync L1 table, Zhang Haoyu, 2014/10/22
- Re: [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_sync L1 table, Gonglei, 2014/10/22
- Re: [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_sync L1 table, Max Reitz, 2014/10/22
- Re: [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_sync L1 table, Max Reitz, 2014/10/22
- Re: [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_sync L1 table, Gonglei, 2014/10/22
- Re: [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_sync L1 table, Max Reitz, 2014/10/22
- Re: [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_sync L1 table, Gonglei, 2014/10/22
- Re: [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_sync L1 table, Max Reitz, 2014/10/22
- Re: [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_sync L1 table, Gonglei, 2014/10/22
Re: [Qemu-devel] [PATCH v4] snapshot: use local variable to bdrv_pwrite_syncL1 table,
Zhang Haoyu <=