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

[Max Reitz]

On 31.07.2014 10:06, Benoît Canet wrote:
> The Saturday 26 Jul 2014 à 21:22:08 (+0200), Max Reitz wrote :
> > The only really time-consuming operation potentially performed by
> > qcow2_amend_options() is zero cluster expansion when downgrading qcow2
> > images from compat=1.1 to compat=0.10, so report status of that
> > operation and that operation only through the status CB.
> >
> > For this, approximate the progress as the number of L1 entries visited
> > during the operation.
> >
> > Signed-off-by: Max Reitz <address@hidden>
> > ---
> >   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 <address@hidden>