[20/22] qcow2-dirty-bitmap: refcounts
diff mbox

Message ID 1475232808-4852-21-git-send-email-vsementsov@virtuozzo.com
State New
Headers show

Commit Message

Vladimir Sementsov-Ogievskiy Sept. 30, 2016, 10:53 a.m. UTC
Calculate refcounts for dirty bitmaps.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2-bitmap.c   | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2-refcount.c | 14 +++++++-----
 block/qcow2.h          |  5 +++++
 3 files changed, 74 insertions(+), 5 deletions(-)

Comments

Max Reitz Oct. 10, 2016, 5:59 p.m. UTC | #1
On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
> Calculate refcounts for dirty bitmaps.

The commit message should mention that this is for qcow2's qemu-img
check implementation.

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/qcow2-bitmap.c   | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  block/qcow2-refcount.c | 14 +++++++-----
>  block/qcow2.h          |  5 +++++
>  3 files changed, 74 insertions(+), 5 deletions(-)
> 
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 76f7e2b..6d9394a 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -901,3 +901,63 @@ out:
>      g_free(new_dir);
>      g_free(dir);
>  }
> +
> +int qcow2_dirty_bitmaps_refcounts(BlockDriverState *bs,
> +                                  BdrvCheckResult *res,
> +                                  void **refcount_table,
> +                                  int64_t *refcount_table_size)

I'd rename this function to make clear that this is for checking the
refcounts, e.g. to "qcow2_check_dirty_bitmaps_refcounts" or
"qcow2_count_dirty_bitmaps_refcounts" or just
"qcow2_check_dirty_bitmaps". Probably the last one is the best because
this function should ideally perform a full check of the dirty bitmaps
extension.

> +{
> +    BDRVQcow2State *s = bs->opaque;
> +    uint8_t *dir;
> +    Qcow2BitmapDirEntry *e;
> +    Error *local_err = NULL;
> +
> +    int i;
> +    int ret = inc_refcounts(bs, res, refcount_table, refcount_table_size,
> +                            s->bitmap_directory_offset,
> +                            s->bitmap_directory_size);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    dir = directory_read(bs, s->bitmap_directory_offset,
> +                         s->bitmap_directory_size, &local_err);
> +    if (dir == NULL) {
> +        error_report_err(local_err);

I think you should increment res->corruptions here.

> +        return -EINVAL;
> +    }
> +
> +    for_each_bitmap_dir_entry(e, dir, s->bitmap_directory_size) {
> +        uint64_t *bitmap_table = NULL;

I think you should call check_constraints() here.

> +
> +        ret = inc_refcounts(bs, res, refcount_table, refcount_table_size,
> +                            e->bitmap_table_offset,
> +                            e->bitmap_table_size);

Probably rather e->bitmap_table_size * sizeof(uint64_t).

> +        if (ret < 0) {
> +            return ret;
> +        }
> +
> +        ret = bitmap_table_load(bs, e, &bitmap_table);
> +        if (ret < 0) {

Again, it would make sense to increment res->corruptions here.

> +            return ret;
> +        }
> +
> +        for (i = 0; i < e->bitmap_table_size; ++i) {
> +            uint64_t off = be64_to_cpu(bitmap_table[i]);
> +            if (off <= 1) {
> +                continue;
> +            }

As I said in some other patch, I'd write this condition differently
(with an offset mask, etc.).

Also, you should check that the offset is aligned to a cluster boundary
and that no unknown flags are set.

> +
> +            ret = inc_refcounts(bs, res, refcount_table, refcount_table_size,
> +                                off, s->cluster_size);
> +            if (ret < 0) {
> +                g_free(bitmap_table);
> +                return ret;
> +            }
> +        }
> +
> +        g_free(bitmap_table);
> +    }
> +
> +    return 0;

dir is leaked here.

> +}
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index cbfb3fe..05bcc57 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -1309,11 +1309,9 @@ static int realloc_refcount_array(BDRVQcow2State *s, void **array,
>   *
>   * Modifies the number of errors in res.
>   */
> -static int inc_refcounts(BlockDriverState *bs,
> -                         BdrvCheckResult *res,
> -                         void **refcount_table,
> -                         int64_t *refcount_table_size,
> -                         int64_t offset, int64_t size)
> +int inc_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
> +                  void **refcount_table, int64_t *refcount_table_size,
> +                  int64_t offset, int64_t size)

First, if you make this function public, you have to give it a qcow2_
prefix.

Second, if this function is public, it should have a name that makes
sense. inc_refcounts() sounds as if it's the same as update_refcount()
with an addend of 1. I'd rename it qcow2_inc_refcounts_imrt(), because
that's probably the shortest name I can come up with (and
qcow2-refcount.c explains what an IMRT is in some comment).

>  {
>      BDRVQcow2State *s = bs->opaque;
>      uint64_t start, last, cluster_offset, k, refcount;
> @@ -1843,6 +1841,12 @@ static int calculate_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>          return ret;
>      }
>  
> +    /* dirty bitmaps */
> +    ret = qcow2_dirty_bitmaps_refcounts(bs, res, refcount_table, nb_clusters);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
>      return check_refblocks(bs, res, fix, rebuild, refcount_table, nb_clusters);
>  }
>  
> diff --git a/block/qcow2.h b/block/qcow2.h
> index a5e7592..0a050ea 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -563,6 +563,9 @@ int qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t offset,
>                                   int64_t size);
>  int qcow2_pre_write_overlap_check(BlockDriverState *bs, int ign, int64_t offset,
>                                    int64_t size);
> +int inc_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
> +                  void **refcount_table, int64_t *refcount_table_size,
> +                  int64_t offset, int64_t size);
>  
>  int qcow2_change_refcount_order(BlockDriverState *bs, int refcount_order,
>                                  BlockDriverAmendStatusCB *status_cb,
> @@ -616,6 +619,8 @@ int qcow2_read_snapshots(BlockDriverState *bs);
>  int qcow2_read_bitmaps(BlockDriverState *bs, Error **errp);
>  
>  int qcow2_delete_bitmaps(BlockDriverState *bs);
> +int qcow2_dirty_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
> +    void **refcount_table, int64_t *refcount_table_size);

Normally we try to align parameters along the opening parenthesis as
long as there is enough space, and there is enough space here.

Max

>  
>  /* qcow2-cache.c functions */
>  Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables);
>

Patch
diff mbox

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 76f7e2b..6d9394a 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -901,3 +901,63 @@  out:
     g_free(new_dir);
     g_free(dir);
 }
+
+int qcow2_dirty_bitmaps_refcounts(BlockDriverState *bs,
+                                  BdrvCheckResult *res,
+                                  void **refcount_table,
+                                  int64_t *refcount_table_size)
+{
+    BDRVQcow2State *s = bs->opaque;
+    uint8_t *dir;
+    Qcow2BitmapDirEntry *e;
+    Error *local_err = NULL;
+
+    int i;
+    int ret = inc_refcounts(bs, res, refcount_table, refcount_table_size,
+                            s->bitmap_directory_offset,
+                            s->bitmap_directory_size);
+    if (ret < 0) {
+        return ret;
+    }
+
+    dir = directory_read(bs, s->bitmap_directory_offset,
+                         s->bitmap_directory_size, &local_err);
+    if (dir == NULL) {
+        error_report_err(local_err);
+        return -EINVAL;
+    }
+
+    for_each_bitmap_dir_entry(e, dir, s->bitmap_directory_size) {
+        uint64_t *bitmap_table = NULL;
+
+        ret = inc_refcounts(bs, res, refcount_table, refcount_table_size,
+                            e->bitmap_table_offset,
+                            e->bitmap_table_size);
+        if (ret < 0) {
+            return ret;
+        }
+
+        ret = bitmap_table_load(bs, e, &bitmap_table);
+        if (ret < 0) {
+            return ret;
+        }
+
+        for (i = 0; i < e->bitmap_table_size; ++i) {
+            uint64_t off = be64_to_cpu(bitmap_table[i]);
+            if (off <= 1) {
+                continue;
+            }
+
+            ret = inc_refcounts(bs, res, refcount_table, refcount_table_size,
+                                off, s->cluster_size);
+            if (ret < 0) {
+                g_free(bitmap_table);
+                return ret;
+            }
+        }
+
+        g_free(bitmap_table);
+    }
+
+    return 0;
+}
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index cbfb3fe..05bcc57 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1309,11 +1309,9 @@  static int realloc_refcount_array(BDRVQcow2State *s, void **array,
  *
  * Modifies the number of errors in res.
  */
-static int inc_refcounts(BlockDriverState *bs,
-                         BdrvCheckResult *res,
-                         void **refcount_table,
-                         int64_t *refcount_table_size,
-                         int64_t offset, int64_t size)
+int inc_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
+                  void **refcount_table, int64_t *refcount_table_size,
+                  int64_t offset, int64_t size)
 {
     BDRVQcow2State *s = bs->opaque;
     uint64_t start, last, cluster_offset, k, refcount;
@@ -1843,6 +1841,12 @@  static int calculate_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
         return ret;
     }
 
+    /* dirty bitmaps */
+    ret = qcow2_dirty_bitmaps_refcounts(bs, res, refcount_table, nb_clusters);
+    if (ret < 0) {
+        return ret;
+    }
+
     return check_refblocks(bs, res, fix, rebuild, refcount_table, nb_clusters);
 }
 
diff --git a/block/qcow2.h b/block/qcow2.h
index a5e7592..0a050ea 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -563,6 +563,9 @@  int qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t offset,
                                  int64_t size);
 int qcow2_pre_write_overlap_check(BlockDriverState *bs, int ign, int64_t offset,
                                   int64_t size);
+int inc_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
+                  void **refcount_table, int64_t *refcount_table_size,
+                  int64_t offset, int64_t size);
 
 int qcow2_change_refcount_order(BlockDriverState *bs, int refcount_order,
                                 BlockDriverAmendStatusCB *status_cb,
@@ -616,6 +619,8 @@  int qcow2_read_snapshots(BlockDriverState *bs);
 int qcow2_read_bitmaps(BlockDriverState *bs, Error **errp);
 
 int qcow2_delete_bitmaps(BlockDriverState *bs);
+int qcow2_dirty_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
+    void **refcount_table, int64_t *refcount_table_size);
 
 /* qcow2-cache.c functions */
 Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables);