snapshot: use local variable to bdrv_pwrite_sync L1 table

PATCH Superseded Zhang Haoyu 1 participant 2 messages Oct 21, 2014 – Oct 22, 2014 Block layer

Share & tools

Zhang Haoyu

author

2014-10-21 13:44 UTC #0 lore raw

PATCH v3 1/1 snapshot: use local variable to bdrv_pwrite_sync L1 table

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 <[email protected]>

Reviewed-by Max Reitz <[email protected]>

---
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 | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

block/qcow2-refcount.c +9 ~~-16~~

index 2bcaaf9..bceadce 100644
@@ -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) {
+        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,11 @@ 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) {
+            memcpy(s->l1_table, l1_table, l1_size2);
         }
     }
-    if (l1_allocated)
-        g_free(l1_table);
+    g_free(l1_table);
     return ret;
 }

Signature · 1 line

1.7.12.4

Zhang Haoyu

author

2014-10-22 01:40 UTC #1 lore raw

Quoted 69 lines

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 <[email protected]>

Reviewed-by Max Reitz <[email protected]>

---
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

block/qcow2-refcount.c | 25 +++++++++----------------

1 file changed, 9 insertions(+), 16 deletions(-)

block/qcow2-refcount.c +6 ~~-11~~

index 2bcaaf9..bceadce 100644
@@ -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) {
+        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++) {

block/qcow2-refcount.c fail: · line 1054

Quoted 8 lines

block/qcow2-refcount.c +2 -3

@@ -1055,13 +1050,11 @@ 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) {
+            memcpy(s->l1_table, l1_table, l1_size2);

I made a mistake, big-endian l1 table was copied back to s->l1_table.

Quoted 10 lines

block/qcow2-refcount.c +1 -2

        }
    }
-    if (l1_allocated)
-        g_free(l1_table);
+    g_free(l1_table);
    return ret;
 }

Signature · 1 line

1.7.12.4