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

[Benoît Canet]

The Saturday 26 Jul 2014 à 21:22:10 (+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 <address@hidden>
> ---
>  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 f8bec6f..e6bff40 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 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,
> -                                      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 */
> @@ -1595,33 +1588,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;
>              }
>  
> @@ -1639,6 +1618,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);

This look like a wrong usage of qcow2_update_cluster_refcount:

/*                                                                              
 * Increases or decreases the refcount of a given cluster by one.               
 * addend must be 1 or -1.                                                      
Here ^
 *                                                                              
 * 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.                      
 */                                                                             
int qcow2_update_cluster_refcount(BlockDriverState *bs,                         
                                  int64_t cluster_index,                        
                                  int addend,                                   
                                  enum qcow2_discard_type type)  

Also this call is in a loop it would do l2_refcount - 1 * n increments on the 
refcount.

> +                    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);
> @@ -1660,29 +1652,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) {
> @@ -1749,9 +1724,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;
>  
> @@ -1762,12 +1735,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) {
> @@ -1803,7 +1771,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) {
> @@ -1814,7 +1781,6 @@ int qcow2_expand_zero_clusters(BlockDriverState *bs,
>      ret = 0;
>  
>  fail:
> -    g_free(expanded_clusters);
>      g_free(l1_table);
>      return ret;
>  }
> -- 
> 2.0.3
> 
>