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

[Qemu-devel] [PATCH alt 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).


This series depends on v2 of my "qemu-img: Allow source cache mode
specification" series.


This series is an alternative of the one with the same name (and one
additional patch) without the tag "alt"; the main difference is that the
first version adds optimizations and be as exact as possible, whereas
this version rather tries to be as simple as possible (because the code
being touched is rarely ever used).

The differences are the following:
 - switched patches 4 and 5, because 5 (now 4) does not rely on 4 (now
   5) in this version
 - patch 5 (here: 4) uses a far simpler model to track progress (instead
   of counting the expanded zero clusters, it counts the number of
   visited L1 entries)
 - patch 6 had to be rebased on top of it
 - patch 7 has been dropped (it optimizes zero cluster expansion in
   exchange for more complicated code for a function which should be
   rarely ever used)
 - patch 8 (here: 7) had to be adapted accordingly:
   - dropped the test case for patch 7 (did not fail without it, but did
     not make any sense either)
   - different progress output due to the differences in patch 5 (here:
     4)


git-upstream-diff output against the non-alt version:

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:[----] [--] 'qemu-img: Add progress output for amend'
003/7:[----] [--] 'qemu-img: Fix insignifcant memleak'
004/7:[0137] [FC] 'block/qcow2: Implement status CB for amend'
005/7:[----] [--] 'block/qcow2: Make get_refcount() global'
006/7:[0011] [FC] 'block/qcow2: Simplify shared L2 handling in amend'
007/7:[0026] [FC] '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 insignifcant 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      | 114 +++++++++++++++++++++------------------------
 block/qcow2-refcount.c     |  23 ++++-----
 block/qcow2.c              |  10 ++--
 block/qcow2.h              |   5 +-
 include/block/block.h      |   5 +-
 include/block/block_int.h  |   3 +-
 qemu-img.c                 |  30 ++++++++++--
 tests/qemu-iotests/061     |  27 +++++++++++
 tests/qemu-iotests/061.out |  32 +++++++++++++
 tests/qemu-iotests/group   |   2 +-
 11 files changed, 171 insertions(+), 85 deletions(-)

-- 
2.0.3

[Qemu-devel] [PATCH alt 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 couroutine 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>
---
 block.c                   | 5 +++--
 block/qcow2.c             | 5 ++++-
 include/block/block.h     | 5 ++++-
 include/block/block_int.h | 3 ++-
 qemu-img.c                | 2 +-
 5 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index 3e252a2..4c8d4d8 100644
--- a/block.c
+++ b/block.c
@@ -5708,12 +5708,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 b0faa69..757f890 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2210,7 +2210,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;
@@ -2223,6 +2224,8 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts)
     int ret;
     QemuOptDesc *desc = opts->list->desc;
 
+    (void)status_cb;
+
     while (desc && desc->name) {
         if (!qemu_opt_find(opts, desc->name)) {
             /* only change explicitly defined options */
diff --git a/include/block/block.h b/include/block/block.h
index 32d3676..f2b1799 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -309,7 +309,10 @@ typedef enum {
 
 int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix);
 
-int bdrv_amend_options(BlockDriverState *bs_new, QemuOpts *opts);
+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 f6c3bef..5c5798d 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -228,7 +228,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 ef74ae4..90d6b79 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2827,7 +2827,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;
-- 
2.0.3

[Qemu-devel] [PATCH alt 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>
---
 qemu-img.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 90d6b79..a06f425 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2732,6 +2732,13 @@ out:
     return 0;
 }
 
+static void amend_status_cb(BlockDriverState *bs,
+                            int64_t offset, int64_t total_work_size)
+{
+    (void)bs;
+    qemu_progress_print(100.f * offset / total_work_size, 0);
+}
+
 static int img_amend(int argc, char **argv)
 {
     int c, ret = 0;
@@ -2740,12 +2747,12 @@ 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;
     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;
         }
@@ -2775,6 +2782,9 @@ static int img_amend(int argc, char **argv)
             case 't':
                 cache = optarg;
                 break;
+            case 'p':
+                progress = true;
+                break;
             case 'q':
                 quiet = true;
                 break;
@@ -2785,6 +2795,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
@@ -2827,13 +2842,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();
+
     if (bs) {
         bdrv_unref(bs);
     }
-- 
2.0.3

[Qemu-devel] [PATCH alt 3/7] qemu-img: Fix insignifcant 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 insignifcant, as qemu-img will
exit subsequently anyway, but there's no point in not fixing it.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qemu-img.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/qemu-img.c b/qemu-img.c
index a06f425..d73a68a 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2809,7 +2809,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;
-- 
2.0.3

[Qemu-devel] [PATCH alt 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>
---
 block/qcow2-cluster.c | 36 ++++++++++++++++++++++++++++++++----
 block/qcow2.c         |  9 ++++-----
 block/qcow2.h         |  3 ++-
 3 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 4208dc0..f8bec6f 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1548,10 +1548,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 ued 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);
@@ -1571,6 +1578,7 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
 
         if (!l2_offset) {
             /* unallocated */
+            (*visited_l1_entries)++;
             continue;
         }
 
@@ -1703,6 +1711,13 @@ 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 << (s->l2_bits + s->cluster_bits),
+                      l1_entries << (s->l2_bits + s->cluster_bits));
+        }
     }
 
     ret = 0;
@@ -1729,21 +1744,32 @@ 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_malloc0((nb_clusters + 7) / 8);
 
     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;
     }
@@ -1777,7 +1803,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 757f890..6e8c8ab 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2147,7 +2147,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;
@@ -2196,7 +2197,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;
     }
@@ -2224,8 +2225,6 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
     int ret;
     QemuOptDesc *desc = opts->list->desc;
 
-    (void)status_cb;
-
     while (desc && desc->name) {
         if (!qemu_opt_find(opts, desc->name)) {
             /* only change explicitly defined options */
@@ -2291,7 +2290,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 b49424b..1c4f9bf 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -522,7 +522,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);
-- 
2.0.3

[Qemu-devel] [PATCH alt 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.

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

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index cc6cf74..0c9887b 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -87,7 +87,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;
@@ -625,7 +625,7 @@ int qcow2_update_cluster_refcount(BlockDriverState *bs,
         return ret;
     }
 
-    return get_refcount(bs, cluster_index);
+    return qcow2_get_refcount(bs, cluster_index);
 }
 
 
@@ -646,7 +646,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;
@@ -710,7 +710,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;
@@ -927,7 +927,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) {
@@ -967,7 +967,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;
@@ -1243,8 +1243,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,
@@ -1265,7 +1265,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;
@@ -1307,7 +1307,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;
@@ -1597,7 +1598,7 @@ int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
 
     /* compare ref counts */
     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 1c4f9bf..c0e1b7b 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -472,6 +472,8 @@ int qcow2_update_header(BlockDriverState *bs);
 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);
 
-- 
2.0.3

[Qemu-devel] [PATCH alt 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>
---
 block/qcow2-cluster.c | 90 ++++++++++++++++-----------------------------------
 1 file changed, 28 insertions(+), 62 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index f8bec6f..e6bff40 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1543,20 +1543,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 ued 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)
 {
@@ -1575,6 +1567,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 */
@@ -1595,33 +1588,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;
             }
 
@@ -1639,6 +1618,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);
@@ -1660,29 +1652,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) {
@@ -1749,9 +1724,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;
 
@@ -1762,12 +1735,7 @@ int qcow2_expand_zero_clusters(BlockDriverState *bs,
         }
     }
 
-    nb_clusters = size_to_clusters(s, bs->file->total_sectors *
-                                   BDRV_SECTOR_SIZE);
-    expanded_clusters = g_malloc0((nb_clusters + 7) / 8);
-
     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) {
@@ -1803,7 +1771,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) {
@@ -1814,7 +1781,6 @@ int qcow2_expand_zero_clusters(BlockDriverState *bs,
     ret = 0;
 
 fail:
-    g_free(expanded_clusters);
     g_free(l1_table);
     return ret;
 }
-- 
2.0.3

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

[Max Reitz]

Add some tests for progress output to 061.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/061     | 27 +++++++++++++++++++++++++++
 tests/qemu-iotests/061.out | 32 ++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |  2 +-
 3 files changed, 60 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/061 b/tests/qemu-iotests/061
index ab98def..fbb5897 100755
--- a/tests/qemu-iotests/061
+++ b/tests/qemu-iotests/061
@@ -209,6 +209,33 @@ $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"
+# COW L2 table 0
+$QEMU_IO -c "write -z 64k 64k" "$TEST_IMG" | _filter_qemu_io
+$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 e372470..352cca3 100644
--- a/tests/qemu-iotests/061.out
+++ b/tests/qemu-iotests/061.out
@@ -384,4 +384,36 @@ 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%)
    (12.50/100%)
    (37.50/100%)
    (62.50/100%)
    (87.50/100%)
    (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)
+wrote 65536/65536 bytes at offset 65536
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+    (0.00/100%)
    (6.25/100%)
    (18.75/100%)
    (31.25/100%)
    (43.75/100%)
    (56.25/100%)
    (68.75/100%)
    (81.25/100%)
    (93.75/100%)
    (100.00/100%)
+No errors were found on the image.
 *** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 6e67f61..b27e2f9 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
-- 
2.0.3

Re: [Qemu-devel] [PATCH alt 3/7] qemu-img: Fix insignifcant memleak

[Eric Blake]

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

On 07/26/2014 01:22 PM, Max Reitz wrote:
> As soon as options is set in img_amend(), it needs to be freed before
> the function returns. This leak is rather insignifcant, as qemu-img will

s/insignifcant/insignificant/

> exit subsequently anyway, but there's no point in not fixing it.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qemu-img.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

1-3 look the same as in the other version of this series, so no further
comments on them. I'll try and review both series to see if either one
makes more sense as being more desirable.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

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

[Eric Blake]

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

On 07/26/2014 01:22 PM, Max Reitz wrote:
> 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>
> ---
>  block/qcow2-cluster.c | 36 ++++++++++++++++++++++++++++++++----
>  block/qcow2.c         |  9 ++++-----
>  block/qcow2.h         |  3 ++-
>  3 files changed, 38 insertions(+), 10 deletions(-)

Seems like a reasonable approximation.  The progress may appear to
change non-linearly (slower when a sequence of L1 entries visit all-zero
data, faster when the entries visit normal data), but we never promised
linear growth.

> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 4208dc0..f8bec6f 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1548,10 +1548,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 ued to keep track of progress for

s/ued/used/

Definitely looks simpler than your other approach.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH alt 3/7] qemu-img: Fix insignifcant memleak

[Max Reitz]

On 30.07.2014 16:58, Eric Blake wrote:
> On 07/26/2014 01:22 PM, Max Reitz wrote:
>> As soon as options is set in img_amend(), it needs to be freed before
>> the function returns. This leak is rather insignifcant, as qemu-img will
> s/insignifcant/insignificant/

I wonder how I was able to get it wrong the same way twice...

>> exit subsequently anyway, but there's no point in not fixing it.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   qemu-img.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
> 1-3 look the same as in the other version of this series, so no further
> comments on them. I'll try and review both series to see if either one
> makes more sense as being more desirable.

Thank you,

Max

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

[Benoît Canet]

The Saturday 26 Jul 2014 à 21:22:05 (+0200), Max Reitz wrote :

> 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 couroutine 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>
> ---
>  block.c                   | 5 +++--
>  block/qcow2.c             | 5 ++++-
>  include/block/block.h     | 5 ++++-
>  include/block/block_int.h | 3 ++-
>  qemu-img.c                | 2 +-
>  5 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 3e252a2..4c8d4d8 100644
> --- a/block.c
> +++ b/block.c
> @@ -5708,12 +5708,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 b0faa69..757f890 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2210,7 +2210,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;
> @@ -2223,6 +2224,8 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts)
>      int ret;
>      QemuOptDesc *desc = opts->list->desc;
>  
> +    (void)status_cb;

This look like a temporary hack to please the compiler.
Am I right ? Should be comment this ?

> +
>      while (desc && desc->name) {
>          if (!qemu_opt_find(opts, desc->name)) {
>              /* only change explicitly defined options */
> diff --git a/include/block/block.h b/include/block/block.h
> index 32d3676..f2b1799 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -309,7 +309,10 @@ typedef enum {
>  
>  int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix);
>  
> -int bdrv_amend_options(BlockDriverState *bs_new, QemuOpts *opts);
> +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 f6c3bef..5c5798d 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -228,7 +228,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 ef74ae4..90d6b79 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -2827,7 +2827,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;
> -- 
> 2.0.3
> 
> 

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

[Benoît Canet]

The Saturday 26 Jul 2014 à 21:22:06 (+0200), Max Reitz wrote :
> Now that bdrv_amend_options() supports a status callback, use it to
> display a progress report.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qemu-img.c | 26 +++++++++++++++++++++++---
>  1 file changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 90d6b79..a06f425 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -2732,6 +2732,13 @@ out:
>      return 0;
>  }
>  
> +static void amend_status_cb(BlockDriverState *bs,
> +                            int64_t offset, int64_t total_work_size)
> +{
> +    (void)bs;
This cast also look like a compiler pleasing thing.
Is it really required ?

> +    qemu_progress_print(100.f * offset / total_work_size, 0);
> +}
> +
>  static int img_amend(int argc, char **argv)
>  {
>      int c, ret = 0;
> @@ -2740,12 +2747,12 @@ 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;
>      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;
>          }
> @@ -2775,6 +2782,9 @@ static int img_amend(int argc, char **argv)
>              case 't':
>                  cache = optarg;
>                  break;
> +            case 'p':
> +                progress = true;
> +                break;
>              case 'q':
>                  quiet = true;
>                  break;
> @@ -2785,6 +2795,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
> @@ -2827,13 +2842,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();
> +
>      if (bs) {
>          bdrv_unref(bs);
>      }
> -- 
> 2.0.3
> 
> 

Re: [Qemu-devel] [PATCH alt 3/7] qemu-img: Fix insignifcant memleak

[Benoît Canet]

The Saturday 26 Jul 2014 à 21:22:07 (+0200), Max Reitz wrote :

> As soon as options is set in img_amend(), it needs to be freed before
> the function returns. This leak is rather insignifcant, as qemu-img will
> exit subsequently anyway, but there's no point in not fixing it.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qemu-img.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index a06f425..d73a68a 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -2809,7 +2809,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;
> -- 
> 2.0.3
> 
> 

Reviewed-by: Benoit Canet <benoit@irqsave.net>

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

[Benoît Canet]

The Saturday 26 Jul 2014 à 21:22:08 (+0200), Max Reitz wrote :
> 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>
> ---
>  block/qcow2-cluster.c | 36 ++++++++++++++++++++++++++++++++----
>  block/qcow2.c         |  9 ++++-----
>  block/qcow2.h         |  3 ++-
>  3 files changed, 38 insertions(+), 10 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 4208dc0..f8bec6f 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1548,10 +1548,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 ued 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);
> @@ -1571,6 +1578,7 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
>  
>          if (!l2_offset) {
>              /* unallocated */
> +            (*visited_l1_entries)++;
>              continue;
>          }
>  
> @@ -1703,6 +1711,13 @@ 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 << (s->l2_bits + s->cluster_bits),
> +                      l1_entries << (s->l2_bits + s->cluster_bits));

Shifting is a multiplication so it keep proportionality intact.
So do we really need these shifts ?

> +        }
>      }
>  
>      ret = 0;
> @@ -1729,21 +1744,32 @@ 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_malloc0((nb_clusters + 7) / 8);
>  
>      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;
>      }
> @@ -1777,7 +1803,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 757f890..6e8c8ab 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2147,7 +2147,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;
> @@ -2196,7 +2197,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;
>      }
> @@ -2224,8 +2225,6 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
>      int ret;
>      QemuOptDesc *desc = opts->list->desc;
>  
> -    (void)status_cb;
> -
>      while (desc && desc->name) {
>          if (!qemu_opt_find(opts, desc->name)) {
>              /* only change explicitly defined options */
> @@ -2291,7 +2290,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 b49424b..1c4f9bf 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -522,7 +522,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);
> -- 
> 2.0.3
> 
> 

Reviewed-by: Benoit Canet <benoit@irqsave.net>

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

[Benoît Canet]

The Thursday 31 Jul 2014 à 09:51:05 (+0200), Benoît Canet wrote :
> The Saturday 26 Jul 2014 à 21:22:05 (+0200), Max Reitz wrote :
> 
> > 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 couroutine 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>
> > ---
> >  block.c                   | 5 +++--
> >  block/qcow2.c             | 5 ++++-
> >  include/block/block.h     | 5 ++++-
> >  include/block/block_int.h | 3 ++-
> >  qemu-img.c                | 2 +-
> >  5 files changed, 14 insertions(+), 6 deletions(-)
> > 
> > diff --git a/block.c b/block.c
> > index 3e252a2..4c8d4d8 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -5708,12 +5708,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 b0faa69..757f890 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -2210,7 +2210,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;
> > @@ -2223,6 +2224,8 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts)
> >      int ret;
> >      QemuOptDesc *desc = opts->list->desc;
> >  
> > +    (void)status_cb;
> 
> This look like a temporary hack to please the compiler.
> Am I right ? Should be comment this ?

For the further patch I now understand this I am just suprised that you need to silence a unused function parameter.

Reviewed-by: Benoit Canet <benoit@irqsave.net>

> 
> > +
> >      while (desc && desc->name) {
> >          if (!qemu_opt_find(opts, desc->name)) {
> >              /* only change explicitly defined options */
> > diff --git a/include/block/block.h b/include/block/block.h
> > index 32d3676..f2b1799 100644
> > --- a/include/block/block.h
> > +++ b/include/block/block.h
> > @@ -309,7 +309,10 @@ typedef enum {
> >  
> >  int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix);
> >  
> > -int bdrv_amend_options(BlockDriverState *bs_new, QemuOpts *opts);
> > +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 f6c3bef..5c5798d 100644
> > --- a/include/block/block_int.h
> > +++ b/include/block/block_int.h
> > @@ -228,7 +228,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 ef74ae4..90d6b79 100644
> > --- a/qemu-img.c
> > +++ b/qemu-img.c
> > @@ -2827,7 +2827,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;
> > -- 
> > 2.0.3
> > 
> > 
> 

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

[Benoît Canet]

The Saturday 26 Jul 2014 à 21:22:09 (+0200), Max Reitz wrote :
> 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.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2-refcount.c | 23 ++++++++++++-----------
>  block/qcow2.h          |  2 ++
>  2 files changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index cc6cf74..0c9887b 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -87,7 +87,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;
> @@ -625,7 +625,7 @@ int qcow2_update_cluster_refcount(BlockDriverState *bs,
>          return ret;
>      }
>  
> -    return get_refcount(bs, cluster_index);
> +    return qcow2_get_refcount(bs, cluster_index);
>  }
>  
>  
> @@ -646,7 +646,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;
> @@ -710,7 +710,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;
> @@ -927,7 +927,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) {
> @@ -967,7 +967,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;
> @@ -1243,8 +1243,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,
> @@ -1265,7 +1265,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;
> @@ -1307,7 +1307,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;
> @@ -1597,7 +1598,7 @@ int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
>  
>      /* compare ref counts */
>      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 1c4f9bf..c0e1b7b 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -472,6 +472,8 @@ int qcow2_update_header(BlockDriverState *bs);
>  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);
>  
> -- 
> 2.0.3
> 
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

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

[Benoît Canet]

The Saturday 26 Jul 2014 à 21:22:10 (+0200), Max Reitz wrote :

> 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>
> ---
>  block/qcow2-cluster.c | 90 ++++++++++++++++-----------------------------------
>  1 file changed, 28 insertions(+), 62 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index f8bec6f..e6bff40 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1543,20 +1543,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 ued 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)
>  {
> @@ -1575,6 +1567,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 */
> @@ -1595,33 +1588,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;
>              }
>  
> @@ -1639,6 +1618,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);

This look like a wrong usage of qcow2_update_cluster_refcount:

/*                                                                              
 * Increases or decreases the refcount of a given cluster by one.               
 * addend must be 1 or -1.                                                      
Here ^
 *                                                                              
 * 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.                      
 */                                                                             
int qcow2_update_cluster_refcount(BlockDriverState *bs,                         
                                  int64_t cluster_index,                        
                                  int addend,                                   
                                  enum qcow2_discard_type type)  

Also this call is in a loop it would do l2_refcount - 1 * n increments on the refcount.

> +                    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);
> @@ -1660,29 +1652,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) {
> @@ -1749,9 +1724,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;
>  
> @@ -1762,12 +1735,7 @@ int qcow2_expand_zero_clusters(BlockDriverState *bs,
>          }
>      }
>  
> -    nb_clusters = size_to_clusters(s, bs->file->total_sectors *
> -                                   BDRV_SECTOR_SIZE);
> -    expanded_clusters = g_malloc0((nb_clusters + 7) / 8);
> -
>      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) {
> @@ -1803,7 +1771,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) {
> @@ -1814,7 +1781,6 @@ int qcow2_expand_zero_clusters(BlockDriverState *bs,
>      ret = 0;
>  
>  fail:
> -    g_free(expanded_clusters);
>      g_free(l1_table);
>      return ret;
>  }
> -- 
> 2.0.3
> 
> 

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

[Benoît Canet]

The Saturday 26 Jul 2014 à 21:22:11 (+0200), Max Reitz wrote :
> Add some tests for progress output to 061.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/061     | 27 +++++++++++++++++++++++++++
>  tests/qemu-iotests/061.out | 32 ++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/group   |  2 +-
>  3 files changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/061 b/tests/qemu-iotests/061
> index ab98def..fbb5897 100755
> --- a/tests/qemu-iotests/061
> +++ b/tests/qemu-iotests/061
> @@ -209,6 +209,33 @@ $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"


> +# COW L2 table 0
It's not clear what you are trying to achieve here since
it does not appear on the tests output.
Maybe that what you are testing is that it will output nothing
but you should notice it.

> +$QEMU_IO -c "write -z 64k 64k" "$TEST_IMG" | _filter_qemu_io
> +$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 e372470..352cca3 100644
> --- a/tests/qemu-iotests/061.out
> +++ b/tests/qemu-iotests/061.out
> @@ -384,4 +384,36 @@ 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%)
>     (12.50/100%)
>     (37.50/100%)
>     (62.50/100%)
>     (87.50/100%)
>     (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)
> +wrote 65536/65536 bytes at offset 65536
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +    (0.00/100%)
>     (6.25/100%)
>     (18.75/100%)
>     (31.25/100%)
>     (43.75/100%)
>     (56.25/100%)
>     (68.75/100%)
>     (81.25/100%)
>     (93.75/100%)
>     (100.00/100%)
> +No errors were found on the image.
>  *** done
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index 6e67f61..b27e2f9 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
> -- 
> 2.0.3
> 
> 

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

[Max Reitz]

On 31.07.2014 09:51, Benoît Canet wrote:
> The Saturday 26 Jul 2014 à 21:22:05 (+0200), Max Reitz wrote :
>
>> 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 couroutine 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>
>> ---
>>   block.c                   | 5 +++--
>>   block/qcow2.c             | 5 ++++-
>>   include/block/block.h     | 5 ++++-
>>   include/block/block_int.h | 3 ++-
>>   qemu-img.c                | 2 +-
>>   5 files changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 3e252a2..4c8d4d8 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -5708,12 +5708,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 b0faa69..757f890 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -2210,7 +2210,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;
>> @@ -2223,6 +2224,8 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts)
>>       int ret;
>>       QemuOptDesc *desc = opts->list->desc;
>>   
>> +    (void)status_cb;
> This look like a temporary hack to please the compiler.
> Am I right ? Should be comment this ?

It's removed in one of the next patches anyway; but I'll add a comment 
in v2.

Max

>> +
>>       while (desc && desc->name) {
>>           if (!qemu_opt_find(opts, desc->name)) {
>>               /* only change explicitly defined options */
>> diff --git a/include/block/block.h b/include/block/block.h
>> index 32d3676..f2b1799 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -309,7 +309,10 @@ typedef enum {
>>   
>>   int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix);
>>   
>> -int bdrv_amend_options(BlockDriverState *bs_new, QemuOpts *opts);
>> +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 f6c3bef..5c5798d 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -228,7 +228,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 ef74ae4..90d6b79 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -2827,7 +2827,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;
>> -- 
>> 2.0.3
>>
>>

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

[Max Reitz]

On 31.07.2014 09:56, Benoît Canet wrote:
> The Saturday 26 Jul 2014 à 21:22:06 (+0200), Max Reitz wrote :
>> Now that bdrv_amend_options() supports a status callback, use it to
>> display a progress report.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   qemu-img.c | 26 +++++++++++++++++++++++---
>>   1 file changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 90d6b79..a06f425 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -2732,6 +2732,13 @@ out:
>>       return 0;
>>   }
>>   
>> +static void amend_status_cb(BlockDriverState *bs,
>> +                            int64_t offset, int64_t total_work_size)
>> +{
>> +    (void)bs;
> This cast also look like a compiler pleasing thing.
> Is it really required ?

You're right, I thought -Wunused-parameter was enabled. Actually, this 
cast is unneeded; I'll drop it in v2.

Max

>> +    qemu_progress_print(100.f * offset / total_work_size, 0);
>> +}
>> +
>>   static int img_amend(int argc, char **argv)
>>   {
>>       int c, ret = 0;
>> @@ -2740,12 +2747,12 @@ 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;
>>       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;
>>           }
>> @@ -2775,6 +2782,9 @@ static int img_amend(int argc, char **argv)
>>               case 't':
>>                   cache = optarg;
>>                   break;
>> +            case 'p':
>> +                progress = true;
>> +                break;
>>               case 'q':
>>                   quiet = true;
>>                   break;
>> @@ -2785,6 +2795,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
>> @@ -2827,13 +2842,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();
>> +
>>       if (bs) {
>>           bdrv_unref(bs);
>>       }
>> -- 
>> 2.0.3
>>
>>

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

[Max Reitz]

On 31.07.2014 10:06, Benoît Canet wrote:
> The Saturday 26 Jul 2014 à 21:22:08 (+0200), Max Reitz wrote :
>> 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>
>> ---
>>   block/qcow2-cluster.c | 36 ++++++++++++++++++++++++++++++++----
>>   block/qcow2.c         |  9 ++++-----
>>   block/qcow2.h         |  3 ++-
>>   3 files changed, 38 insertions(+), 10 deletions(-)
>>
>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
>> index 4208dc0..f8bec6f 100644
>> --- a/block/qcow2-cluster.c
>> +++ b/block/qcow2-cluster.c
>> @@ -1548,10 +1548,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 ued 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);
>> @@ -1571,6 +1578,7 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
>>   
>>           if (!l2_offset) {
>>               /* unallocated */
>> +            (*visited_l1_entries)++;
>>               continue;
>>           }
>>   
>> @@ -1703,6 +1711,13 @@ 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 << (s->l2_bits + s->cluster_bits),
>> +                      l1_entries << (s->l2_bits + s->cluster_bits));
> Shifting is a multiplication so it keep proportionality intact.
> So do we really need these shifts ?

As of patch 1, the variables are defined as "offset" and "working_size" 
which I meant to be byte ranges. We could indeed leave the unit for 
BlockDriverAmendStatusCB's parameters undefined (and leave it up to the 
block driver, only specifying that offset / working_size has to be the 
progress ratio), but then we could just as well just give a floating 
point percentage to that function.

Bytes as a unit seemed safe to me, however, since all of qemu's code 
assumes byte ranges to always fit in int64_t; and the reason I preferred 
them over a percentage is that block jobs use bytes as well.

A real reason not to use bytes would be that some driver is unable to 
give a "byte" representation of its workload during the amend progress; 
however, this seems unlikely to me (every large workload which should be 
part of the progress report should be somehow representable as bytes).

Max

>> +        }
>>       }
>>   
>>       ret = 0;
>> @@ -1729,21 +1744,32 @@ 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_malloc0((nb_clusters + 7) / 8);
>>   
>>       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;
>>       }
>> @@ -1777,7 +1803,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 757f890..6e8c8ab 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -2147,7 +2147,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;
>> @@ -2196,7 +2197,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;
>>       }
>> @@ -2224,8 +2225,6 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
>>       int ret;
>>       QemuOptDesc *desc = opts->list->desc;
>>   
>> -    (void)status_cb;
>> -
>>       while (desc && desc->name) {
>>           if (!qemu_opt_find(opts, desc->name)) {
>>               /* only change explicitly defined options */
>> @@ -2291,7 +2290,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 b49424b..1c4f9bf 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -522,7 +522,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);
>> -- 
>> 2.0.3
>>
>>
> Reviewed-by: Benoit Canet <benoit@irqsave.net>

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

[Max Reitz]

On 31.07.2014 10:30, Benoît Canet wrote:
> The Saturday 26 Jul 2014 à 21:22:11 (+0200), Max Reitz wrote :
>> Add some tests for progress output to 061.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   tests/qemu-iotests/061     | 27 +++++++++++++++++++++++++++
>>   tests/qemu-iotests/061.out | 32 ++++++++++++++++++++++++++++++++
>>   tests/qemu-iotests/group   |  2 +-
>>   3 files changed, 60 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/qemu-iotests/061 b/tests/qemu-iotests/061
>> index ab98def..fbb5897 100755
>> --- a/tests/qemu-iotests/061
>> +++ b/tests/qemu-iotests/061
>> @@ -209,6 +209,33 @@ $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"
>
>> +# COW L2 table 0
> It's not clear what you are trying to achieve here since
> it does not appear on the tests output.
> Maybe that what you are testing is that it will output nothing
> but you should notice it.

Well, this made more use with the other version of the series with the 
more correct progress output.

If you just take a snapshot, all L2 tables will be shared between the 
active L1 table and the snapshot's L1 table. Therefore, all zero 
clusters will be expanded after the active L1 table has been iterated 
over, because the snapshot's L1 table points to exactly the same L2 
tables. When the snapshot's L1 table and its L2 tables are iterated 
over, the zero cluster expansion function will find no zero clusters any 
more and so you'd get exactly the same output as without a snapshot (0 
%, 25 %, 50 %, 75 %, 100 %).

Now, if you modify one of the L2 tables as in this test which creates a 
second zero cluster in the first L2 table, that L2 table can no longer 
be shared between the active and the snapshot L1 table. Instead, it 
needs to be COW'ed and now there are five L2 tables in the image: Each 
L1 table has an own version of the very first L2 table, and the other 
three are shared. In the original version, this led to the following 
progress output: 0 %, 22 %, 44 %, 67 %, 89 %, 100 % (the first step is 
for the first active L2 table which contains *two* zero clusters; the 
following three steps are for the three shared L2 tables which contain 
one zero cluster each, but since they are shared between two L1 tables, 
those are accounted for twice; and the last step is the L2 table owned 
by the snapshot which only has a single zero cluster as well and is 
referenced only once).

In this version, the L2 zero cluster entries are not counted anymore; 
all that counts are L1 entries. And since there are as many entries in 
the first as in the second L1 table, we get the output 0 %, 13 %, 25 %, 
38 %, 50 %, etc..

I guest I can drop this COW enforcement in this version of the series, 
though, since it doesn't (and isn't supposed to) change anything.

Max

>> +$QEMU_IO -c "write -z 64k 64k" "$TEST_IMG" | _filter_qemu_io
>> +$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 e372470..352cca3 100644
>> --- a/tests/qemu-iotests/061.out
>> +++ b/tests/qemu-iotests/061.out
>> @@ -384,4 +384,36 @@ 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%)
>>      (12.50/100%)
>>      (37.50/100%)
>>      (62.50/100%)
>>      (87.50/100%)
>>      (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)
>> +wrote 65536/65536 bytes at offset 65536
>> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>> +    (0.00/100%)
>>      (6.25/100%)
>>      (18.75/100%)
>>      (31.25/100%)
>>      (43.75/100%)
>>      (56.25/100%)
>>      (68.75/100%)
>>      (81.25/100%)
>>      (93.75/100%)
>>      (100.00/100%)
>> +No errors were found on the image.
>>   *** done
>> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
>> index 6e67f61..b27e2f9 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
>> -- 
>> 2.0.3
>>
>>

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

[Eric Blake]

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

On 08/01/2014 02:18 PM, Max Reitz wrote:

>>> +        if (status_cb) {
>>> +            status_cb(bs, *visited_l1_entries << (s->l2_bits +
>>> s->cluster_bits),
>>> +                      l1_entries << (s->l2_bits + s->cluster_bits));
>> Shifting is a multiplication so it keep proportionality intact.
>> So do we really need these shifts ?
> 
> As of patch 1, the variables are defined as "offset" and "working_size"
> which I meant to be byte ranges. We could indeed leave the unit for
> BlockDriverAmendStatusCB's parameters undefined (and leave it up to the
> block driver, only specifying that offset / working_size has to be the
> progress ratio), but then we could just as well just give a floating
> point percentage to that function.

As it is, block jobs are already documented as merely exposing
completion percentage, and NOT tied to the size of the underlying block
device.  Your own pending patch is proof of this:

https://lists.gnu.org/archive/html/qemu-devel/2014-07/msg00960.html

When doing drive-mirror, we WANT to have the total size grow according
to how many dirty blocks are encountered through each pass, and the
current offset grow in rough proportion to how fast we are converging to
a mirrored state (or even having the percentage go backwards, when we
are diverging from too much I/O pressure, as a sign that some throttling
is needed).  Artificially trying to constrain that progress reporting to
the size in bytes of the block device does not help matters.

> 
> Bytes as a unit seemed safe to me, however, since all of qemu's code
> assumes byte ranges to always fit in int64_t; and the reason I preferred
> them over a percentage is that block jobs use bytes as well.
> 
> A real reason not to use bytes would be that some driver is unable to
> give a "byte" representation of its workload during the amend progress;
> however, this seems unlikely to me (every large workload which should be
> part of the progress report should be somehow representable as bytes).

I don't think we can promise anything more than two relative numbers,
with no bearing on what they are measuring under the hood, so scaling
both numbers just to produce progress output buys nothing.  There may
eventually be a device where we can't report progress in any more
granularity than 0 (still working) or 1 (done).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

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

[Max Reitz]

On 01.08.2014 22:38, Eric Blake wrote:
> On 08/01/2014 02:18 PM, Max Reitz wrote:
>
>>>> +        if (status_cb) {
>>>> +            status_cb(bs, *visited_l1_entries << (s->l2_bits +
>>>> s->cluster_bits),
>>>> +                      l1_entries << (s->l2_bits + s->cluster_bits));
>>> Shifting is a multiplication so it keep proportionality intact.
>>> So do we really need these shifts ?
>> As of patch 1, the variables are defined as "offset" and "working_size"
>> which I meant to be byte ranges. We could indeed leave the unit for
>> BlockDriverAmendStatusCB's parameters undefined (and leave it up to the
>> block driver, only specifying that offset / working_size has to be the
>> progress ratio), but then we could just as well just give a floating
>> point percentage to that function.
> As it is, block jobs are already documented as merely exposing
> completion percentage, and NOT tied to the size of the underlying block
> device.  Your own pending patch is proof of this:
>
> https://lists.gnu.org/archive/html/qemu-devel/2014-07/msg00960.html
>
> When doing drive-mirror, we WANT to have the total size grow according
> to how many dirty blocks are encountered through each pass, and the
> current offset grow in rough proportion to how fast we are converging to
> a mirrored state (or even having the percentage go backwards, when we
> are diverging from too much I/O pressure, as a sign that some throttling
> is needed).  Artificially trying to constrain that progress reporting to
> the size in bytes of the block device does not help matters.

Well, this would just as well work with a single floating point percentage.

>> Bytes as a unit seemed safe to me, however, since all of qemu's code
>> assumes byte ranges to always fit in int64_t; and the reason I preferred
>> them over a percentage is that block jobs use bytes as well.
>>
>> A real reason not to use bytes would be that some driver is unable to
>> give a "byte" representation of its workload during the amend progress;
>> however, this seems unlikely to me (every large workload which should be
>> part of the progress report should be somehow representable as bytes).
> I don't think we can promise anything more than two relative numbers,
> with no bearing on what they are measuring under the hood, so scaling
> both numbers just to produce progress output buys nothing.  There may
> eventually be a device where we can't report progress in any more
> granularity than 0 (still working) or 1 (done).

In that case it should never call the callback.

Note how qcow2 can do a whole number of things including resizing the 
image during an amend operation; but only the zero cluster expansion is 
costly enough to justify a progress output. Consequently, it's only that 
function which invokes the callback. If the image is not being 
downgraded from compat=1.1 to compat=0.10, the callback is never 
invoked; there even is a small comment about this in qemu-img.c (/* In 
case the driver does not call amend_status_cb() */).

I can however see that a driver invokes a couple of time-consuming 
operations and there is no progress report for each of those; so uses 
the number of operations as the workload size and the parameter 
currently named "offset" as the index of the operation executed next. 
Frankly, I'd blame the driver then, since the operations should give a 
real progress report (which I still think *has* to be convertable to 
bytes somehow).

Anyway, you're probably both right and we should just drop the unit 
enforcement and allow the driver to use any unit it desires; but in this 
case I still think we could just give a floating point percentage 
instead of a numerator and a denominator.

Max

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

[Max Reitz]

On 31.07.2014 10:24, Benoît Canet wrote:
> The Saturday 26 Jul 2014 à 21:22:10 (+0200), Max Reitz wrote :
>
>> 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>
>> ---
>>   block/qcow2-cluster.c | 90 ++++++++++++++++-----------------------------------
>>   1 file changed, 28 insertions(+), 62 deletions(-)
>>
>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
>> index f8bec6f..e6bff40 100644
>> --- a/block/qcow2-cluster.c
>> +++ b/block/qcow2-cluster.c
>> @@ -1543,20 +1543,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 ued 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)
>>   {
>> @@ -1575,6 +1567,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 */
>> @@ -1595,33 +1588,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;
>>               }
>>   
>> @@ -1639,6 +1618,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);
> This look like a wrong usage of qcow2_update_cluster_refcount:
>
> /*
>   * Increases or decreases the refcount of a given cluster by one.
>   * addend must be 1 or -1.
> Here ^

As far as I can see, that comment no longer applies (anything in 
update_refcount() very well allows arbitrary values). I'll remove it in v2.

>   *
>   * 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.
>   */
> int qcow2_update_cluster_refcount(BlockDriverState *bs,
>                                    int64_t cluster_index,
>                                    int addend,
>                                    enum qcow2_discard_type type)
>
> Also this call is in a loop it would do l2_refcount - 1 * n increments on the refcount.

Hm? The cluster at "offset" is allocated directly before the call to 
qcow2_update_cluster_refcount(). There is no loop which the latter call 
is in, but the allocation is not.

Max

>> +                    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);
>> @@ -1660,29 +1652,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) {
>> @@ -1749,9 +1724,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;
>>   
>> @@ -1762,12 +1735,7 @@ int qcow2_expand_zero_clusters(BlockDriverState *bs,
>>           }
>>       }
>>   
>> -    nb_clusters = size_to_clusters(s, bs->file->total_sectors *
>> -                                   BDRV_SECTOR_SIZE);
>> -    expanded_clusters = g_malloc0((nb_clusters + 7) / 8);
>> -
>>       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) {
>> @@ -1803,7 +1771,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) {
>> @@ -1814,7 +1781,6 @@ int qcow2_expand_zero_clusters(BlockDriverState *bs,
>>       ret = 0;
>>   
>>   fail:
>> -    g_free(expanded_clusters);
>>       g_free(l1_table);
>>       return ret;
>>   }
>> -- 
>> 2.0.3
>>
>>