* [Qemu-devel] [PATCH v3 1/7] block: Add status callback to bdrv_amend_options()
2014-08-15 15:47 [Qemu-devel] [PATCH v3 0/7] block/qcow2: Improve zero cluster expansion Max Reitz
@ 2014-08-15 15:47 ` Max Reitz
2014-08-15 15:47 ` [Qemu-devel] [PATCH v3 2/7] qemu-img: Add progress output for amend Max Reitz
` (6 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Max Reitz @ 2014-08-15 15:47 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, Max Reitz
Depending on the changed options and the image format,
bdrv_amend_options() may take a significant amount of time. In these
cases, a way to be informed about the operation's status is desirable.
Since the operation is rather complex and may fundamentally change the
image, implementing it as AIO or a coroutine does not seem feasible. On
the other hand, implementing it as a block job would be significantly
more difficult than a simple callback and would not add benefits other
than progress report to the amending operation, because it should not
actually be run as a block job at all.
A callback may not be very pretty, but it's very easy to implement and
perfectly fits its purpose here.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>
---
block.c | 5 +++--
block/qcow2.c | 3 ++-
include/block/block.h | 8 +++++++-
include/block/block_int.h | 3 ++-
qemu-img.c | 2 +-
5 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/block.c b/block.c
index 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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [Qemu-devel] [PATCH v3 2/7] qemu-img: Add progress output for amend
2014-08-15 15:47 [Qemu-devel] [PATCH v3 0/7] block/qcow2: Improve zero cluster expansion Max Reitz
2014-08-15 15:47 ` [Qemu-devel] [PATCH v3 1/7] block: Add status callback to bdrv_amend_options() Max Reitz
@ 2014-08-15 15:47 ` Max Reitz
2014-08-15 15:47 ` [Qemu-devel] [PATCH v3 3/7] qemu-img: Fix insignificant memleak Max Reitz
` (5 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Max Reitz @ 2014-08-15 15:47 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, Max Reitz
Now that bdrv_amend_options() supports a status callback, use it to
display a progress report.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>
---
qemu-img-cmds.hx | 4 ++--
qemu-img.c | 25 ++++++++++++++++++++++---
qemu-img.texi | 2 +-
3 files changed, 25 insertions(+), 6 deletions(-)
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 55aec6b..d83f3d0 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -70,8 +70,8 @@ STEXI
ETEXI
DEF("amend", img_amend,
- "amend [-q] [-f fmt] [-t cache] -o options filename")
+ "amend [-p] [-q] [-f fmt] [-t cache] -o options filename")
STEXI
-@item amend [-q] [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
+@item amend [-p] [-q] [-f @var{fmt}] [-t @var{cache}] -o @var{options} @var{filename}
@end table
ETEXI
diff --git a/qemu-img.c b/qemu-img.c
index 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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [Qemu-devel] [PATCH v3 3/7] qemu-img: Fix insignificant memleak
2014-08-15 15:47 [Qemu-devel] [PATCH v3 0/7] block/qcow2: Improve zero cluster expansion Max Reitz
2014-08-15 15:47 ` [Qemu-devel] [PATCH v3 1/7] block: Add status callback to bdrv_amend_options() Max Reitz
2014-08-15 15:47 ` [Qemu-devel] [PATCH v3 2/7] qemu-img: Add progress output for amend Max Reitz
@ 2014-08-15 15:47 ` Max Reitz
2014-08-15 15:47 ` [Qemu-devel] [PATCH v3 4/7] block/qcow2: Implement status CB for amend Max Reitz
` (4 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Max Reitz @ 2014-08-15 15:47 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [Qemu-devel] [PATCH v3 4/7] block/qcow2: Implement status CB for amend
2014-08-15 15:47 [Qemu-devel] [PATCH v3 0/7] block/qcow2: Improve zero cluster expansion Max Reitz
` (2 preceding siblings ...)
2014-08-15 15:47 ` [Qemu-devel] [PATCH v3 3/7] qemu-img: Fix insignificant memleak Max Reitz
@ 2014-08-15 15:47 ` Max Reitz
2014-08-15 15:47 ` [Qemu-devel] [PATCH v3 5/7] block/qcow2: Make get_refcount() global Max Reitz
` (3 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Max Reitz @ 2014-08-15 15:47 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, Max Reitz
The only really time-consuming operation potentially performed by
qcow2_amend_options() is zero cluster expansion when downgrading qcow2
images from compat=1.1 to compat=0.10, so report status of that
operation and that operation only through the status CB.
For this, approximate the progress as the number of L1 entries visited
during the operation.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>
---
block/qcow2-cluster.c | 37 +++++++++++++++++++++++++++++++++----
block/qcow2.c | 7 ++++---
block/qcow2.h | 3 ++-
3 files changed, 39 insertions(+), 8 deletions(-)
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [Qemu-devel] [PATCH v3 5/7] block/qcow2: Make get_refcount() global
2014-08-15 15:47 [Qemu-devel] [PATCH v3 0/7] block/qcow2: Improve zero cluster expansion Max Reitz
` (3 preceding siblings ...)
2014-08-15 15:47 ` [Qemu-devel] [PATCH v3 4/7] block/qcow2: Implement status CB for amend Max Reitz
@ 2014-08-15 15:47 ` Max Reitz
2014-08-15 15:47 ` [Qemu-devel] [PATCH v3 6/7] block/qcow2: Simplify shared L2 handling in amend Max Reitz
` (2 subsequent siblings)
7 siblings, 0 replies; 9+ messages in thread
From: Max Reitz @ 2014-08-15 15:47 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, Max Reitz
Reading the refcount of a cluster is an operation which can be useful in
all of the qcow2 code, so make that function globally available.
While touching this function, amend the comment describing the "addend"
parameter: It is (no longer, if it ever was) necessary to have it set to
-1 or 1; any value is fine.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>
---
block/qcow2-refcount.c | 26 +++++++++++++-------------
block/qcow2.h | 2 ++
2 files changed, 15 insertions(+), 13 deletions(-)
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [Qemu-devel] [PATCH v3 6/7] block/qcow2: Simplify shared L2 handling in amend
2014-08-15 15:47 [Qemu-devel] [PATCH v3 0/7] block/qcow2: Improve zero cluster expansion Max Reitz
` (4 preceding siblings ...)
2014-08-15 15:47 ` [Qemu-devel] [PATCH v3 5/7] block/qcow2: Make get_refcount() global Max Reitz
@ 2014-08-15 15:47 ` Max Reitz
2014-08-15 15:47 ` [Qemu-devel] [PATCH v3 7/7] iotests: Expand test 061 Max Reitz
2014-10-08 19:48 ` [Qemu-devel] [PATCH v3 0/7] block/qcow2: Improve zero cluster expansion Max Reitz
7 siblings, 0 replies; 9+ messages in thread
From: Max Reitz @ 2014-08-15 15:47 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, Max Reitz
Currently, we have a bitmap for keeping track of which clusters have
been created during the zero cluster expansion process. This was
necessary because we need to properly increase the refcount for shared
L2 tables.
However, now we can simply take the L2 refcount and use it for the
cluster allocated for expansion. This will be the correct refcount and
therefore we don't have to remember that cluster having been allocated
any more.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>
---
block/qcow2-cluster.c | 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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [Qemu-devel] [PATCH v3 7/7] iotests: Expand test 061
2014-08-15 15:47 [Qemu-devel] [PATCH v3 0/7] block/qcow2: Improve zero cluster expansion Max Reitz
` (5 preceding siblings ...)
2014-08-15 15:47 ` [Qemu-devel] [PATCH v3 6/7] block/qcow2: Simplify shared L2 handling in amend Max Reitz
@ 2014-08-15 15:47 ` Max Reitz
2014-10-08 19:48 ` [Qemu-devel] [PATCH v3 0/7] block/qcow2: Improve zero cluster expansion Max Reitz
7 siblings, 0 replies; 9+ messages in thread
From: Max Reitz @ 2014-08-15 15:47 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, Max Reitz
Add some tests for progress output to 061.
Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Benoît Canet <benoit.canet@nodalink.com>
---
tests/qemu-iotests/061 | 25 +++++++++++++++++++++++++
tests/qemu-iotests/061.out | 30 ++++++++++++++++++++++++++++++
tests/qemu-iotests/group | 2 +-
3 files changed, 56 insertions(+), 1 deletion(-)
diff --git a/tests/qemu-iotests/061 b/tests/qemu-iotests/061
index ab98def..8d37f8a 100755
--- a/tests/qemu-iotests/061
+++ b/tests/qemu-iotests/061
@@ -209,6 +209,31 @@ $QEMU_IMG amend -o "compat=0.10" "$TEST_IMG"
_check_test_img
$QEMU_IO -c "read -P 0 0 64M" "$TEST_IMG" | _filter_qemu_io
+echo
+echo "=== Testing progress report without snapshot ==="
+echo
+IMGOPTS="compat=1.1" TEST_IMG="$TEST_IMG.base" _make_test_img 4G
+IMGOPTS="compat=1.1" _make_test_img -b "$TEST_IMG.base" 4G
+$QEMU_IO -c "write -z 0 64k" \
+ -c "write -z 1G 64k" \
+ -c "write -z 2G 64k" \
+ -c "write -z 3G 64k" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IMG amend -p -o "compat=0.10" "$TEST_IMG"
+_check_test_img
+
+echo
+echo "=== Testing progress report with snapshot ==="
+echo
+IMGOPTS="compat=1.1" TEST_IMG="$TEST_IMG.base" _make_test_img 4G
+IMGOPTS="compat=1.1" _make_test_img -b "$TEST_IMG.base" 4G
+$QEMU_IO -c "write -z 0 64k" \
+ -c "write -z 1G 64k" \
+ -c "write -z 2G 64k" \
+ -c "write -z 3G 64k" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IMG snapshot -c foo "$TEST_IMG"
+$QEMU_IMG amend -p -o "compat=0.10" "$TEST_IMG"
+_check_test_img
+
# success, all done
echo "*** done"
rm -f $seq.full
diff --git a/tests/qemu-iotests/061.out b/tests/qemu-iotests/061.out
index 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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH v3 0/7] block/qcow2: Improve zero cluster expansion
2014-08-15 15:47 [Qemu-devel] [PATCH v3 0/7] block/qcow2: Improve zero cluster expansion Max Reitz
` (6 preceding siblings ...)
2014-08-15 15:47 ` [Qemu-devel] [PATCH v3 7/7] iotests: Expand test 061 Max Reitz
@ 2014-10-08 19:48 ` Max Reitz
7 siblings, 0 replies; 9+ messages in thread
From: Max Reitz @ 2014-10-08 19:48 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi
On 15.08.2014 17:47, Max Reitz wrote:
> The main purpose of this series is to add a progress report to
> qemu-img amend. This is achieved by adding a callback function to
> bdrv_amend_options() - the reasons for this choice are explained in
> patch 1.
>
> While adapting qcow2's expand_zero_clusters_in_l1() accordingly, I
> noticed a way to simplify it and get rid of the rather ugly bitmap used
> there (patch 6).
>
> This series depends on v2 of my "qemu-img: Allow source cache mode
> specification" series.
>
>
> Changes from v2:
> - Patch 1: Fixed misspelling in the commit message [Eric]
>
>
> git-backport-diff against v2:
>
> Key:
> [----] : patches are identical
> [####] : number of functional differences between upstream/downstream patch
> [down] : patch is downstream-only
> The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
>
> 001/7:[----] [--] 'block: Add status callback to bdrv_amend_options()'
> 002/7:[----] [--] 'qemu-img: Add progress output for amend'
> 003/7:[----] [--] 'qemu-img: Fix insignificant memleak'
> 004/7:[----] [--] 'block/qcow2: Implement status CB for amend'
> 005/7:[----] [--] 'block/qcow2: Make get_refcount() global'
> 006/7:[----] [--] 'block/qcow2: Simplify shared L2 handling in amend'
> 007/7:[----] [--] 'iotests: Expand test 061'
>
>
> Max Reitz (7):
> block: Add status callback to bdrv_amend_options()
> qemu-img: Add progress output for amend
> qemu-img: Fix insignificant memleak
> block/qcow2: Implement status CB for amend
> block/qcow2: Make get_refcount() global
> block/qcow2: Simplify shared L2 handling in amend
> iotests: Expand test 061
>
> block.c | 5 +-
> block/qcow2-cluster.c | 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(-)
Ping. All patches have been reviewed by Eric and Benoît.
Max
^ permalink raw reply [flat|nested] 9+ messages in thread