diff mbox

[v4,06/26] qcow2: Use 64 bits for refcount values

Message ID 1417613866-25890-7-git-send-email-mreitz@redhat.com
State New
Headers show

Commit Message

Max Reitz Dec. 3, 2014, 1:37 p.m. UTC 
Refcounts may have a width of up to 64 bits, so qemu should use the same
width to represent refcount values internally.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-cluster.c  |  2 +-
 block/qcow2-refcount.c | 42 ++++++++++++++++++++----------------------
 block/qcow2.h          |  4 ++--
 3 files changed, 23 insertions(+), 25 deletions(-)

Comments

Eric Blake Dec. 3, 2014, 4:11 p.m. UTC | #1 
On 12/03/2014 06:37 AM, Max Reitz wrote:
> Refcounts may have a width of up to 64 bits, so qemu should use the same
> width to represent refcount values internally.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2-cluster.c  |  2 +-
>  block/qcow2-refcount.c | 42 ++++++++++++++++++++----------------------
>  block/qcow2.h          |  4 ++--
>  3 files changed, 23 insertions(+), 25 deletions(-)
> 

> @@ -1698,7 +1696,7 @@ static void compare_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>                                        refcount2 - refcount1,
>                                        refcount1 > refcount2,
>                                        QCOW2_DISCARD_ALWAYS);
> -                if (ret >= 0) {
> +                if (ret == 0) {
>                      (*num_fixed)++;
>                      continue;
>                  }

This hunk looks a bit misplaced (it is unrelated to a sizing change).
Patch 5 altered update_refcount, but does not document its return value.
 But a careful step through update_refcount() shows that all error paths
return negative, and the only places where we do things like 'ret =
alloc_refcount_block(...)" are later followed by an unconditional 'ret =
0' on success paths (so I didn't check whether alloc_refcount_block
returns positive values, but didn't need to).  If you have to respin, it
might be better to add documentation of the return value and update
callers to assume a non-positive return as a separate patch, rather than
sliding it in with this one.  But as I don't see any code faults, and am
not sure it is worth a respin just for this issue,

Reviewed-by: Eric Blake <eblake@redhat.com>
Max Reitz Dec. 3, 2014, 4:18 p.m. UTC | #2 
On 2014-12-03 at 17:11, Eric Blake wrote:
> On 12/03/2014 06:37 AM, Max Reitz wrote:
>> Refcounts may have a width of up to 64 bits, so qemu should use the same
>> width to represent refcount values internally.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block/qcow2-cluster.c  |  2 +-
>>   block/qcow2-refcount.c | 42 ++++++++++++++++++++----------------------
>>   block/qcow2.h          |  4 ++--
>>   3 files changed, 23 insertions(+), 25 deletions(-)
>>
>> @@ -1698,7 +1696,7 @@ static void compare_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>>                                         refcount2 - refcount1,
>>                                         refcount1 > refcount2,
>>                                         QCOW2_DISCARD_ALWAYS);
>> -                if (ret >= 0) {
>> +                if (ret == 0) {
>>                       (*num_fixed)++;
>>                       continue;
>>                   }
> This hunk looks a bit misplaced (it is unrelated to a sizing change).

Ah, right, I think it should have been in patch 3. I even looked through 
the patches before sending them, but somehow I missed this...

> Patch 5 altered update_refcount, but does not document its return value.
>   But a careful step through update_refcount() shows that all error paths
> return negative, and the only places where we do things like 'ret =
> alloc_refcount_block(...)" are later followed by an unconditional 'ret =
> 0' on success paths (so I didn't check whether alloc_refcount_block
> returns positive values, but didn't need to).  If you have to respin, it
> might be better to add documentation of the return value and update
> callers to assume a non-positive return as a separate patch, rather than
> sliding it in with this one.  But as I don't see any code faults, and am
> not sure it is worth a respin just for this issue,

Hm, I don't think this is something that should be fixed up by the 
applying maintainer. I will respin, even if it's just for this, but I'll 
wait until this series has been sufficiently looked at.

> Reviewed-by: Eric Blake <eblake@redhat.com>

Once again, thank you!

Max
Stefan Hajnoczi Dec. 11, 2014, 11:04 a.m. UTC | #3 
On Wed, Dec 03, 2014 at 02:37:26PM +0100, Max Reitz wrote:
> Refcounts may have a width of up to 64 bits, so qemu should use the same
> width to represent refcount values internally.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2-cluster.c  |  2 +-
>  block/qcow2-refcount.c | 42 ++++++++++++++++++++----------------------
>  block/qcow2.h          |  4 ++--
>  3 files changed, 23 insertions(+), 25 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
diff mbox

Patch

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index da37535..5a678f3 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1640,7 +1640,7 @@  static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
     for (i = 0; i < l1_size; i++) {
         uint64_t l2_offset = l1_table[i] & L1E_OFFSET_MASK;
         bool l2_dirty = false;
-        uint16_t l2_refcount;
+        uint64_t l2_refcount;
 
         if (!l2_offset) {
             /* unallocated */
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index b3aed9c..095ff9b 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -29,7 +29,7 @@ 
 
 static int64_t alloc_clusters_noref(BlockDriverState *bs, uint64_t size);
 static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs,
-                            int64_t offset, int64_t length, uint16_t addend,
+                            int64_t offset, int64_t length, uint64_t addend,
                             bool decrease, enum qcow2_discard_type type);
 
 
@@ -91,7 +91,7 @@  static int load_refcount_block(BlockDriverState *bs,
  * *refcount. Returns 0 on success and -errno on failure.
  */
 int qcow2_get_refcount(BlockDriverState *bs, int64_t cluster_index,
-                       uint16_t *refcount)
+                       uint64_t *refcount)
 {
     BDRVQcowState *s = bs->opaque;
     uint64_t refcount_table_index, block_index;
@@ -537,7 +537,7 @@  found:
 static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs,
                                                    int64_t offset,
                                                    int64_t length,
-                                                   uint16_t addend,
+                                                   uint64_t addend,
                                                    bool decrease,
                                                    enum qcow2_discard_type type)
 {
@@ -549,8 +549,8 @@  static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs,
 
 #ifdef DEBUG_ALLOC2
     fprintf(stderr, "update_refcount: offset=%" PRId64 " size=%" PRId64
-            " addend=%" PRId16 " decrease=%d\n", offset, length,
-            (int16_t)addend, decrease);
+            " addend=%" PRId64 " decrease=%d\n", offset, length,
+            (int64_t)addend, decrease);
 #endif
     if (length < 0) {
         return -EINVAL;
@@ -569,7 +569,7 @@  static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs,
         cluster_offset += s->cluster_size)
     {
         int block_index;
-        uint16_t refcount;
+        uint64_t refcount;
         int64_t cluster_index = cluster_offset >> s->cluster_bits;
         int64_t table_index = cluster_index >> s->refcount_block_bits;
 
@@ -596,9 +596,9 @@  static int QEMU_WARN_UNUSED_RESULT update_refcount(BlockDriverState *bs,
         block_index = cluster_index & (s->refcount_block_size - 1);
 
         refcount = be16_to_cpu(refcount_block[block_index]);
-        if ((uint16_t)(refcount + addend) > s->refcount_max ||
-            (!decrease && (uint16_t)(refcount + addend) < refcount) ||
-            ( decrease && (uint16_t)(refcount + addend) > refcount))
+        if (refcount + addend > s->refcount_max ||
+            (!decrease && refcount + addend < refcount) ||
+            ( decrease && refcount + addend > refcount))
         {
             ret = -EINVAL;
             goto fail;
@@ -656,7 +656,7 @@  fail:
  */
 int qcow2_update_cluster_refcount(BlockDriverState *bs,
                                   int64_t cluster_index,
-                                  uint16_t addend, bool decrease,
+                                  uint64_t addend, bool decrease,
                                   enum qcow2_discard_type type)
 {
     BDRVQcowState *s = bs->opaque;
@@ -682,8 +682,7 @@  int qcow2_update_cluster_refcount(BlockDriverState *bs,
 static int64_t alloc_clusters_noref(BlockDriverState *bs, uint64_t size)
 {
     BDRVQcowState *s = bs->opaque;
-    uint64_t i, nb_clusters;
-    uint16_t refcount;
+    uint64_t i, nb_clusters, refcount;
     int ret;
 
     nb_clusters = size_to_clusters(s, size);
@@ -741,9 +740,8 @@  int qcow2_alloc_clusters_at(BlockDriverState *bs, uint64_t offset,
     int nb_clusters)
 {
     BDRVQcowState *s = bs->opaque;
-    uint64_t cluster_index;
+    uint64_t cluster_index, refcount;
     uint64_t i;
-    uint16_t refcount;
     int ret;
 
     assert(nb_clusters >= 0);
@@ -897,11 +895,10 @@  int qcow2_update_snapshot_refcount(BlockDriverState *bs,
     int64_t l1_table_offset, int l1_size, int addend)
 {
     BDRVQcowState *s = bs->opaque;
-    uint64_t *l1_table, *l2_table, l2_offset, offset, l1_size2;
+    uint64_t *l1_table, *l2_table, l2_offset, offset, l1_size2, refcount;
     bool l1_allocated = false;
     int64_t old_offset, old_l2_offset;
     int i, j, l1_modified = 0, nb_csectors;
-    uint16_t refcount;
     int ret;
 
     l2_table = NULL;
@@ -1368,7 +1365,7 @@  static int check_oflag_copied(BlockDriverState *bs, BdrvCheckResult *res,
     BDRVQcowState *s = bs->opaque;
     uint64_t *l2_table = qemu_blockalign(bs, s->cluster_size);
     int ret;
-    uint16_t refcount;
+    uint64_t refcount;
     int i, j;
 
     for (i = 0; i < s->l1_size; i++) {
@@ -1388,7 +1385,7 @@  static int check_oflag_copied(BlockDriverState *bs, BdrvCheckResult *res,
         }
         if ((refcount == 1) != ((l1_entry & QCOW_OFLAG_COPIED) != 0)) {
             fprintf(stderr, "%s OFLAG_COPIED L2 cluster: l1_index=%d "
-                    "l1_entry=%" PRIx64 " refcount=%d\n",
+                    "l1_entry=%" PRIx64 " refcount=%" PRIu64 "\n",
                     fix & BDRV_FIX_ERRORS ? "Repairing" :
                                             "ERROR",
                     i, l1_entry, refcount);
@@ -1432,7 +1429,7 @@  static int check_oflag_copied(BlockDriverState *bs, BdrvCheckResult *res,
                 }
                 if ((refcount == 1) != ((l2_entry & QCOW_OFLAG_COPIED) != 0)) {
                     fprintf(stderr, "%s OFLAG_COPIED data cluster: "
-                            "l2_entry=%" PRIx64 " refcount=%d\n",
+                            "l2_entry=%" PRIx64 " refcount=%" PRIu64 "\n",
                             fix & BDRV_FIX_ERRORS ? "Repairing" :
                                                     "ERROR",
                             l2_entry, refcount);
@@ -1658,7 +1655,7 @@  static void compare_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
 {
     BDRVQcowState *s = bs->opaque;
     int64_t i;
-    uint16_t refcount1, refcount2;
+    uint64_t refcount1, refcount2;
     int ret;
 
     for (i = 0, *highest_cluster = 0; i < nb_clusters; i++) {
@@ -1687,7 +1684,8 @@  static void compare_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
                 num_fixed = &res->corruptions_fixed;
             }
 
-            fprintf(stderr, "%s cluster %" PRId64 " refcount=%d reference=%d\n",
+            fprintf(stderr, "%s cluster %" PRId64 " refcount=%" PRIu64
+                    " reference=%" PRIu64 "\n",
                    num_fixed != NULL     ? "Repairing" :
                    refcount1 < refcount2 ? "ERROR" :
                                            "Leaked",
@@ -1698,7 +1696,7 @@  static void compare_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
                                       refcount2 - refcount1,
                                       refcount1 > refcount2,
                                       QCOW2_DISCARD_ALWAYS);
-                if (ret >= 0) {
+                if (ret == 0) {
                     (*num_fixed)++;
                     continue;
                 }
diff --git a/block/qcow2.h b/block/qcow2.h
index 0240ee8..adf515c 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -490,10 +490,10 @@  int qcow2_refcount_init(BlockDriverState *bs);
 void qcow2_refcount_close(BlockDriverState *bs);
 
 int qcow2_get_refcount(BlockDriverState *bs, int64_t cluster_index,
-                       uint16_t *refcount);
+                       uint64_t *refcount);
 
 int qcow2_update_cluster_refcount(BlockDriverState *bs, int64_t cluster_index,
-                                  uint16_t addend, bool decrease,
+                                  uint64_t addend, bool decrease,
                                   enum qcow2_discard_type type);
 
 int64_t qcow2_alloc_clusters(BlockDriverState *bs, uint64_t size);