Message ID | 1406402531-9278-5-git-send-email-mreitz@redhat.com |
---|---|
State | New |
Headers | show |
On 07/26/2014 01:22 PM, Max Reitz wrote: > The only really time-consuming operation potentially performed by > qcow2_amend_options() is zero cluster expansion when downgrading qcow2 > images from compat=1.1 to compat=0.10, so report status of that > operation and that operation only through the status CB. > > For this, approximate the progress as the number of L1 entries visited > during the operation. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block/qcow2-cluster.c | 36 ++++++++++++++++++++++++++++++++---- > block/qcow2.c | 9 ++++----- > block/qcow2.h | 3 ++- > 3 files changed, 38 insertions(+), 10 deletions(-) Seems like a reasonable approximation. The progress may appear to change non-linearly (slower when a sequence of L1 entries visit all-zero data, faster when the entries visit normal data), but we never promised linear growth. > > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c > index 4208dc0..f8bec6f 100644 > --- a/block/qcow2-cluster.c > +++ b/block/qcow2-cluster.c > @@ -1548,10 +1548,17 @@ fail: > * zero expansion (i.e., has been filled with zeroes and is referenced from an > * L2 table). nb_clusters contains the total cluster count of the image file, > * i.e., the number of bits in expanded_clusters. > + * > + * l1_entries and *visited_l1_entries are ued to keep track of progress for s/ued/used/ Definitely looks simpler than your other approach.
The Saturday 26 Jul 2014 à 21:22:08 (+0200), Max Reitz wrote : > The only really time-consuming operation potentially performed by > qcow2_amend_options() is zero cluster expansion when downgrading qcow2 > images from compat=1.1 to compat=0.10, so report status of that > operation and that operation only through the status CB. > > For this, approximate the progress as the number of L1 entries visited > during the operation. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block/qcow2-cluster.c | 36 ++++++++++++++++++++++++++++++++---- > block/qcow2.c | 9 ++++----- > block/qcow2.h | 3 ++- > 3 files changed, 38 insertions(+), 10 deletions(-) > > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c > index 4208dc0..f8bec6f 100644 > --- a/block/qcow2-cluster.c > +++ b/block/qcow2-cluster.c > @@ -1548,10 +1548,17 @@ fail: > * zero expansion (i.e., has been filled with zeroes and is referenced from an > * L2 table). nb_clusters contains the total cluster count of the image file, > * i.e., the number of bits in expanded_clusters. > + * > + * l1_entries and *visited_l1_entries are ued to keep track of progress for > + * status_cb(). l1_entries contains the total number of L1 entries and > + * *visited_l1_entries counts all visited L1 entries. > */ > static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table, > int l1_size, uint8_t **expanded_clusters, > - uint64_t *nb_clusters) > + uint64_t *nb_clusters, > + int64_t *visited_l1_entries, > + int64_t l1_entries, > + BlockDriverAmendStatusCB *status_cb) > { > BDRVQcowState *s = bs->opaque; > bool is_active_l1 = (l1_table == s->l1_table); > @@ -1571,6 +1578,7 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table, > > if (!l2_offset) { > /* unallocated */ > + (*visited_l1_entries)++; > continue; > } > > @@ -1703,6 +1711,13 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table, > } > } > } > + > + (*visited_l1_entries)++; > + > + if (status_cb) { > + status_cb(bs, *visited_l1_entries << (s->l2_bits + s->cluster_bits), > + l1_entries << (s->l2_bits + s->cluster_bits)); Shifting is a multiplication so it keep proportionality intact. So do we really need these shifts ? > + } > } > > ret = 0; > @@ -1729,21 +1744,32 @@ fail: > * allocation for pre-allocated ones). This is important for downgrading to a > * qcow2 version which doesn't yet support metadata zero clusters. > */ > -int qcow2_expand_zero_clusters(BlockDriverState *bs) > +int qcow2_expand_zero_clusters(BlockDriverState *bs, > + BlockDriverAmendStatusCB *status_cb) > { > BDRVQcowState *s = bs->opaque; > uint64_t *l1_table = NULL; > uint64_t nb_clusters; > + int64_t l1_entries = 0, visited_l1_entries = 0; > uint8_t *expanded_clusters; > int ret; > int i, j; > > + if (status_cb) { > + l1_entries = s->l1_size; > + for (i = 0; i < s->nb_snapshots; i++) { > + l1_entries += s->snapshots[i].l1_size; > + } > + } > + > nb_clusters = size_to_clusters(s, bs->file->total_sectors * > BDRV_SECTOR_SIZE); > expanded_clusters = g_malloc0((nb_clusters + 7) / 8); > > ret = expand_zero_clusters_in_l1(bs, s->l1_table, s->l1_size, > - &expanded_clusters, &nb_clusters); > + &expanded_clusters, &nb_clusters, > + &visited_l1_entries, l1_entries, > + status_cb); > if (ret < 0) { > goto fail; > } > @@ -1777,7 +1803,9 @@ int qcow2_expand_zero_clusters(BlockDriverState *bs) > } > > ret = expand_zero_clusters_in_l1(bs, l1_table, s->snapshots[i].l1_size, > - &expanded_clusters, &nb_clusters); > + &expanded_clusters, &nb_clusters, > + &visited_l1_entries, l1_entries, > + status_cb); > if (ret < 0) { > goto fail; > } > diff --git a/block/qcow2.c b/block/qcow2.c > index 757f890..6e8c8ab 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -2147,7 +2147,8 @@ static int qcow2_load_vmstate(BlockDriverState *bs, uint8_t *buf, > * Downgrades an image's version. To achieve this, any incompatible features > * have to be removed. > */ > -static int qcow2_downgrade(BlockDriverState *bs, int target_version) > +static int qcow2_downgrade(BlockDriverState *bs, int target_version, > + BlockDriverAmendStatusCB *status_cb) > { > BDRVQcowState *s = bs->opaque; > int current_version = s->qcow_version; > @@ -2196,7 +2197,7 @@ static int qcow2_downgrade(BlockDriverState *bs, int target_version) > /* clearing autoclear features is trivial */ > s->autoclear_features = 0; > > - ret = qcow2_expand_zero_clusters(bs); > + ret = qcow2_expand_zero_clusters(bs, status_cb); > if (ret < 0) { > return ret; > } > @@ -2224,8 +2225,6 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts, > int ret; > QemuOptDesc *desc = opts->list->desc; > > - (void)status_cb; > - > while (desc && desc->name) { > if (!qemu_opt_find(opts, desc->name)) { > /* only change explicitly defined options */ > @@ -2291,7 +2290,7 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts, > return ret; > } > } else { > - ret = qcow2_downgrade(bs, new_version); > + ret = qcow2_downgrade(bs, new_version, status_cb); > if (ret < 0) { > return ret; > } > diff --git a/block/qcow2.h b/block/qcow2.h > index b49424b..1c4f9bf 100644 > --- a/block/qcow2.h > +++ b/block/qcow2.h > @@ -522,7 +522,8 @@ int qcow2_discard_clusters(BlockDriverState *bs, uint64_t offset, > int nb_sectors, enum qcow2_discard_type type); > int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int nb_sectors); > > -int qcow2_expand_zero_clusters(BlockDriverState *bs); > +int qcow2_expand_zero_clusters(BlockDriverState *bs, > + BlockDriverAmendStatusCB *status_cb); > > /* qcow2-snapshot.c functions */ > int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info); > -- > 2.0.3 > > Reviewed-by: Benoit Canet <benoit@irqsave.net>
On 31.07.2014 10:06, Benoît Canet wrote: > The Saturday 26 Jul 2014 à 21:22:08 (+0200), Max Reitz wrote : >> The only really time-consuming operation potentially performed by >> qcow2_amend_options() is zero cluster expansion when downgrading qcow2 >> images from compat=1.1 to compat=0.10, so report status of that >> operation and that operation only through the status CB. >> >> For this, approximate the progress as the number of L1 entries visited >> during the operation. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> block/qcow2-cluster.c | 36 ++++++++++++++++++++++++++++++++---- >> block/qcow2.c | 9 ++++----- >> block/qcow2.h | 3 ++- >> 3 files changed, 38 insertions(+), 10 deletions(-) >> >> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c >> index 4208dc0..f8bec6f 100644 >> --- a/block/qcow2-cluster.c >> +++ b/block/qcow2-cluster.c >> @@ -1548,10 +1548,17 @@ fail: >> * zero expansion (i.e., has been filled with zeroes and is referenced from an >> * L2 table). nb_clusters contains the total cluster count of the image file, >> * i.e., the number of bits in expanded_clusters. >> + * >> + * l1_entries and *visited_l1_entries are ued to keep track of progress for >> + * status_cb(). l1_entries contains the total number of L1 entries and >> + * *visited_l1_entries counts all visited L1 entries. >> */ >> static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table, >> int l1_size, uint8_t **expanded_clusters, >> - uint64_t *nb_clusters) >> + uint64_t *nb_clusters, >> + int64_t *visited_l1_entries, >> + int64_t l1_entries, >> + BlockDriverAmendStatusCB *status_cb) >> { >> BDRVQcowState *s = bs->opaque; >> bool is_active_l1 = (l1_table == s->l1_table); >> @@ -1571,6 +1578,7 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table, >> >> if (!l2_offset) { >> /* unallocated */ >> + (*visited_l1_entries)++; >> continue; >> } >> >> @@ -1703,6 +1711,13 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table, >> } >> } >> } >> + >> + (*visited_l1_entries)++; >> + >> + if (status_cb) { >> + status_cb(bs, *visited_l1_entries << (s->l2_bits + s->cluster_bits), >> + l1_entries << (s->l2_bits + s->cluster_bits)); > Shifting is a multiplication so it keep proportionality intact. > So do we really need these shifts ? As of patch 1, the variables are defined as "offset" and "working_size" which I meant to be byte ranges. We could indeed leave the unit for BlockDriverAmendStatusCB's parameters undefined (and leave it up to the block driver, only specifying that offset / working_size has to be the progress ratio), but then we could just as well just give a floating point percentage to that function. Bytes as a unit seemed safe to me, however, since all of qemu's code assumes byte ranges to always fit in int64_t; and the reason I preferred them over a percentage is that block jobs use bytes as well. A real reason not to use bytes would be that some driver is unable to give a "byte" representation of its workload during the amend progress; however, this seems unlikely to me (every large workload which should be part of the progress report should be somehow representable as bytes). Max >> + } >> } >> >> ret = 0; >> @@ -1729,21 +1744,32 @@ fail: >> * allocation for pre-allocated ones). This is important for downgrading to a >> * qcow2 version which doesn't yet support metadata zero clusters. >> */ >> -int qcow2_expand_zero_clusters(BlockDriverState *bs) >> +int qcow2_expand_zero_clusters(BlockDriverState *bs, >> + BlockDriverAmendStatusCB *status_cb) >> { >> BDRVQcowState *s = bs->opaque; >> uint64_t *l1_table = NULL; >> uint64_t nb_clusters; >> + int64_t l1_entries = 0, visited_l1_entries = 0; >> uint8_t *expanded_clusters; >> int ret; >> int i, j; >> >> + if (status_cb) { >> + l1_entries = s->l1_size; >> + for (i = 0; i < s->nb_snapshots; i++) { >> + l1_entries += s->snapshots[i].l1_size; >> + } >> + } >> + >> nb_clusters = size_to_clusters(s, bs->file->total_sectors * >> BDRV_SECTOR_SIZE); >> expanded_clusters = g_malloc0((nb_clusters + 7) / 8); >> >> ret = expand_zero_clusters_in_l1(bs, s->l1_table, s->l1_size, >> - &expanded_clusters, &nb_clusters); >> + &expanded_clusters, &nb_clusters, >> + &visited_l1_entries, l1_entries, >> + status_cb); >> if (ret < 0) { >> goto fail; >> } >> @@ -1777,7 +1803,9 @@ int qcow2_expand_zero_clusters(BlockDriverState *bs) >> } >> >> ret = expand_zero_clusters_in_l1(bs, l1_table, s->snapshots[i].l1_size, >> - &expanded_clusters, &nb_clusters); >> + &expanded_clusters, &nb_clusters, >> + &visited_l1_entries, l1_entries, >> + status_cb); >> if (ret < 0) { >> goto fail; >> } >> diff --git a/block/qcow2.c b/block/qcow2.c >> index 757f890..6e8c8ab 100644 >> --- a/block/qcow2.c >> +++ b/block/qcow2.c >> @@ -2147,7 +2147,8 @@ static int qcow2_load_vmstate(BlockDriverState *bs, uint8_t *buf, >> * Downgrades an image's version. To achieve this, any incompatible features >> * have to be removed. >> */ >> -static int qcow2_downgrade(BlockDriverState *bs, int target_version) >> +static int qcow2_downgrade(BlockDriverState *bs, int target_version, >> + BlockDriverAmendStatusCB *status_cb) >> { >> BDRVQcowState *s = bs->opaque; >> int current_version = s->qcow_version; >> @@ -2196,7 +2197,7 @@ static int qcow2_downgrade(BlockDriverState *bs, int target_version) >> /* clearing autoclear features is trivial */ >> s->autoclear_features = 0; >> >> - ret = qcow2_expand_zero_clusters(bs); >> + ret = qcow2_expand_zero_clusters(bs, status_cb); >> if (ret < 0) { >> return ret; >> } >> @@ -2224,8 +2225,6 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts, >> int ret; >> QemuOptDesc *desc = opts->list->desc; >> >> - (void)status_cb; >> - >> while (desc && desc->name) { >> if (!qemu_opt_find(opts, desc->name)) { >> /* only change explicitly defined options */ >> @@ -2291,7 +2290,7 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts, >> return ret; >> } >> } else { >> - ret = qcow2_downgrade(bs, new_version); >> + ret = qcow2_downgrade(bs, new_version, status_cb); >> if (ret < 0) { >> return ret; >> } >> diff --git a/block/qcow2.h b/block/qcow2.h >> index b49424b..1c4f9bf 100644 >> --- a/block/qcow2.h >> +++ b/block/qcow2.h >> @@ -522,7 +522,8 @@ int qcow2_discard_clusters(BlockDriverState *bs, uint64_t offset, >> int nb_sectors, enum qcow2_discard_type type); >> int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int nb_sectors); >> >> -int qcow2_expand_zero_clusters(BlockDriverState *bs); >> +int qcow2_expand_zero_clusters(BlockDriverState *bs, >> + BlockDriverAmendStatusCB *status_cb); >> >> /* qcow2-snapshot.c functions */ >> int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info); >> -- >> 2.0.3 >> >> > Reviewed-by: Benoit Canet <benoit@irqsave.net>
On 08/01/2014 02:18 PM, Max Reitz wrote: >>> + if (status_cb) { >>> + status_cb(bs, *visited_l1_entries << (s->l2_bits + >>> s->cluster_bits), >>> + l1_entries << (s->l2_bits + s->cluster_bits)); >> Shifting is a multiplication so it keep proportionality intact. >> So do we really need these shifts ? > > As of patch 1, the variables are defined as "offset" and "working_size" > which I meant to be byte ranges. We could indeed leave the unit for > BlockDriverAmendStatusCB's parameters undefined (and leave it up to the > block driver, only specifying that offset / working_size has to be the > progress ratio), but then we could just as well just give a floating > point percentage to that function. As it is, block jobs are already documented as merely exposing completion percentage, and NOT tied to the size of the underlying block device. Your own pending patch is proof of this: https://lists.gnu.org/archive/html/qemu-devel/2014-07/msg00960.html When doing drive-mirror, we WANT to have the total size grow according to how many dirty blocks are encountered through each pass, and the current offset grow in rough proportion to how fast we are converging to a mirrored state (or even having the percentage go backwards, when we are diverging from too much I/O pressure, as a sign that some throttling is needed). Artificially trying to constrain that progress reporting to the size in bytes of the block device does not help matters. > > Bytes as a unit seemed safe to me, however, since all of qemu's code > assumes byte ranges to always fit in int64_t; and the reason I preferred > them over a percentage is that block jobs use bytes as well. > > A real reason not to use bytes would be that some driver is unable to > give a "byte" representation of its workload during the amend progress; > however, this seems unlikely to me (every large workload which should be > part of the progress report should be somehow representable as bytes). I don't think we can promise anything more than two relative numbers, with no bearing on what they are measuring under the hood, so scaling both numbers just to produce progress output buys nothing. There may eventually be a device where we can't report progress in any more granularity than 0 (still working) or 1 (done).
On 01.08.2014 22:38, Eric Blake wrote: > On 08/01/2014 02:18 PM, Max Reitz wrote: > >>>> + if (status_cb) { >>>> + status_cb(bs, *visited_l1_entries << (s->l2_bits + >>>> s->cluster_bits), >>>> + l1_entries << (s->l2_bits + s->cluster_bits)); >>> Shifting is a multiplication so it keep proportionality intact. >>> So do we really need these shifts ? >> As of patch 1, the variables are defined as "offset" and "working_size" >> which I meant to be byte ranges. We could indeed leave the unit for >> BlockDriverAmendStatusCB's parameters undefined (and leave it up to the >> block driver, only specifying that offset / working_size has to be the >> progress ratio), but then we could just as well just give a floating >> point percentage to that function. > As it is, block jobs are already documented as merely exposing > completion percentage, and NOT tied to the size of the underlying block > device. Your own pending patch is proof of this: > > https://lists.gnu.org/archive/html/qemu-devel/2014-07/msg00960.html > > When doing drive-mirror, we WANT to have the total size grow according > to how many dirty blocks are encountered through each pass, and the > current offset grow in rough proportion to how fast we are converging to > a mirrored state (or even having the percentage go backwards, when we > are diverging from too much I/O pressure, as a sign that some throttling > is needed). Artificially trying to constrain that progress reporting to > the size in bytes of the block device does not help matters. Well, this would just as well work with a single floating point percentage. >> Bytes as a unit seemed safe to me, however, since all of qemu's code >> assumes byte ranges to always fit in int64_t; and the reason I preferred >> them over a percentage is that block jobs use bytes as well. >> >> A real reason not to use bytes would be that some driver is unable to >> give a "byte" representation of its workload during the amend progress; >> however, this seems unlikely to me (every large workload which should be >> part of the progress report should be somehow representable as bytes). > I don't think we can promise anything more than two relative numbers, > with no bearing on what they are measuring under the hood, so scaling > both numbers just to produce progress output buys nothing. There may > eventually be a device where we can't report progress in any more > granularity than 0 (still working) or 1 (done). In that case it should never call the callback. Note how qcow2 can do a whole number of things including resizing the image during an amend operation; but only the zero cluster expansion is costly enough to justify a progress output. Consequently, it's only that function which invokes the callback. If the image is not being downgraded from compat=1.1 to compat=0.10, the callback is never invoked; there even is a small comment about this in qemu-img.c (/* In case the driver does not call amend_status_cb() */). I can however see that a driver invokes a couple of time-consuming operations and there is no progress report for each of those; so uses the number of operations as the workload size and the parameter currently named "offset" as the index of the operation executed next. Frankly, I'd blame the driver then, since the operations should give a real progress report (which I still think *has* to be convertable to bytes somehow). Anyway, you're probably both right and we should just drop the unit enforcement and allow the driver to use any unit it desires; but in this case I still think we could just give a floating point percentage instead of a numerator and a denominator. Max
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index 4208dc0..f8bec6f 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -1548,10 +1548,17 @@ fail: * zero expansion (i.e., has been filled with zeroes and is referenced from an * L2 table). nb_clusters contains the total cluster count of the image file, * i.e., the number of bits in expanded_clusters. + * + * l1_entries and *visited_l1_entries are ued to keep track of progress for + * status_cb(). l1_entries contains the total number of L1 entries and + * *visited_l1_entries counts all visited L1 entries. */ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table, int l1_size, uint8_t **expanded_clusters, - uint64_t *nb_clusters) + uint64_t *nb_clusters, + int64_t *visited_l1_entries, + int64_t l1_entries, + BlockDriverAmendStatusCB *status_cb) { BDRVQcowState *s = bs->opaque; bool is_active_l1 = (l1_table == s->l1_table); @@ -1571,6 +1578,7 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table, if (!l2_offset) { /* unallocated */ + (*visited_l1_entries)++; continue; } @@ -1703,6 +1711,13 @@ static int expand_zero_clusters_in_l1(BlockDriverState *bs, uint64_t *l1_table, } } } + + (*visited_l1_entries)++; + + if (status_cb) { + status_cb(bs, *visited_l1_entries << (s->l2_bits + s->cluster_bits), + l1_entries << (s->l2_bits + s->cluster_bits)); + } } ret = 0; @@ -1729,21 +1744,32 @@ fail: * allocation for pre-allocated ones). This is important for downgrading to a * qcow2 version which doesn't yet support metadata zero clusters. */ -int qcow2_expand_zero_clusters(BlockDriverState *bs) +int qcow2_expand_zero_clusters(BlockDriverState *bs, + BlockDriverAmendStatusCB *status_cb) { BDRVQcowState *s = bs->opaque; uint64_t *l1_table = NULL; uint64_t nb_clusters; + int64_t l1_entries = 0, visited_l1_entries = 0; uint8_t *expanded_clusters; int ret; int i, j; + if (status_cb) { + l1_entries = s->l1_size; + for (i = 0; i < s->nb_snapshots; i++) { + l1_entries += s->snapshots[i].l1_size; + } + } + nb_clusters = size_to_clusters(s, bs->file->total_sectors * BDRV_SECTOR_SIZE); expanded_clusters = g_malloc0((nb_clusters + 7) / 8); ret = expand_zero_clusters_in_l1(bs, s->l1_table, s->l1_size, - &expanded_clusters, &nb_clusters); + &expanded_clusters, &nb_clusters, + &visited_l1_entries, l1_entries, + status_cb); if (ret < 0) { goto fail; } @@ -1777,7 +1803,9 @@ int qcow2_expand_zero_clusters(BlockDriverState *bs) } ret = expand_zero_clusters_in_l1(bs, l1_table, s->snapshots[i].l1_size, - &expanded_clusters, &nb_clusters); + &expanded_clusters, &nb_clusters, + &visited_l1_entries, l1_entries, + status_cb); if (ret < 0) { goto fail; } diff --git a/block/qcow2.c b/block/qcow2.c index 757f890..6e8c8ab 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2147,7 +2147,8 @@ static int qcow2_load_vmstate(BlockDriverState *bs, uint8_t *buf, * Downgrades an image's version. To achieve this, any incompatible features * have to be removed. */ -static int qcow2_downgrade(BlockDriverState *bs, int target_version) +static int qcow2_downgrade(BlockDriverState *bs, int target_version, + BlockDriverAmendStatusCB *status_cb) { BDRVQcowState *s = bs->opaque; int current_version = s->qcow_version; @@ -2196,7 +2197,7 @@ static int qcow2_downgrade(BlockDriverState *bs, int target_version) /* clearing autoclear features is trivial */ s->autoclear_features = 0; - ret = qcow2_expand_zero_clusters(bs); + ret = qcow2_expand_zero_clusters(bs, status_cb); if (ret < 0) { return ret; } @@ -2224,8 +2225,6 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts, int ret; QemuOptDesc *desc = opts->list->desc; - (void)status_cb; - while (desc && desc->name) { if (!qemu_opt_find(opts, desc->name)) { /* only change explicitly defined options */ @@ -2291,7 +2290,7 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts, return ret; } } else { - ret = qcow2_downgrade(bs, new_version); + ret = qcow2_downgrade(bs, new_version, status_cb); if (ret < 0) { return ret; } diff --git a/block/qcow2.h b/block/qcow2.h index b49424b..1c4f9bf 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -522,7 +522,8 @@ int qcow2_discard_clusters(BlockDriverState *bs, uint64_t offset, int nb_sectors, enum qcow2_discard_type type); int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int nb_sectors); -int qcow2_expand_zero_clusters(BlockDriverState *bs); +int qcow2_expand_zero_clusters(BlockDriverState *bs, + BlockDriverAmendStatusCB *status_cb); /* qcow2-snapshot.c functions */ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info);
The only really time-consuming operation potentially performed by qcow2_amend_options() is zero cluster expansion when downgrading qcow2 images from compat=1.1 to compat=0.10, so report status of that operation and that operation only through the status CB. For this, approximate the progress as the number of L1 entries visited during the operation. Signed-off-by: Max Reitz <mreitz@redhat.com> --- block/qcow2-cluster.c | 36 ++++++++++++++++++++++++++++++++---- block/qcow2.c | 9 ++++----- block/qcow2.h | 3 ++- 3 files changed, 38 insertions(+), 10 deletions(-)