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 <address@hidden>
> ---
>  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 <address@hidden>