Re: [PATCH v4 05/30] qcow2: Process QCOW2_CLUSTER_ZERO_ALLOC clusters in handle_copied()

[Alberto Garcia]

On Thu 09 Apr 2020 12:59:30 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
>>   static void calculate_l2_meta(BlockDriverState *bs,
>>                                 uint64_t host_cluster_offset,
>>                                 uint64_t guest_offset, unsigned bytes,
>> -                              QCowL2Meta **m, bool keep_old)
>> +                              uint64_t *l2_slice, QCowL2Meta **m, bool 
>> keep_old)
>>   {
>>       BDRVQcow2State *s = bs->opaque;
>> -    unsigned cow_start_from = 0;
>> +    int l2_index = offset_to_l2_slice_index(s, guest_offset);
>> +    uint64_t l2_entry;
>> +    unsigned cow_start_from, cow_end_to;
>>       unsigned cow_start_to = offset_into_cluster(s, guest_offset);
>>       unsigned cow_end_from = cow_start_to + bytes;
>> -    unsigned cow_end_to = ROUND_UP(cow_end_from, s->cluster_size);
>>       unsigned nb_clusters = size_to_clusters(s, cow_end_from);
>>       QCowL2Meta *old_m = *m;
>> +    QCow2ClusterType type;
>> +
>> +    assert(nb_clusters <= s->l2_slice_size - l2_index);
>> +
>> +    /* Return if there's no COW (all clusters are normal and we keep them) 
>> */
>> +    if (keep_old) {
>> +        int i;
>> +        for (i = 0; i < nb_clusters; i++) {
>> +            l2_entry = be64_to_cpu(l2_slice[l2_index + i]);
>> +            if (qcow2_get_cluster_type(bs, l2_entry) != 
>> QCOW2_CLUSTER_NORMAL) {
>
> Could we also allow full ZERO_ALLOC clusters here?

No, because the L2 entry needs to be modified (in order to remove the
'all zeroes' bit) and we need to create a QCowL2Meta entry for that (see
qcow2_handle_l2meta()).

>> +    /* Get the L2 entry of the first cluster */
>> +    l2_entry = be64_to_cpu(l2_slice[l2_index]);
>> +    type = qcow2_get_cluster_type(bs, l2_entry);
>> +
>> +    if (type == QCOW2_CLUSTER_NORMAL && keep_old) {
>> +        cow_start_from = cow_start_to;
>> +    } else {
>> +        cow_start_from = 0;
>> +    }
>> +
>> +    /* Get the L2 entry of the last cluster */
>> +    l2_entry = be64_to_cpu(l2_slice[l2_index + nb_clusters - 1]);
>> +    type = qcow2_get_cluster_type(bs, l2_entry);
>> +
>> +    if (type == QCOW2_CLUSTER_NORMAL && keep_old) {
>> +        cow_end_to = cow_end_from;
>> +    } else {
>> +        cow_end_to = ROUND_UP(cow_end_from, s->cluster_size);
>> +    }
>
> These two ifs may be moved into if (keep_old), and drop "&& keep_old"
> from conditions. This also will allow to drop extra calculations, move
> new variables to if (keep_old) {} block and allow to pass
> l2_slice=NULL together with keep_old=false.

In subsequent patches we're going to have more cases than just
QCOW2_CLUSTER_NORMAL so I don't think it makes sense to move the
keep_old check around.

>> @@ -1239,7 +1304,8 @@ static int handle_copied(BlockDriverState *bs, 
>> uint64_t guest_offset,
>>   
>>       l2_index = offset_to_l2_slice_index(s, guest_offset);
>>       nb_clusters = MIN(nb_clusters, s->l2_slice_size - l2_index);
>> -    assert(nb_clusters <= INT_MAX);
>> +    /* Limit total byte count to BDRV_REQUEST_MAX_BYTES */
>> +    nb_clusters = MIN(nb_clusters, BDRV_REQUEST_MAX_BYTES >> 
>> s->cluster_bits);
>>   
>>       /* Find L2 entry for the first involved cluster */
>>       ret = get_cluster_table(bs, guest_offset, &l2_slice, &l2_index);
>> @@ -1249,18 +1315,17 @@ static int handle_copied(BlockDriverState *bs, 
>> uint64_t guest_offset,
>>   
>>       cluster_offset = be64_to_cpu(l2_slice[l2_index]);
>
> It would be good to s/cluster_offset/l2_entry/
>
> And, "cluster_offset & L2E_OFFSET_MASK" is used so many times, so, I'd
> not substitute, but keep both variables: l2_entry and cluster_offset.

Sounds good, I can change that.

>> +    /* Allocate at a given offset in the image file */
>> +    alloc_cluster_offset = *host_offset == INV_OFFSET ? INV_OFFSET :
>> +        start_of_cluster(s, *host_offset);
>> +    ret = do_alloc_cluster_offset(bs, guest_offset, &alloc_cluster_offset,
>> +                                  &nb_clusters);
>> +    if (ret < 0) {
>> +        goto out;
>>       }
>>   
>> -    qcow2_cache_put(s->l2_table_cache, (void **) &l2_slice);
>
> actually we don't need l2_slice for keep_old=false in
> calculate_l2_meta, so if calculate_l2_meta modified a bit, change of
> function tail is not needed..
>
> Still, may be l2_slice will be used in calculate_l2_meta() in further
> patches? Will see..

We'll need it in a later patch.

>> -fail:
>> -    if (*m && (*m)->nb_clusters > 0) {
>> +out:
>> +    qcow2_cache_put(s->l2_table_cache, (void **) &l2_slice);
>> +    if (ret < 0 && *m && (*m)->nb_clusters > 0) {
>>           QLIST_REMOVE(*m, next_in_flight);
>>       }
>
> Hmm, unrelated to the patch, but why do we remove meta, which we
> didn't create?

Not sure actually, I would need to check further...

Berto