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

[Qemu-devel] [PATCH v2 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 v2 is based on the "alt" (alternative) variant of v1 of this series.

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


Changes from v1:
 - Patch 1:
   - Drop unnecessary -Wunused-parameter silencing statement (that
     warning is not enabled) [Benoît]
   - The BlockDriverAmendStatusCB function takes parameters of an
     arbitrary unit; document that [Benoît and Eric]
 - Patch 2:
   - Add documentation for the new parameter [Eric]
   - Drop unnecessary -Wunused-parameter silencing statement [Benoît]
 - Patch 3: Fixed commit message [Eric]
 - Patch 4:
   - Fixed comment [Eric]
   - As the BlockDriverAmendStatusCB function does no longer have to be
     called with byte values, cancel common factor out [Benoît]
   - Call the status CB after having passed an empty L1 entry as well
 - Patch 5: qcow2_get_refcount() may be called with any addend; remove
   the artificial restriction from the documenting comment
 - Patch 7:
   - Drop unnecessary L2 COW enforcing write [Benoît]
   - Since the status CB is now called after empty L1 entries, too, the
     progress output is more fine-grained (every L1 entry results in a
     progress increase)


git-upstream-diff output against v1 alt:

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:[0005] [FC] 'block: Add status callback to bdrv_amend_options()'
002/7:[0007] [FC] 'qemu-img: Add progress output for amend'
003/7:[down] 'qemu-img: Fix insignificant memleak'
       ^^^^ false negative due to the fixed commit message
            (actually [----] [--])
004/7:[0011] [FC] 'block/qcow2: Implement status CB for amend'
005/7:[0003] [FC] 'block/qcow2: Make get_refcount() global'
006/7:[----] [-C] 'block/qcow2: Simplify shared L2 handling in amend'
007/7:[0008] [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 insignificant memleak
  block/qcow2: Implement status CB for amend
  block/qcow2: Make get_refcount() global
  block/qcow2: Simplify shared L2 handling in amend
  iotests: Expand test 061

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

-- 
2.0.3

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

diff --git a/block.c b/block.c
index 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..a3a7efc 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;
diff --git a/include/block/block.h b/include/block/block.h
index 32d3676..3ce3076 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -309,7 +309,13 @@ typedef enum {
 
 int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix);
 
-int bdrv_amend_options(BlockDriverState *bs_new, QemuOpts *opts);
+/* The units of offset and total_work_size may be chosen arbitrarily by the
+ * block driver; total_work_size may change during the course of the amendment
+ * operation */
+typedef void BlockDriverAmendStatusCB(BlockDriverState *bs, int64_t offset,
+                                      int64_t total_work_size);
+int bdrv_amend_options(BlockDriverState *bs_new, QemuOpts *opts,
+                       BlockDriverAmendStatusCB *status_cb);
 
 /* external snapshots */
 bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 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 v2 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-cmds.hx |  4 ++--
 qemu-img.c       | 25 ++++++++++++++++++++++---
 qemu-img.texi    |  2 +-
 3 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 55aec6b..d83f3d0 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -70,8 +70,8 @@ STEXI
 ETEXI
 
 DEF("amend", img_amend,
-    "amend [-q] [-f fmt] [-t cache] -o options filename")
+    "amend [-p] [-q] [-f fmt] [-t cache] -o options filename")
 STEXI
-@item amend [-q] [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
+@item amend [-p] [-q] [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
 @end table
 ETEXI
diff --git a/qemu-img.c b/qemu-img.c
index 90d6b79..4b6406b 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2732,6 +2732,12 @@ out:
     return 0;
 }
 
+static void amend_status_cb(BlockDriverState *bs,
+                            int64_t offset, int64_t total_work_size)
+{
+    qemu_progress_print(100.f * offset / total_work_size, 0);
+}
+
 static int img_amend(int argc, char **argv)
 {
     int c, ret = 0;
@@ -2740,12 +2746,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 +2781,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 +2794,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 +2841,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);
     }
diff --git a/qemu-img.texi b/qemu-img.texi
index cb68948..2c66603 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -397,7 +397,7 @@ After using this command to grow a disk image, you must use file system and
 partitioning tools inside the VM to actually begin using the new space on the
 device.
 
-@item amend [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
+@item amend [-p] [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
 
 Amends the image format specific @var{options} for the image file
 @var{filename}. Not all file formats support this operation.
-- 
2.0.3

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

[Max Reitz]

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

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

diff --git a/qemu-img.c b/qemu-img.c
index 4b6406b..0bd8fa5 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2808,7 +2808,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 v2 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 | 37 +++++++++++++++++++++++++++++++++----
 block/qcow2.c         |  7 ++++---
 block/qcow2.h         |  3 ++-
 3 files changed, 39 insertions(+), 8 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 4208dc0..2607715 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 used to keep track of progress for
+ * status_cb(). l1_entries contains the total number of L1 entries and
+ * *visited_l1_entries counts all visited L1 entries.
  */
 static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
                                       int l1_size, uint8_t **expanded_clusters,
-                                      uint64_t *nb_clusters)
+                                      uint64_t *nb_clusters,
+                                      int64_t *visited_l1_entries,
+                                      int64_t l1_entries,
+                                      BlockDriverAmendStatusCB *status_cb)
 {
     BDRVQcowState *s = bs->opaque;
     bool is_active_l1 = (l1_table == s->l1_table);
@@ -1571,6 +1578,10 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
 
         if (!l2_offset) {
             /* unallocated */
+            (*visited_l1_entries)++;
+            if (status_cb) {
+                status_cb(bs, *visited_l1_entries, l1_entries);
+            }
             continue;
         }
 
@@ -1703,6 +1714,11 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
                 }
             }
         }
+
+        (*visited_l1_entries)++;
+        if (status_cb) {
+            status_cb(bs, *visited_l1_entries, l1_entries);
+        }
     }
 
     ret = 0;
@@ -1729,21 +1745,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 +1804,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 a3a7efc..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;
     }
@@ -2289,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 v2 5/7] block/qcow2: Make get_refcount() global

[Max Reitz]

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

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

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

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index cc6cf74..1c2ab8c 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;
@@ -605,8 +605,7 @@ fail:
 }
 
 /*
- * Increases or decreases the refcount of a given cluster by one.
- * addend must be 1 or -1.
+ * Increases or decreases the refcount of a given cluster.
  *
  * If the return value is non-negative, it is the new refcount of the cluster.
  * If it is negative, it is -errno and indicates an error.
@@ -625,7 +624,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 +645,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 +709,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 +926,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 +966,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 +1242,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 +1264,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 +1306,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 +1597,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 v2 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 2607715..7e65c13 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 used to keep track of progress for
  * status_cb(). l1_entries contains the total number of L1 entries and
  * *visited_l1_entries counts all visited L1 entries.
  */
 static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
-                                      int l1_size, uint8_t **expanded_clusters,
-                                      uint64_t *nb_clusters,
-                                      int64_t *visited_l1_entries,
+                                      int l1_size, int64_t *visited_l1_entries,
                                       int64_t l1_entries,
                                       BlockDriverAmendStatusCB *status_cb)
 {
@@ -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 */
@@ -1598,33 +1591,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;
             }
 
@@ -1642,6 +1621,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);
@@ -1663,29 +1655,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) {
@@ -1750,9 +1725,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;
 
@@ -1763,12 +1736,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) {
@@ -1804,7 +1772,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) {
@@ -1815,7 +1782,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 v2 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     | 25 +++++++++++++++++++++++++
 tests/qemu-iotests/061.out | 30 ++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |  2 +-
 3 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/061 b/tests/qemu-iotests/061
index ab98def..8d37f8a 100755
--- a/tests/qemu-iotests/061
+++ b/tests/qemu-iotests/061
@@ -209,6 +209,31 @@ $QEMU_IMG amend -o "compat=0.10" "$TEST_IMG"
 _check_test_img
 $QEMU_IO -c "read -P 0 0 64M" "$TEST_IMG" | _filter_qemu_io
 
+echo
+echo "=== Testing progress report without snapshot ==="
+echo
+IMGOPTS="compat=1.1" TEST_IMG="$TEST_IMG.base" _make_test_img 4G
+IMGOPTS="compat=1.1" _make_test_img -b "$TEST_IMG.base" 4G
+$QEMU_IO -c "write -z 0  64k" \
+         -c "write -z 1G 64k" \
+         -c "write -z 2G 64k" \
+         -c "write -z 3G 64k" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IMG amend -p -o "compat=0.10" "$TEST_IMG"
+_check_test_img
+
+echo
+echo "=== Testing progress report with snapshot ==="
+echo
+IMGOPTS="compat=1.1" TEST_IMG="$TEST_IMG.base" _make_test_img 4G
+IMGOPTS="compat=1.1" _make_test_img -b "$TEST_IMG.base" 4G
+$QEMU_IO -c "write -z 0  64k" \
+         -c "write -z 1G 64k" \
+         -c "write -z 2G 64k" \
+         -c "write -z 3G 64k" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IMG snapshot -c foo "$TEST_IMG"
+$QEMU_IMG amend -p -o "compat=0.10" "$TEST_IMG"
+_check_test_img
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/061.out b/tests/qemu-iotests/061.out
index e372470..789f651 100644
--- a/tests/qemu-iotests/061.out
+++ b/tests/qemu-iotests/061.out
@@ -384,4 +384,34 @@ wrote 67108864/67108864 bytes at offset 0
 No errors were found on the image.
 read 67108864/67108864 bytes at offset 0
 64 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Testing progress report without snapshot ===
+
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=4294967296 
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=4294967296 backing_file='TEST_DIR/t.IMGFMT.base' 
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 65536/65536 bytes at offset 1073741824
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 65536/65536 bytes at offset 2147483648
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 65536/65536 bytes at offset 3221225472
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+    (0.00/100%)
    (12.50/100%)
    (25.00/100%)
    (37.50/100%)
    (50.00/100%)
    (62.50/100%)
    (75.00/100%)
    (87.50/100%)
    (100.00/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)
+    (0.00/100%)
    (6.25/100%)
    (12.50/100%)
    (18.75/100%)
    (25.00/100%)
    (31.25/100%)
    (37.50/100%)
    (43.75/100%)
    (50.00/100%)
    (56.25/100%)
    (62.50/100%)
    (68.75/100%)
    (75.00/100%)
    (81.25/100%)
    (87.50/100%)
    (93.75/100%)
    (100.00/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 v2 1/7] block: Add status callback to bdrv_amend_options()

[Benoît Canet]

The Saturday 02 Aug 2014 à 01:49:15 (+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             | 3 ++-
>  include/block/block.h     | 8 +++++++-
>  include/block/block_int.h | 3 ++-
>  qemu-img.c                | 2 +-
>  5 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 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..a3a7efc 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;
> diff --git a/include/block/block.h b/include/block/block.h
> index 32d3676..3ce3076 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -309,7 +309,13 @@ typedef enum {
>  
>  int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix);
>  
> -int bdrv_amend_options(BlockDriverState *bs_new, QemuOpts *opts);
> +/* The units of offset and total_work_size may be chosen arbitrarily by the
> + * block driver; total_work_size may change during the course of the amendment
> + * operation */
> +typedef void BlockDriverAmendStatusCB(BlockDriverState *bs, int64_t offset,
> +                                      int64_t total_work_size);
> +int bdrv_amend_options(BlockDriverState *bs_new, QemuOpts *opts,
> +                       BlockDriverAmendStatusCB *status_cb);
>  
>  /* external snapshots */
>  bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs,
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 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
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

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

[Benoît Canet]

The Saturday 02 Aug 2014 à 01:49:16 (+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-cmds.hx |  4 ++--
>  qemu-img.c       | 25 ++++++++++++++++++++++---
>  qemu-img.texi    |  2 +-
>  3 files changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
> index 55aec6b..d83f3d0 100644
> --- a/qemu-img-cmds.hx
> +++ b/qemu-img-cmds.hx
> @@ -70,8 +70,8 @@ STEXI
>  ETEXI
>  
>  DEF("amend", img_amend,
> -    "amend [-q] [-f fmt] [-t cache] -o options filename")
> +    "amend [-p] [-q] [-f fmt] [-t cache] -o options filename")
>  STEXI
> -@item amend [-q] [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
> +@item amend [-p] [-q] [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
>  @end table
>  ETEXI
> diff --git a/qemu-img.c b/qemu-img.c
> index 90d6b79..4b6406b 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -2732,6 +2732,12 @@ out:
>      return 0;
>  }
>  
> +static void amend_status_cb(BlockDriverState *bs,
> +                            int64_t offset, int64_t total_work_size)
> +{
> +    qemu_progress_print(100.f * offset / total_work_size, 0);
> +}
> +
>  static int img_amend(int argc, char **argv)
>  {
>      int c, ret = 0;
> @@ -2740,12 +2746,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 +2781,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 +2794,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 +2841,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);
>      }
> diff --git a/qemu-img.texi b/qemu-img.texi
> index cb68948..2c66603 100644
> --- a/qemu-img.texi
> +++ b/qemu-img.texi
> @@ -397,7 +397,7 @@ After using this command to grow a disk image, you must use file system and
>  partitioning tools inside the VM to actually begin using the new space on the
>  device.
>  
> -@item amend [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
> +@item amend [-p] [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
>  
>  Amends the image format specific @var{options} for the image file
>  @var{filename}. Not all file formats support this operation.
> -- 
> 2.0.3
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

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

[Benoît Canet]

The Saturday 02 Aug 2014 à 01:49:18 (+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 | 37 +++++++++++++++++++++++++++++++++----
>  block/qcow2.c         |  7 ++++---
>  block/qcow2.h         |  3 ++-
>  3 files changed, 39 insertions(+), 8 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 4208dc0..2607715 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 used to keep track of progress for
> + * status_cb(). l1_entries contains the total number of L1 entries and
> + * *visited_l1_entries counts all visited L1 entries.
>   */
>  static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
>                                        int l1_size, uint8_t **expanded_clusters,
> -                                      uint64_t *nb_clusters)
> +                                      uint64_t *nb_clusters,
> +                                      int64_t *visited_l1_entries,
> +                                      int64_t l1_entries,
> +                                      BlockDriverAmendStatusCB *status_cb)
>  {
>      BDRVQcowState *s = bs->opaque;
>      bool is_active_l1 = (l1_table == s->l1_table);
> @@ -1571,6 +1578,10 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
>  
>          if (!l2_offset) {
>              /* unallocated */
> +            (*visited_l1_entries)++;
> +            if (status_cb) {
> +                status_cb(bs, *visited_l1_entries, l1_entries);
> +            }
>              continue;
>          }
>  
> @@ -1703,6 +1714,11 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
>                  }
>              }
>          }
> +
> +        (*visited_l1_entries)++;
> +        if (status_cb) {
> +            status_cb(bs, *visited_l1_entries, l1_entries);
> +        }
>      }
>  
>      ret = 0;
> @@ -1729,21 +1745,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 +1804,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 a3a7efc..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;
>      }
> @@ -2289,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 v2 5/7] block/qcow2: Make get_refcount() global

[Benoît Canet]

The Saturday 02 Aug 2014 à 01:49:19 (+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.
> 
> While touching this function, amend the comment describing the "addend"
> parameter: It is (no longer, if it ever was) necessary to have it set to
> -1 or 1; any value is fine.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2-refcount.c | 26 +++++++++++++-------------
>  block/qcow2.h          |  2 ++
>  2 files changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index cc6cf74..1c2ab8c 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;
> @@ -605,8 +605,7 @@ fail:
>  }
>  
>  /*
> - * Increases or decreases the refcount of a given cluster by one.
> - * addend must be 1 or -1.
> + * Increases or decreases the refcount of a given cluster.
>   *
>   * If the return value is non-negative, it is the new refcount of the cluster.
>   * If it is negative, it is -errno and indicates an error.
> @@ -625,7 +624,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 +645,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 +709,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 +926,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 +966,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 +1242,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 +1264,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 +1306,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 +1597,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 v2 6/7] block/qcow2: Simplify shared L2 handling in amend

[Benoît Canet]

The Saturday 02 Aug 2014 à 01:49:20 (+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 2607715..7e65c13 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 used to keep track of progress for
>   * status_cb(). l1_entries contains the total number of L1 entries and
>   * *visited_l1_entries counts all visited L1 entries.
>   */
>  static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
> -                                      int l1_size, uint8_t **expanded_clusters,
> -                                      uint64_t *nb_clusters,
> -                                      int64_t *visited_l1_entries,
> +                                      int l1_size, int64_t *visited_l1_entries,
>                                        int64_t l1_entries,
>                                        BlockDriverAmendStatusCB *status_cb)
>  {
> @@ -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 */
> @@ -1598,33 +1591,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;
>              }
>  
> @@ -1642,6 +1621,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);
> @@ -1663,29 +1655,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) {
> @@ -1750,9 +1725,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;
>  
> @@ -1763,12 +1736,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) {
> @@ -1804,7 +1772,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) {
> @@ -1815,7 +1782,6 @@ int qcow2_expand_zero_clusters(BlockDriverState *bs,
>      ret = 0;
>  
>  fail:
> -    g_free(expanded_clusters);
>      g_free(l1_table);
>      return ret;
>  }
> -- 
> 2.0.3
> 

I don't understand this one very well so I will not Rev-By it.

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

[Eric Blake]

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

On 08/01/2014 05:49 PM, 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

s/couroutine/coroutine/

> 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             | 3 ++-
>  include/block/block.h     | 8 +++++++-
>  include/block/block_int.h | 3 ++-
>  qemu-img.c                | 2 +-
>  5 files changed, 15 insertions(+), 6 deletions(-)

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

-- 
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 v2 2/7] qemu-img: Add progress output for amend

[Eric Blake]

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

On 08/01/2014 05:49 PM, 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-cmds.hx |  4 ++--
>  qemu-img.c       | 25 ++++++++++++++++++++++---
>  qemu-img.texi    |  2 +-
>  3 files changed, 25 insertions(+), 6 deletions(-)

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

-- 
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 v2 4/7] block/qcow2: Implement status CB for amend

[Eric Blake]

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

On 08/01/2014 05:49 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 | 37 +++++++++++++++++++++++++++++++++----
>  block/qcow2.c         |  7 ++++---
>  block/qcow2.h         |  3 ++-
>  3 files changed, 39 insertions(+), 8 deletions(-)

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


-- 
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 v2 5/7] block/qcow2: Make get_refcount() global

[Eric Blake]

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

On 08/01/2014 05:49 PM, 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.
> 
> While touching this function, amend the comment describing the "addend"
> parameter: It is (no longer, if it ever was) necessary to have it set to
> -1 or 1; any value is fine.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2-refcount.c | 26 +++++++++++++-------------
>  block/qcow2.h          |  2 ++
>  2 files changed, 15 insertions(+), 13 deletions(-)

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

-- 
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 v2 6/7] block/qcow2: Simplify shared L2 handling in amend

[Eric Blake]

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

On 08/01/2014 05:49 PM, 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(-)
> 

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

-- 
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 v2 7/7] iotests: Expand test 061

[Eric Blake]

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

On 08/01/2014 05:49 PM, Max Reitz wrote:
> Add some tests for progress output to 061.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/061     | 25 +++++++++++++++++++++++++
>  tests/qemu-iotests/061.out | 30 ++++++++++++++++++++++++++++++
>  tests/qemu-iotests/group   |  2 +-
>  3 files changed, 56 insertions(+), 1 deletion(-)
> 

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

-- 
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 v2 1/7] block: Add status callback to bdrv_amend_options()

[Benoît Canet]

The Saturday 02 Aug 2014 à 01:49:15 (+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             | 3 ++-
>  include/block/block.h     | 8 +++++++-
>  include/block/block_int.h | 3 ++-
>  qemu-img.c                | 2 +-
>  5 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 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..a3a7efc 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;
> diff --git a/include/block/block.h b/include/block/block.h
> index 32d3676..3ce3076 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -309,7 +309,13 @@ typedef enum {
>  
>  int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix);
>  
> -int bdrv_amend_options(BlockDriverState *bs_new, QemuOpts *opts);
> +/* The units of offset and total_work_size may be chosen arbitrarily by the
> + * block driver; total_work_size may change during the course of the amendment
> + * operation */
> +typedef void BlockDriverAmendStatusCB(BlockDriverState *bs, int64_t offset,
> +                                      int64_t total_work_size);
> +int bdrv_amend_options(BlockDriverState *bs_new, QemuOpts *opts,
> +                       BlockDriverAmendStatusCB *status_cb);
>  
>  /* external snapshots */
>  bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs,
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 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
> 
Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>

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

[Benoît Canet]

The Saturday 02 Aug 2014 à 01:49:16 (+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-cmds.hx |  4 ++--
>  qemu-img.c       | 25 ++++++++++++++++++++++---
>  qemu-img.texi    |  2 +-
>  3 files changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
> index 55aec6b..d83f3d0 100644
> --- a/qemu-img-cmds.hx
> +++ b/qemu-img-cmds.hx
> @@ -70,8 +70,8 @@ STEXI
>  ETEXI
>  
>  DEF("amend", img_amend,
> -    "amend [-q] [-f fmt] [-t cache] -o options filename")
> +    "amend [-p] [-q] [-f fmt] [-t cache] -o options filename")
>  STEXI
> -@item amend [-q] [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
> +@item amend [-p] [-q] [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
>  @end table
>  ETEXI
> diff --git a/qemu-img.c b/qemu-img.c
> index 90d6b79..4b6406b 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -2732,6 +2732,12 @@ out:
>      return 0;
>  }
>  
> +static void amend_status_cb(BlockDriverState *bs,
> +                            int64_t offset, int64_t total_work_size)
> +{
> +    qemu_progress_print(100.f * offset / total_work_size, 0);
> +}
> +
>  static int img_amend(int argc, char **argv)
>  {
>      int c, ret = 0;
> @@ -2740,12 +2746,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 +2781,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 +2794,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 +2841,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);
>      }
> diff --git a/qemu-img.texi b/qemu-img.texi
> index cb68948..2c66603 100644
> --- a/qemu-img.texi
> +++ b/qemu-img.texi
> @@ -397,7 +397,7 @@ After using this command to grow a disk image, you must use file system and
>  partitioning tools inside the VM to actually begin using the new space on the
>  device.
>  
> -@item amend [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
> +@item amend [-p] [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
>  
>  Amends the image format specific @var{options} for the image file
>  @var{filename}. Not all file formats support this operation.
> -- 
> 2.0.3
> 

This patchs is fine and sweet.
However could we add an assert(amend_status_cb) in bdrv_amend_options somewhere in the series ?
So if a coder pass NULL as callback he will have a clue near the root cause.

Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>

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

[Benoît Canet]

The Saturday 02 Aug 2014 à 01:49:18 (+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 | 37 +++++++++++++++++++++++++++++++++----
>  block/qcow2.c         |  7 ++++---
>  block/qcow2.h         |  3 ++-
>  3 files changed, 39 insertions(+), 8 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 4208dc0..2607715 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 used to keep track of progress for
> + * status_cb(). l1_entries contains the total number of L1 entries and
> + * *visited_l1_entries counts all visited L1 entries.
>   */
>  static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
>                                        int l1_size, uint8_t **expanded_clusters,
> -                                      uint64_t *nb_clusters)
> +                                      uint64_t *nb_clusters,
> +                                      int64_t *visited_l1_entries,
> +                                      int64_t l1_entries,
> +                                      BlockDriverAmendStatusCB *status_cb)
>  {
>      BDRVQcowState *s = bs->opaque;
>      bool is_active_l1 = (l1_table == s->l1_table);
> @@ -1571,6 +1578,10 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
>  
>          if (!l2_offset) {
>              /* unallocated */
> +            (*visited_l1_entries)++;
> +            if (status_cb) {
> +                status_cb(bs, *visited_l1_entries, l1_entries);
> +            }
>              continue;
>          }
>  
> @@ -1703,6 +1714,11 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
>                  }
>              }
>          }
> +
> +        (*visited_l1_entries)++;
> +        if (status_cb) {
> +            status_cb(bs, *visited_l1_entries, l1_entries);
> +        }
>      }
>  
>      ret = 0;
> @@ -1729,21 +1745,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 +1804,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 a3a7efc..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;
>      }
> @@ -2289,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
> 
> 

Ok it's the driver responsability to check if the callback is not null.
That make sense.

Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>

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

[Benoît Canet]

The Saturday 02 Aug 2014 à 01:49:19 (+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.
> 
> While touching this function, amend the comment describing the "addend"
> parameter: It is (no longer, if it ever was) necessary to have it set to
> -1 or 1; any value is fine.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2-refcount.c | 26 +++++++++++++-------------
>  block/qcow2.h          |  2 ++
>  2 files changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index cc6cf74..1c2ab8c 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;
> @@ -605,8 +605,7 @@ fail:
>  }
>  
>  /*
> - * Increases or decreases the refcount of a given cluster by one.
> - * addend must be 1 or -1.
> + * Increases or decreases the refcount of a given cluster.
>   *
>   * If the return value is non-negative, it is the new refcount of the cluster.
>   * If it is negative, it is -errno and indicates an error.
> @@ -625,7 +624,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 +645,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 +709,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 +926,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 +966,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 +1242,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 +1264,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 +1306,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 +1597,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: Benoît Canet <benoit.canet@nodalink.com>

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

[Benoît Canet]

The Saturday 02 Aug 2014 à 01:49:20 (+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 2607715..7e65c13 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 used to keep track of progress for
>   * status_cb(). l1_entries contains the total number of L1 entries and
>   * *visited_l1_entries counts all visited L1 entries.
>   */
>  static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table,
> -                                      int l1_size, uint8_t **expanded_clusters,
> -                                      uint64_t *nb_clusters,
> -                                      int64_t *visited_l1_entries,
> +                                      int l1_size, int64_t *visited_l1_entries,
>                                        int64_t l1_entries,
>                                        BlockDriverAmendStatusCB *status_cb)
>  {
> @@ -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 */
> @@ -1598,33 +1591,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;
>              }
>  
> @@ -1642,6 +1621,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);
> @@ -1663,29 +1655,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) {
> @@ -1750,9 +1725,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;
>  
> @@ -1763,12 +1736,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) {
> @@ -1804,7 +1772,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) {
> @@ -1815,7 +1782,6 @@ int qcow2_expand_zero_clusters(BlockDriverState *bs,
>      ret = 0;
>  
>  fail:
> -    g_free(expanded_clusters);
>      g_free(l1_table);
>      return ret;
>  }
> -- 
> 2.0.3
> 

This look like a nice simplification.
Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>

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

[Benoît Canet]

The Saturday 02 Aug 2014 à 01:49:21 (+0200), Max Reitz wrote :
> Add some tests for progress output to 061.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  tests/qemu-iotests/061     | 25 +++++++++++++++++++++++++
>  tests/qemu-iotests/061.out | 30 ++++++++++++++++++++++++++++++
>  tests/qemu-iotests/group   |  2 +-
>  3 files changed, 56 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qemu-iotests/061 b/tests/qemu-iotests/061
> index ab98def..8d37f8a 100755
> --- a/tests/qemu-iotests/061
> +++ b/tests/qemu-iotests/061
> @@ -209,6 +209,31 @@ $QEMU_IMG amend -o "compat=0.10" "$TEST_IMG"
>  _check_test_img
>  $QEMU_IO -c "read -P 0 0 64M" "$TEST_IMG" | _filter_qemu_io
>  
> +echo
> +echo "=== Testing progress report without snapshot ==="
> +echo
> +IMGOPTS="compat=1.1" TEST_IMG="$TEST_IMG.base" _make_test_img 4G
> +IMGOPTS="compat=1.1" _make_test_img -b "$TEST_IMG.base" 4G
> +$QEMU_IO -c "write -z 0  64k" \
> +         -c "write -z 1G 64k" \
> +         -c "write -z 2G 64k" \
> +         -c "write -z 3G 64k" "$TEST_IMG" | _filter_qemu_io
> +$QEMU_IMG amend -p -o "compat=0.10" "$TEST_IMG"
> +_check_test_img
> +
> +echo
> +echo "=== Testing progress report with snapshot ==="
> +echo
> +IMGOPTS="compat=1.1" TEST_IMG="$TEST_IMG.base" _make_test_img 4G
> +IMGOPTS="compat=1.1" _make_test_img -b "$TEST_IMG.base" 4G
> +$QEMU_IO -c "write -z 0  64k" \
> +         -c "write -z 1G 64k" \
> +         -c "write -z 2G 64k" \
> +         -c "write -z 3G 64k" "$TEST_IMG" | _filter_qemu_io
> +$QEMU_IMG snapshot -c foo "$TEST_IMG"
> +$QEMU_IMG amend -p -o "compat=0.10" "$TEST_IMG"
> +_check_test_img
> +
>  # success, all done
>  echo "*** done"
>  rm -f $seq.full
> diff --git a/tests/qemu-iotests/061.out b/tests/qemu-iotests/061.out
> index e372470..789f651 100644
> --- a/tests/qemu-iotests/061.out
> +++ b/tests/qemu-iotests/061.out
> @@ -384,4 +384,34 @@ wrote 67108864/67108864 bytes at offset 0
>  No errors were found on the image.
>  read 67108864/67108864 bytes at offset 0
>  64 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +
> +=== Testing progress report without snapshot ===
> +
> +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=4294967296 
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=4294967296 backing_file='TEST_DIR/t.IMGFMT.base' 
> +wrote 65536/65536 bytes at offset 0
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +wrote 65536/65536 bytes at offset 1073741824
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +wrote 65536/65536 bytes at offset 2147483648
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +wrote 65536/65536 bytes at offset 3221225472
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +    (0.00/100%)\r    (12.50/100%)\r    (25.00/100%)\r    (37.50/100%)\r    (50.00/100%)\r    (62.50/100%)\r    (75.00/100%)\r    (87.50/100%)\r    (100.00/100%)\r    (100.00/100%)
> +No errors were found on the image.
> +
> +=== Testing progress report with snapshot ===
> +
> +Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=4294967296 
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=4294967296 backing_file='TEST_DIR/t.IMGFMT.base' 
> +wrote 65536/65536 bytes at offset 0
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +wrote 65536/65536 bytes at offset 1073741824
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +wrote 65536/65536 bytes at offset 2147483648
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +wrote 65536/65536 bytes at offset 3221225472
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +    (0.00/100%)\r    (6.25/100%)\r    (12.50/100%)\r    (18.75/100%)\r    (25.00/100%)\r    (31.25/100%)\r    (37.50/100%)\r    (43.75/100%)\r    (50.00/100%)\r    (56.25/100%)\r    (62.50/100%)\r    (68.75/100%)\r    (75.00/100%)\r    (81.25/100%)\r    (87.50/100%)\r    (93.75/100%)\r    (100.00/100%)\r    (100.00/100%)
> +No errors were found on the image.
>  *** done
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index 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
> 
Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>

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

[Max Reitz]

On 15.08.2014 13:18, Benoît Canet wrote:
> The Saturday 02 Aug 2014 à 01:49:16 (+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-cmds.hx |  4 ++--
>>   qemu-img.c       | 25 ++++++++++++++++++++++---
>>   qemu-img.texi    |  2 +-
>>   3 files changed, 25 insertions(+), 6 deletions(-)
>>
>> diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
>> index 55aec6b..d83f3d0 100644
>> --- a/qemu-img-cmds.hx
>> +++ b/qemu-img-cmds.hx
>> @@ -70,8 +70,8 @@ STEXI
>>   ETEXI
>>   
>>   DEF("amend", img_amend,
>> -    "amend [-q] [-f fmt] [-t cache] -o options filename")
>> +    "amend [-p] [-q] [-f fmt] [-t cache] -o options filename")
>>   STEXI
>> -@item amend [-q] [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
>> +@item amend [-p] [-q] [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
>>   @end table
>>   ETEXI
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 90d6b79..4b6406b 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -2732,6 +2732,12 @@ out:
>>       return 0;
>>   }
>>   
>> +static void amend_status_cb(BlockDriverState *bs,
>> +                            int64_t offset, int64_t total_work_size)
>> +{
>> +    qemu_progress_print(100.f * offset / total_work_size, 0);
>> +}
>> +
>>   static int img_amend(int argc, char **argv)
>>   {
>>       int c, ret = 0;
>> @@ -2740,12 +2746,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 +2781,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 +2794,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 +2841,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);
>>       }
>> diff --git a/qemu-img.texi b/qemu-img.texi
>> index cb68948..2c66603 100644
>> --- a/qemu-img.texi
>> +++ b/qemu-img.texi
>> @@ -397,7 +397,7 @@ After using this command to grow a disk image, you must use file system and
>>   partitioning tools inside the VM to actually begin using the new space on the
>>   device.
>>   
>> -@item amend [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
>> +@item amend [-p] [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
>>   
>>   Amends the image format specific @var{options} for the image file
>>   @var{filename}. Not all file formats support this operation.
>> -- 
>> 2.0.3
>>
> This patchs is fine and sweet.
> However could we add an assert(amend_status_cb) in bdrv_amend_options somewhere in the series ?
> So if a coder pass NULL as callback he will have a clue near the root cause.

Judging from your response to patch 4, this seems no longer necessary...?

> Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>

Just now seeing your new email address, hopefully I don't end up 
accidentally typing out the old one when adding the tag to commits. :-)

Max

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

[Benoît Canet]

The Friday 15 Aug 2014 à 15:06:12 (+0200), Max Reitz wrote :
> On 15.08.2014 13:18, Benoît Canet wrote:
> >The Saturday 02 Aug 2014 à 01:49:16 (+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-cmds.hx |  4 ++--
> >>  qemu-img.c       | 25 ++++++++++++++++++++++---
> >>  qemu-img.texi    |  2 +-
> >>  3 files changed, 25 insertions(+), 6 deletions(-)
> >>
> >>diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
> >>index 55aec6b..d83f3d0 100644
> >>--- a/qemu-img-cmds.hx
> >>+++ b/qemu-img-cmds.hx
> >>@@ -70,8 +70,8 @@ STEXI
> >>  ETEXI
> >>  DEF("amend", img_amend,
> >>-    "amend [-q] [-f fmt] [-t cache] -o options filename")
> >>+    "amend [-p] [-q] [-f fmt] [-t cache] -o options filename")
> >>  STEXI
> >>-@item amend [-q] [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
> >>+@item amend [-p] [-q] [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
> >>  @end table
> >>  ETEXI
> >>diff --git a/qemu-img.c b/qemu-img.c
> >>index 90d6b79..4b6406b 100644
> >>--- a/qemu-img.c
> >>+++ b/qemu-img.c
> >>@@ -2732,6 +2732,12 @@ out:
> >>      return 0;
> >>  }
> >>+static void amend_status_cb(BlockDriverState *bs,
> >>+                            int64_t offset, int64_t total_work_size)
> >>+{
> >>+    qemu_progress_print(100.f * offset / total_work_size, 0);
> >>+}
> >>+
> >>  static int img_amend(int argc, char **argv)
> >>  {
> >>      int c, ret = 0;
> >>@@ -2740,12 +2746,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 +2781,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 +2794,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 +2841,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);
> >>      }
> >>diff --git a/qemu-img.texi b/qemu-img.texi
> >>index cb68948..2c66603 100644
> >>--- a/qemu-img.texi
> >>+++ b/qemu-img.texi
> >>@@ -397,7 +397,7 @@ After using this command to grow a disk image, you must use file system and
> >>  partitioning tools inside the VM to actually begin using the new space on the
> >>  device.
> >>-@item amend [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
> >>+@item amend [-p] [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
> >>  Amends the image format specific @var{options} for the image file
> >>  @var{filename}. Not all file formats support this operation.
> >>-- 
> >>2.0.3
> >>
> >This patchs is fine and sweet.
> >However could we add an assert(amend_status_cb) in bdrv_amend_options somewhere in the series ?
> >So if a coder pass NULL as callback he will have a clue near the root cause.
> 
> Judging from your response to patch 4, this seems no longer necessary...?

I was thinking about avoiding adding burden to the qemu know how
(like "oh I added a field to BlockDriverState so I must modify bdrv_swap")
but I trust your jugement on this one.

> 
> >Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>
> 
> Just now seeing your new email address, hopefully I don't end up
> accidentally typing out the old one when adding the tag to commits. :-)
> 
> Max
>