[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-trivial] [Qemu-devel] [PATCH] snapshot: use local variable to
From: |
Max Reitz |
Subject: |
Re: [Qemu-trivial] [Qemu-devel] [PATCH] snapshot: use local variable to bdrv_pwrite_sync L1 table |
Date: |
Tue, 21 Oct 2014 11:24:46 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 |
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 <address@hidden>
---
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