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

[Max Reitz <mreitz@redhat.com>]

On 31.07.2014 10:24, Benoît Canet wrote:
> 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 <mreitz@redhat.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 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 ^

As far as I can see, that comment no longer applies (anything in 
update_refcount() very well allows arbitrary values). I'll remove it in v2.

>   *
>   * 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.

Hm? The cluster at "offset" is allocated directly before the call to 
qcow2_update_cluster_refcount(). There is no loop which the latter call 
is in, but the allocation is not.

Max

>> +                    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
>>
>>