[Qemu-devel] [PATCH v4 0/7] block/qcow2: Improve zero cluster expansion

[Qemu-devel] [PATCH v4 0/7] block/qcow2: Improve zero cluster expansion

[Max Reitz]

The main purpose of this series is to add a progress report to
qemu-img amend. This is achieved by adding a callback function to
bdrv_amend_options() - the reasons for this choice are explained in
patch 1.

While adapting qcow2's expand_zero_clusters_in_l1() accordingly, I
noticed a way to simplify it and get rid of the rather ugly bitmap used
there (patch 6).


Changes from v3 (v4 is a rebase of v3 on current master)
- Patch 2: There is BlockBackend now (only contextual)
- Patch 4: An instance of g_try_malloc0() instead of g_malloc0() (only
  contextual)
- Patch 5: Changes in the qcow2 check functions due to my "qcow2: Fix
  image repairing" series (only contextual)
- Patch 6: Originally, contained a hunk which removed a g_malloc0() and
  the buffer size calculation before it; now, it's a g_try_malloc0() and
  therefore the hunk has to remove the error handling as well. This is a
  functional change, but I decided to keep the R-bs anyway because it is
  really straightforward.

Since with the exception of patch 6 all of these were contextual changes
(and the change to patch 6 was really straightforward), I kept all the
Reviewed-bys and this series is therefore still fully reviewed.


git-backport-diff against v3:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/7:[----] [--] 'block: Add status callback to bdrv_amend_options()'
002/7:[----] [-C] 'qemu-img: Add progress output for amend'
003/7:[----] [--] 'qemu-img: Fix insignificant memleak'
004/7:[----] [-C] 'block/qcow2: Implement status CB for amend'
005/7:[----] [-C] 'block/qcow2: Make get_refcount() global'
006/7:[0006] [FC] 'block/qcow2: Simplify shared L2 handling in amend'
007/7:[----] [--] 'iotests: Expand test 061'


Max Reitz (7):
  block: Add status callback to bdrv_amend_options()
  qemu-img: Add progress output for amend
  qemu-img: Fix insignificant memleak
  block/qcow2: Implement status CB for amend
  block/qcow2: Make get_refcount() global
  block/qcow2: Simplify shared L2 handling in amend
  iotests: Expand test 061

 block.c                    |   5 +-
 block/qcow2-cluster.c      | 117 +++++++++++++++++++++------------------------
 block/qcow2-refcount.c     |  26 +++++-----
 block/qcow2.c              |  10 ++--
 block/qcow2.h              |   5 +-
 include/block/block.h      |   8 +++-
 include/block/block_int.h  |   3 +-
 qemu-img-cmds.hx           |   4 +-
 qemu-img.c                 |  29 +++++++++--
 qemu-img.texi              |   2 +-
 tests/qemu-iotests/061     |  25 ++++++++++
 tests/qemu-iotests/061.out |  30 ++++++++++++
 tests/qemu-iotests/group   |   2 +-
 13 files changed, 173 insertions(+), 93 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v4 1/7] block: Add status callback to bdrv_amend_options()

[Max Reitz]

Depending on the changed options and the image format,
bdrv_amend_options() may take a significant amount of time. In these
cases, a way to be informed about the operation's status is desirable.

Since the operation is rather complex and may fundamentally change the
image, implementing it as AIO or a coroutine does not seem feasible. On
the other hand, implementing it as a block job would be significantly
more difficult than a simple callback and would not add benefits other
than progress report to the amending operation, because it should not
actually be run as a block job at all.

A callback may not be very pretty, but it's very easy to implement and
perfectly fits its purpose here.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>
---
 block.c                   | 5 +++--
 block/qcow2.c             | 3 ++-
 include/block/block.h     | 8 +++++++-
 include/block/block_int.h | 3 ++-
 qemu-img.c                | 2 +-
 5 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index 88f6d9b..4228cb7 100644
--- a/block.c
+++ b/block.c
@@ -5750,12 +5750,13 @@ void bdrv_add_before_write_notifier(BlockDriverState *bs,
     notifier_with_return_list_add(&bs->before_write_notifiers, notifier);
 }
 
-int bdrv_amend_options(BlockDriverState *bs, QemuOpts *opts)
+int bdrv_amend_options(BlockDriverState *bs, QemuOpts *opts,
+                       BlockDriverAmendStatusCB *status_cb)
 {
     if (!bs->drv->bdrv_amend_options) {
         return -ENOTSUP;
     }
-    return bs->drv->bdrv_amend_options(bs, opts);
+    return bs->drv->bdrv_amend_options(bs, opts, status_cb);
 }
 
 /* This function will be called by the bdrv_recurse_is_first_non_filter method
diff --git a/block/qcow2.c b/block/qcow2.c
index d031515..a121a63 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2424,7 +2424,8 @@ static int qcow2_downgrade(BlockDriverState *bs, int target_version)
     return 0;
 }
 
-static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts)
+static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
+                               BlockDriverAmendStatusCB *status_cb)
 {
     BDRVQcowState *s = bs->opaque;
     int old_version = s->qcow_version, new_version = old_version;
diff --git a/include/block/block.h b/include/block/block.h
index 341054d..2ff96e5 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -268,7 +268,13 @@ typedef enum {
 
 int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix);
 
-int bdrv_amend_options(BlockDriverState *bs_new, QemuOpts *opts);
+/* The units of offset and total_work_size may be chosen arbitrarily by the
+ * block driver; total_work_size may change during the course of the amendment
+ * operation */
+typedef void BlockDriverAmendStatusCB(BlockDriverState *bs, int64_t offset,
+                                      int64_t total_work_size);
+int bdrv_amend_options(BlockDriverState *bs_new, QemuOpts *opts,
+                       BlockDriverAmendStatusCB *status_cb);
 
 /* external snapshots */
 bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8898c6c..df9e349 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -232,7 +232,8 @@ struct BlockDriver {
     int (*bdrv_check)(BlockDriverState* bs, BdrvCheckResult *result,
         BdrvCheckMode fix);
 
-    int (*bdrv_amend_options)(BlockDriverState *bs, QemuOpts *opts);
+    int (*bdrv_amend_options)(BlockDriverState *bs, QemuOpts *opts,
+                              BlockDriverAmendStatusCB *status_cb);
 
     void (*bdrv_debug_event)(BlockDriverState *bs, BlkDebugEvent event);
 
diff --git a/qemu-img.c b/qemu-img.c
index 731502c..58973ef 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2867,7 +2867,7 @@ static int img_amend(int argc, char **argv)
         goto out;
     }
 
-    ret = bdrv_amend_options(bs, opts);
+    ret = bdrv_amend_options(bs, opts, NULL);
     if (ret < 0) {
         error_report("Error while amending options: %s", strerror(-ret));
         goto out;
-- 
1.9.3

[Qemu-devel] [PATCH v4 2/7] qemu-img: Add progress output for amend

[Max Reitz]

Now that bdrv_amend_options() supports a status callback, use it to
display a progress report.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>
---
 qemu-img-cmds.hx |  4 ++--
 qemu-img.c       | 25 ++++++++++++++++++++++---
 qemu-img.texi    |  2 +-
 3 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 55aec6b..d83f3d0 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -70,8 +70,8 @@ STEXI
 ETEXI
 
 DEF("amend", img_amend,
-    "amend [-q] [-f fmt] [-t cache] -o options filename")
+    "amend [-p] [-q] [-f fmt] [-t cache] -o options filename")
 STEXI
-@item amend [-q] [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
+@item amend [-p] [-q] [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
 @end table
 ETEXI
diff --git a/qemu-img.c b/qemu-img.c
index 58973ef..9ec2c28 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2770,6 +2770,12 @@ out:
     return 0;
 }
 
+static void amend_status_cb(BlockDriverState *bs,
+                            int64_t offset, int64_t total_work_size)
+{
+    qemu_progress_print(100.f * offset / total_work_size, 0);
+}
+
 static int img_amend(int argc, char **argv)
 {
     int c, ret = 0;
@@ -2778,13 +2784,13 @@ static int img_amend(int argc, char **argv)
     QemuOpts *opts = NULL;
     const char *fmt = NULL, *filename, *cache;
     int flags;
-    bool quiet = false;
+    bool quiet = false, progress = false;
     BlockBackend *blk = NULL;
     BlockDriverState *bs = NULL;
 
     cache = BDRV_DEFAULT_CACHE;
     for (;;) {
-        c = getopt(argc, argv, "ho:f:t:q");
+        c = getopt(argc, argv, "ho:f:t:pq");
         if (c == -1) {
             break;
         }
@@ -2814,6 +2820,9 @@ static int img_amend(int argc, char **argv)
             case 't':
                 cache = optarg;
                 break;
+            case 'p':
+                progress = true;
+                break;
             case 'q':
                 quiet = true;
                 break;
@@ -2824,6 +2833,11 @@ static int img_amend(int argc, char **argv)
         error_exit("Must specify options (-o)");
     }
 
+    if (quiet) {
+        progress = false;
+    }
+    qemu_progress_init(progress, 1.0);
+
     filename = (optind == argc - 1) ? argv[argc - 1] : NULL;
     if (fmt && has_help_option(options)) {
         /* If a format is explicitly specified (and possibly no filename is
@@ -2867,13 +2881,18 @@ static int img_amend(int argc, char **argv)
         goto out;
     }
 
-    ret = bdrv_amend_options(bs, opts, NULL);
+    /* In case the driver does not call amend_status_cb() */
+    qemu_progress_print(0.f, 0);
+    ret = bdrv_amend_options(bs, opts, &amend_status_cb);
+    qemu_progress_print(100.f, 0);
     if (ret < 0) {
         error_report("Error while amending options: %s", strerror(-ret));
         goto out;
     }
 
 out:
+    qemu_progress_end();
+
     blk_unref(blk);
     qemu_opts_del(opts);
     qemu_opts_free(create_opts);
diff --git a/qemu-img.texi b/qemu-img.texi
index e3ef4a0..1889b5d 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -398,7 +398,7 @@ After using this command to grow a disk image, you must use file system and
 partitioning tools inside the VM to actually begin using the new space on the
 device.
 
-@item amend [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
+@item amend [-p] [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
 
 Amends the image format specific @var{options} for the image file
 @var{filename}. Not all file formats support this operation.
-- 
1.9.3

[Qemu-devel] [PATCH v4 3/7] qemu-img: Fix insignificant memleak

[Max Reitz]

As soon as options is set in img_amend(), it needs to be freed before
the function returns. This leak is rather insignificant, as qemu-img
will exit subsequently anyway, but there's no point in not fixing it.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
---
 qemu-img.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/qemu-img.c b/qemu-img.c
index 9ec2c28..a60da8c 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2847,7 +2847,9 @@ static int img_amend(int argc, char **argv)
     }
 
     if (optind != argc - 1) {
-        error_exit("Expecting one image file name");
+        error_report("Expecting one image file name");
+        ret = -1;
+        goto out;
     }
 
     flags = BDRV_O_FLAGS | BDRV_O_RDWR;
-- 
1.9.3

[Qemu-devel] [PATCH v4 4/7] block/qcow2: Implement status CB for amend

[Max Reitz]

The only really time-consuming operation potentially performed by
qcow2_amend_options() is zero cluster expansion when downgrading qcow2
images from compat=1.1 to compat=0.10, so report status of that
operation and that operation only through the status CB.

For this, approximate the progress as the number of L1 entries visited
during the operation.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>
---
 block/qcow2-cluster.c | 37 +++++++++++++++++++++++++++++++++----
 block/qcow2.c         |  7 ++++---
 block/qcow2.h         |  3 ++-
 3 files changed, 39 insertions(+), 8 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 4d888c7..6445c79 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1611,10 +1611,17 @@ fail:
  * zero expansion (i.e., has been filled with zeroes and is referenced from an
  * L2 table). nb_clusters contains the total cluster count of the image file,
  * i.e., the number of bits in expanded_clusters.
+ *
+ * l1_entries and *visited_l1_entries are used to keep track of progress for
+ * status_cb(). l1_entries contains the total number of L1 entries and
+ * *visited_l1_entries counts all visited L1 entries.
  */
 static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
                                       int l1_size, uint8_t **expanded_clusters,
-                                      uint64_t *nb_clusters)
+                                      uint64_t *nb_clusters,
+                                      int64_t *visited_l1_entries,
+                                      int64_t l1_entries,
+                                      BlockDriverAmendStatusCB *status_cb)
 {
     BDRVQcowState *s = bs->opaque;
     bool is_active_l1 = (l1_table == s->l1_table);
@@ -1637,6 +1644,10 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
 
         if (!l2_offset) {
             /* unallocated */
+            (*visited_l1_entries)++;
+            if (status_cb) {
+                status_cb(bs, *visited_l1_entries, l1_entries);
+            }
             continue;
         }
 
@@ -1769,6 +1780,11 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
                 }
             }
         }
+
+        (*visited_l1_entries)++;
+        if (status_cb) {
+            status_cb(bs, *visited_l1_entries, l1_entries);
+        }
     }
 
     ret = 0;
@@ -1795,15 +1811,24 @@ fail:
  * allocation for pre-allocated ones). This is important for downgrading to a
  * qcow2 version which doesn't yet support metadata zero clusters.
  */
-int qcow2_expand_zero_clusters(BlockDriverState *bs)
+int qcow2_expand_zero_clusters(BlockDriverState *bs,
+                               BlockDriverAmendStatusCB *status_cb)
 {
     BDRVQcowState *s = bs->opaque;
     uint64_t *l1_table = NULL;
     uint64_t nb_clusters;
+    int64_t l1_entries = 0, visited_l1_entries = 0;
     uint8_t *expanded_clusters;
     int ret;
     int i, j;
 
+    if (status_cb) {
+        l1_entries = s->l1_size;
+        for (i = 0; i < s->nb_snapshots; i++) {
+            l1_entries += s->snapshots[i].l1_size;
+        }
+    }
+
     nb_clusters = size_to_clusters(s, bs->file->total_sectors *
                                    BDRV_SECTOR_SIZE);
     expanded_clusters = g_try_malloc0((nb_clusters + 7) / 8);
@@ -1813,7 +1838,9 @@ int qcow2_expand_zero_clusters(BlockDriverState *bs)
     }
 
     ret = expand_zero_clusters_in_l1(bs, s->l1_table, s->l1_size,
-                                     &expanded_clusters, &nb_clusters);
+                                     &expanded_clusters, &nb_clusters,
+                                     &visited_l1_entries, l1_entries,
+                                     status_cb);
     if (ret < 0) {
         goto fail;
     }
@@ -1847,7 +1874,9 @@ int qcow2_expand_zero_clusters(BlockDriverState *bs)
         }
 
         ret = expand_zero_clusters_in_l1(bs, l1_table, s->snapshots[i].l1_size,
-                                         &expanded_clusters, &nb_clusters);
+                                         &expanded_clusters, &nb_clusters,
+                                         &visited_l1_entries, l1_entries,
+                                         status_cb);
         if (ret < 0) {
             goto fail;
         }
diff --git a/block/qcow2.c b/block/qcow2.c
index a121a63..cf6452e 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2361,7 +2361,8 @@ static int qcow2_load_vmstate(BlockDriverState *bs, uint8_t *buf,
  * Downgrades an image's version. To achieve this, any incompatible features
  * have to be removed.
  */
-static int qcow2_downgrade(BlockDriverState *bs, int target_version)
+static int qcow2_downgrade(BlockDriverState *bs, int target_version,
+                           BlockDriverAmendStatusCB *status_cb)
 {
     BDRVQcowState *s = bs->opaque;
     int current_version = s->qcow_version;
@@ -2410,7 +2411,7 @@ static int qcow2_downgrade(BlockDriverState *bs, int target_version)
     /* clearing autoclear features is trivial */
     s->autoclear_features = 0;
 
-    ret = qcow2_expand_zero_clusters(bs);
+    ret = qcow2_expand_zero_clusters(bs, status_cb);
     if (ret < 0) {
         return ret;
     }
@@ -2503,7 +2504,7 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
                 return ret;
             }
         } else {
-            ret = qcow2_downgrade(bs, new_version);
+            ret = qcow2_downgrade(bs, new_version, status_cb);
             if (ret < 0) {
                 return ret;
             }
diff --git a/block/qcow2.h b/block/qcow2.h
index 577ccd1..ddefa04 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -537,7 +537,8 @@ int qcow2_discard_clusters(BlockDriverState *bs, uint64_t offset,
     int nb_sectors, enum qcow2_discard_type type);
 int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int nb_sectors);
 
-int qcow2_expand_zero_clusters(BlockDriverState *bs);
+int qcow2_expand_zero_clusters(BlockDriverState *bs,
+                               BlockDriverAmendStatusCB *status_cb);
 
 /* qcow2-snapshot.c functions */
 int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info);
-- 
1.9.3

[Qemu-devel] [PATCH v4 5/7] block/qcow2: Make get_refcount() global

[Max Reitz]

Reading the refcount of a cluster is an operation which can be useful in
all of the qcow2 code, so make that function globally available.

While touching this function, amend the comment describing the "addend"
parameter: It is (no longer, if it ever was) necessary to have it set to
-1 or 1; any value is fine.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>
---
 block/qcow2-refcount.c | 26 +++++++++++++-------------
 block/qcow2.h          |  2 ++
 2 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 1477031..9afdb40 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -91,7 +91,7 @@ static int load_refcount_block(BlockDriverState *bs,
  * return value is the refcount of the cluster, negative values are -errno
  * and indicate an error.
  */
-static int get_refcount(BlockDriverState *bs, int64_t cluster_index)
+int qcow2_get_refcount(BlockDriverState *bs, int64_t cluster_index)
 {
     BDRVQcowState *s = bs->opaque;
     uint64_t refcount_table_index, block_index;
@@ -629,8 +629,7 @@ fail:
 }
 
 /*
- * Increases or decreases the refcount of a given cluster by one.
- * addend must be 1 or -1.
+ * Increases or decreases the refcount of a given cluster.
  *
  * If the return value is non-negative, it is the new refcount of the cluster.
  * If it is negative, it is -errno and indicates an error.
@@ -649,7 +648,7 @@ int qcow2_update_cluster_refcount(BlockDriverState *bs,
         return ret;
     }
 
-    return get_refcount(bs, cluster_index);
+    return qcow2_get_refcount(bs, cluster_index);
 }
 
 
@@ -670,7 +669,7 @@ static int64_t alloc_clusters_noref(BlockDriverState *bs, uint64_t size)
 retry:
     for(i = 0; i < nb_clusters; i++) {
         uint64_t next_cluster_index = s->free_cluster_index++;
-        refcount = get_refcount(bs, next_cluster_index);
+        refcount = qcow2_get_refcount(bs, next_cluster_index);
 
         if (refcount < 0) {
             return refcount;
@@ -734,7 +733,7 @@ int qcow2_alloc_clusters_at(BlockDriverState *bs, uint64_t offset,
         /* Check how many clusters there are free */
         cluster_index = offset >> s->cluster_bits;
         for(i = 0; i < nb_clusters; i++) {
-            refcount = get_refcount(bs, cluster_index++);
+            refcount = qcow2_get_refcount(bs, cluster_index++);
 
             if (refcount < 0) {
                 return refcount;
@@ -981,7 +980,7 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
                                     cluster_index, addend,
                                     QCOW2_DISCARD_SNAPSHOT);
                         } else {
-                            refcount = get_refcount(bs, cluster_index);
+                            refcount = qcow2_get_refcount(bs, cluster_index);
                         }
 
                         if (refcount < 0) {
@@ -1021,7 +1020,7 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
                 refcount = qcow2_update_cluster_refcount(bs, l2_offset >>
                         s->cluster_bits, addend, QCOW2_DISCARD_SNAPSHOT);
             } else {
-                refcount = get_refcount(bs, l2_offset >> s->cluster_bits);
+                refcount = qcow2_get_refcount(bs, l2_offset >> s->cluster_bits);
             }
             if (refcount < 0) {
                 ret = refcount;
@@ -1332,8 +1331,8 @@ fail:
  * Checks the OFLAG_COPIED flag for all L1 and L2 entries.
  *
  * This function does not print an error message nor does it increment
- * check_errors if get_refcount fails (this is because such an error will have
- * been already detected and sufficiently signaled by the calling function
+ * check_errors if qcow2_get_refcount fails (this is because such an error will
+ * have been already detected and sufficiently signaled by the calling function
  * (qcow2_check_refcounts) by the time this function is called).
  */
 static int check_oflag_copied(BlockDriverState *bs, BdrvCheckResult *res,
@@ -1354,7 +1353,7 @@ static int check_oflag_copied(BlockDriverState *bs, BdrvCheckResult *res,
             continue;
         }
 
-        refcount = get_refcount(bs, l2_offset >> s->cluster_bits);
+        refcount = qcow2_get_refcount(bs, l2_offset >> s->cluster_bits);
         if (refcount < 0) {
             /* don't print message nor increment check_errors */
             continue;
@@ -1396,7 +1395,8 @@ static int check_oflag_copied(BlockDriverState *bs, BdrvCheckResult *res,
 
             if ((cluster_type == QCOW2_CLUSTER_NORMAL) ||
                 ((cluster_type == QCOW2_CLUSTER_ZERO) && (data_offset != 0))) {
-                refcount = get_refcount(bs, data_offset >> s->cluster_bits);
+                refcount = qcow2_get_refcount(bs,
+                                              data_offset >> s->cluster_bits);
                 if (refcount < 0) {
                     /* don't print message nor increment check_errors */
                     continue;
@@ -1632,7 +1632,7 @@ static void compare_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
     int refcount1, refcount2, ret;
 
     for (i = 0, *highest_cluster = 0; i < nb_clusters; i++) {
-        refcount1 = get_refcount(bs, i);
+        refcount1 = qcow2_get_refcount(bs, i);
         if (refcount1 < 0) {
             fprintf(stderr, "Can't get refcount for cluster %" PRId64 ": %s\n",
                 i, strerror(-refcount1));
diff --git a/block/qcow2.h b/block/qcow2.h
index ddefa04..65949bd 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -487,6 +487,8 @@ void qcow2_signal_corruption(BlockDriverState *bs, bool fatal, int64_t offset,
 int qcow2_refcount_init(BlockDriverState *bs);
 void qcow2_refcount_close(BlockDriverState *bs);
 
+int qcow2_get_refcount(BlockDriverState *bs, int64_t cluster_index);
+
 int qcow2_update_cluster_refcount(BlockDriverState *bs, int64_t cluster_index,
                                   int addend, enum qcow2_discard_type type);
 
-- 
1.9.3

[Qemu-devel] [PATCH v4 6/7] block/qcow2: Simplify shared L2 handling in amend

[Max Reitz]

Currently, we have a bitmap for keeping track of which clusters have
been created during the zero cluster expansion process. This was
necessary because we need to properly increase the refcount for shared
L2 tables.

However, now we can simply take the L2 refcount and use it for the
cluster allocated for expansion. This will be the correct refcount and
therefore we don't have to remember that cluster having been allocated
any more.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>
---
 block/qcow2-cluster.c | 94 +++++++++++++++------------------------------------
 1 file changed, 28 insertions(+), 66 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 6445c79..3c82507 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1606,20 +1606,12 @@ fail:
  * Expands all zero clusters in a specific L1 table (or deallocates them, for
  * non-backed non-pre-allocated zero clusters).
  *
- * expanded_clusters is a bitmap where every bit corresponds to one cluster in
- * the image file; a bit gets set if the corresponding cluster has been used for
- * zero expansion (i.e., has been filled with zeroes and is referenced from an
- * L2 table). nb_clusters contains the total cluster count of the image file,
- * i.e., the number of bits in expanded_clusters.
- *
  * l1_entries and *visited_l1_entries are used to keep track of progress for
  * status_cb(). l1_entries contains the total number of L1 entries and
  * *visited_l1_entries counts all visited L1 entries.
  */
 static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
-                                      int l1_size, uint8_t **expanded_clusters,
-                                      uint64_t *nb_clusters,
-                                      int64_t *visited_l1_entries,
+                                      int l1_size, int64_t *visited_l1_entries,
                                       int64_t l1_entries,
                                       BlockDriverAmendStatusCB *status_cb)
 {
@@ -1641,6 +1633,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;
+        int l2_refcount;
 
         if (!l2_offset) {
             /* unallocated */
@@ -1664,33 +1657,19 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
             goto fail;
         }
 
+        l2_refcount = qcow2_get_refcount(bs, l2_offset >> s->cluster_bits);
+        if (l2_refcount < 0) {
+            ret = l2_refcount;
+            goto fail;
+        }
+
         for (j = 0; j < s->l2_size; j++) {
             uint64_t l2_entry = be64_to_cpu(l2_table[j]);
-            int64_t offset = l2_entry & L2E_OFFSET_MASK, cluster_index;
+            int64_t offset = l2_entry & L2E_OFFSET_MASK;
             int cluster_type = qcow2_get_cluster_type(l2_entry);
             bool preallocated = offset != 0;
 
-            if (cluster_type == QCOW2_CLUSTER_NORMAL) {
-                cluster_index = offset >> s->cluster_bits;
-                assert((cluster_index >= 0) && (cluster_index < *nb_clusters));
-                if ((*expanded_clusters)[cluster_index / 8] &
-                    (1 << (cluster_index % 8))) {
-                    /* Probably a shared L2 table; this cluster was a zero
-                     * cluster which has been expanded, its refcount
-                     * therefore most likely requires an update. */
-                    ret = qcow2_update_cluster_refcount(bs, cluster_index, 1,
-                                                        QCOW2_DISCARD_NEVER);
-                    if (ret < 0) {
-                        goto fail;
-                    }
-                    /* Since we just increased the refcount, the COPIED flag may
-                     * no longer be set. */
-                    l2_table[j] = cpu_to_be64(l2_entry & ~QCOW_OFLAG_COPIED);
-                    l2_dirty = true;
-                }
-                continue;
-            }
-            else if (qcow2_get_cluster_type(l2_entry) != QCOW2_CLUSTER_ZERO) {
+            if (cluster_type != QCOW2_CLUSTER_ZERO) {
                 continue;
             }
 
@@ -1708,6 +1687,19 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
                     ret = offset;
                     goto fail;
                 }
+
+                if (l2_refcount > 1) {
+                    /* For shared L2 tables, set the refcount accordingly (it is
+                     * already 1 and needs to be l2_refcount) */
+                    ret = qcow2_update_cluster_refcount(bs,
+                            offset >> s->cluster_bits, l2_refcount - 1,
+                            QCOW2_DISCARD_OTHER);
+                    if (ret < 0) {
+                        qcow2_free_clusters(bs, offset, s->cluster_size,
+                                            QCOW2_DISCARD_OTHER);
+                        goto fail;
+                    }
+                }
             }
 
             ret = qcow2_pre_write_overlap_check(bs, 0, offset, s->cluster_size);
@@ -1729,29 +1721,12 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
                 goto fail;
             }
 
-            l2_table[j] = cpu_to_be64(offset | QCOW_OFLAG_COPIED);
-            l2_dirty = true;
-
-            cluster_index = offset >> s->cluster_bits;
-
-            if (cluster_index >= *nb_clusters) {
-                uint64_t old_bitmap_size = (*nb_clusters + 7) / 8;
-                uint64_t new_bitmap_size;
-                /* The offset may lie beyond the old end of the underlying image
-                 * file for growable files only */
-                assert(bs->file->growable);
-                *nb_clusters = size_to_clusters(s, bs->file->total_sectors *
-                                                BDRV_SECTOR_SIZE);
-                new_bitmap_size = (*nb_clusters + 7) / 8;
-                *expanded_clusters = g_realloc(*expanded_clusters,
-                                               new_bitmap_size);
-                /* clear the newly allocated space */
-                memset(&(*expanded_clusters)[old_bitmap_size], 0,
-                       new_bitmap_size - old_bitmap_size);
+            if (l2_refcount == 1) {
+                l2_table[j] = cpu_to_be64(offset | QCOW_OFLAG_COPIED);
+            } else {
+                l2_table[j] = cpu_to_be64(offset);
             }
-
-            assert((cluster_index >= 0) && (cluster_index < *nb_clusters));
-            (*expanded_clusters)[cluster_index / 8] |= 1 << (cluster_index % 8);
+            l2_dirty = true;
         }
 
         if (is_active_l1) {
@@ -1816,9 +1791,7 @@ int qcow2_expand_zero_clusters(BlockDriverState *bs,
 {
     BDRVQcowState *s = bs->opaque;
     uint64_t *l1_table = NULL;
-    uint64_t nb_clusters;
     int64_t l1_entries = 0, visited_l1_entries = 0;
-    uint8_t *expanded_clusters;
     int ret;
     int i, j;
 
@@ -1829,16 +1802,7 @@ int qcow2_expand_zero_clusters(BlockDriverState *bs,
         }
     }
 
-    nb_clusters = size_to_clusters(s, bs->file->total_sectors *
-                                   BDRV_SECTOR_SIZE);
-    expanded_clusters = g_try_malloc0((nb_clusters + 7) / 8);
-    if (expanded_clusters == NULL) {
-        ret = -ENOMEM;
-        goto fail;
-    }
-
     ret = expand_zero_clusters_in_l1(bs, s->l1_table, s->l1_size,
-                                     &expanded_clusters, &nb_clusters,
                                      &visited_l1_entries, l1_entries,
                                      status_cb);
     if (ret < 0) {
@@ -1874,7 +1838,6 @@ int qcow2_expand_zero_clusters(BlockDriverState *bs,
         }
 
         ret = expand_zero_clusters_in_l1(bs, l1_table, s->snapshots[i].l1_size,
-                                         &expanded_clusters, &nb_clusters,
                                          &visited_l1_entries, l1_entries,
                                          status_cb);
         if (ret < 0) {
@@ -1885,7 +1848,6 @@ int qcow2_expand_zero_clusters(BlockDriverState *bs,
     ret = 0;
 
 fail:
-    g_free(expanded_clusters);
     g_free(l1_table);
     return ret;
 }
-- 
1.9.3

[Qemu-devel] [PATCH v4 7/7] iotests: Expand test 061

[Max Reitz]

Add some tests for progress output to 061.

Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>
---
 tests/qemu-iotests/061     | 25 +++++++++++++++++++++++++
 tests/qemu-iotests/061.out | 30 ++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |  2 +-
 3 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/061 b/tests/qemu-iotests/061
index ab98def..8d37f8a 100755
--- a/tests/qemu-iotests/061
+++ b/tests/qemu-iotests/061
@@ -209,6 +209,31 @@ $QEMU_IMG amend -o "compat=0.10" "$TEST_IMG"
 _check_test_img
 $QEMU_IO -c "read -P 0 0 64M" "$TEST_IMG" | _filter_qemu_io
 
+echo
+echo "=== Testing progress report without snapshot ==="
+echo
+IMGOPTS="compat=1.1" TEST_IMG="$TEST_IMG.base" _make_test_img 4G
+IMGOPTS="compat=1.1" _make_test_img -b "$TEST_IMG.base" 4G
+$QEMU_IO -c "write -z 0  64k" \
+         -c "write -z 1G 64k" \
+         -c "write -z 2G 64k" \
+         -c "write -z 3G 64k" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IMG amend -p -o "compat=0.10" "$TEST_IMG"
+_check_test_img
+
+echo
+echo "=== Testing progress report with snapshot ==="
+echo
+IMGOPTS="compat=1.1" TEST_IMG="$TEST_IMG.base" _make_test_img 4G
+IMGOPTS="compat=1.1" _make_test_img -b "$TEST_IMG.base" 4G
+$QEMU_IO -c "write -z 0  64k" \
+         -c "write -z 1G 64k" \
+         -c "write -z 2G 64k" \
+         -c "write -z 3G 64k" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IMG snapshot -c foo "$TEST_IMG"
+$QEMU_IMG amend -p -o "compat=0.10" "$TEST_IMG"
+_check_test_img
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/061.out b/tests/qemu-iotests/061.out
index 4ae6472..f8f82e9 100644
--- a/tests/qemu-iotests/061.out
+++ b/tests/qemu-iotests/061.out
@@ -390,4 +390,34 @@ wrote 67108864/67108864 bytes at offset 0
 No errors were found on the image.
 read 67108864/67108864 bytes at offset 0
 64 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Testing progress report without snapshot ===
+
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=4294967296 
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=4294967296 backing_file='TEST_DIR/t.IMGFMT.base' 
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 65536/65536 bytes at offset 1073741824
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 65536/65536 bytes at offset 2147483648
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 65536/65536 bytes at offset 3221225472
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+    (0.00/100%)\r    (12.50/100%)\r    (25.00/100%)\r    (37.50/100%)\r    (50.00/100%)\r    (62.50/100%)\r    (75.00/100%)\r    (87.50/100%)\r    (100.00/100%)\r    (100.00/100%)
+No errors were found on the image.
+
+=== Testing progress report with snapshot ===
+
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=4294967296 
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=4294967296 backing_file='TEST_DIR/t.IMGFMT.base' 
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 65536/65536 bytes at offset 1073741824
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 65536/65536 bytes at offset 2147483648
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 65536/65536 bytes at offset 3221225472
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+    (0.00/100%)\r    (6.25/100%)\r    (12.50/100%)\r    (18.75/100%)\r    (25.00/100%)\r    (31.25/100%)\r    (37.50/100%)\r    (43.75/100%)\r    (50.00/100%)\r    (56.25/100%)\r    (62.50/100%)\r    (68.75/100%)\r    (75.00/100%)\r    (81.25/100%)\r    (87.50/100%)\r    (93.75/100%)\r    (100.00/100%)\r    (100.00/100%)
+No errors were found on the image.
 *** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 9bbd5d3..6e67da4 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -67,7 +67,7 @@
 058 rw auto quick
 059 rw auto quick
 060 rw auto quick
-061 rw auto quick
+061 rw auto
 062 rw auto quick
 063 rw auto quick
 064 rw auto quick
-- 
1.9.3

Re: [Qemu-devel] [PATCH v4 7/7] iotests: Expand test 061

[Kevin Wolf]

Am 27.10.2014 um 11:12 hat Max Reitz geschrieben:
> Add some tests for progress output to 061.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>

> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index 9bbd5d3..6e67da4 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -67,7 +67,7 @@
>  058 rw auto quick
>  059 rw auto quick
>  060 rw auto quick
> -061 rw auto quick
> +061 rw auto
>  062 rw auto quick
>  063 rw auto quick
>  064 rw auto quick

It's still quick enough for me, why the change?

Kevin

Re: [Qemu-devel] [PATCH v4 0/7] block/qcow2: Improve zero cluster expansion

[Kevin Wolf]

Am 27.10.2014 um 11:12 hat Max Reitz geschrieben:
> The main purpose of this series is to add a progress report to
> qemu-img amend. This is achieved by adding a callback function to
> bdrv_amend_options() - the reasons for this choice are explained in
> patch 1.
> 
> While adapting qcow2's expand_zero_clusters_in_l1() accordingly, I
> noticed a way to simplify it and get rid of the rather ugly bitmap used
> there (patch 6).

Had a question on patch 7, but regardless:

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH v4 7/7] iotests: Expand test 061

[Max Reitz]

On 2014-10-28 at 17:38, Kevin Wolf wrote:
> Am 27.10.2014 um 11:12 hat Max Reitz geschrieben:
>> Add some tests for progress output to 061.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>
>> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
>> index 9bbd5d3..6e67da4 100644
>> --- a/tests/qemu-iotests/group
>> +++ b/tests/qemu-iotests/group
>> @@ -67,7 +67,7 @@
>>   058 rw auto quick
>>   059 rw auto quick
>>   060 rw auto quick
>> -061 rw auto quick
>> +061 rw auto
>>   062 rw auto quick
>>   063 rw auto quick
>>   064 rw auto quick
> It's still quick enough for me, why the change?

Wasn't for me. 12 seconds here on my laptop, probably faster at home 
where I have an SSD, but I'd rather use a plain HDD for measure 
regarding what is quick and what isn't.

Thank you for reviewing!

Max

Re: [Qemu-devel] [PATCH v4 0/7] block/qcow2: Improve zero cluster expansion

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3205 bytes --]

On Mon, Oct 27, 2014 at 11:12:49AM +0100, Max Reitz wrote:
> The main purpose of this series is to add a progress report to
> qemu-img amend. This is achieved by adding a callback function to
> bdrv_amend_options() - the reasons for this choice are explained in
> patch 1.
> 
> While adapting qcow2's expand_zero_clusters_in_l1() accordingly, I
> noticed a way to simplify it and get rid of the rather ugly bitmap used
> there (patch 6).
> 
> 
> Changes from v3 (v4 is a rebase of v3 on current master)
> - Patch 2: There is BlockBackend now (only contextual)
> - Patch 4: An instance of g_try_malloc0() instead of g_malloc0() (only
>   contextual)
> - Patch 5: Changes in the qcow2 check functions due to my "qcow2: Fix
>   image repairing" series (only contextual)
> - Patch 6: Originally, contained a hunk which removed a g_malloc0() and
>   the buffer size calculation before it; now, it's a g_try_malloc0() and
>   therefore the hunk has to remove the error handling as well. This is a
>   functional change, but I decided to keep the R-bs anyway because it is
>   really straightforward.
> 
> Since with the exception of patch 6 all of these were contextual changes
> (and the change to patch 6 was really straightforward), I kept all the
> Reviewed-bys and this series is therefore still fully reviewed.
> 
> 
> git-backport-diff against v3:
> 
> Key:
> [----] : patches are identical
> [####] : number of functional differences between upstream/downstream patch
> [down] : patch is downstream-only
> The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
> 
> 001/7:[----] [--] 'block: Add status callback to bdrv_amend_options()'
> 002/7:[----] [-C] 'qemu-img: Add progress output for amend'
> 003/7:[----] [--] 'qemu-img: Fix insignificant memleak'
> 004/7:[----] [-C] 'block/qcow2: Implement status CB for amend'
> 005/7:[----] [-C] 'block/qcow2: Make get_refcount() global'
> 006/7:[0006] [FC] 'block/qcow2: Simplify shared L2 handling in amend'
> 007/7:[----] [--] 'iotests: Expand test 061'
> 
> 
> Max Reitz (7):
>   block: Add status callback to bdrv_amend_options()
>   qemu-img: Add progress output for amend
>   qemu-img: Fix insignificant memleak
>   block/qcow2: Implement status CB for amend
>   block/qcow2: Make get_refcount() global
>   block/qcow2: Simplify shared L2 handling in amend
>   iotests: Expand test 061
> 
>  block.c                    |   5 +-
>  block/qcow2-cluster.c      | 117 +++++++++++++++++++++------------------------
>  block/qcow2-refcount.c     |  26 +++++-----
>  block/qcow2.c              |  10 ++--
>  block/qcow2.h              |   5 +-
>  include/block/block.h      |   8 +++-
>  include/block/block_int.h  |   3 +-
>  qemu-img-cmds.hx           |   4 +-
>  qemu-img.c                 |  29 +++++++++--
>  qemu-img.texi              |   2 +-
>  tests/qemu-iotests/061     |  25 ++++++++++
>  tests/qemu-iotests/061.out |  30 ++++++++++++
>  tests/qemu-iotests/group   |   2 +-
>  13 files changed, 173 insertions(+), 93 deletions(-)
> 
> -- 
> 1.9.3
> 
> 

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]