Re: [RFC PATCH v2 02/26] qcow2: Split cluster_needs_cow() out of count_cow_clusters()

[Vladimir Sementsov-Ogievskiy]

27.10.2019 0:25, Alberto Garcia wrote:
> We are going to need it in other places.
> 
> Signed-off-by: Alberto Garcia <address@hidden>
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

> ---
>   block/qcow2-cluster.c | 34 +++++++++++++++++++---------------
>   1 file changed, 19 insertions(+), 15 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 6c1dcdc781..aa1010a515 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1068,6 +1068,24 @@ static void calculate_l2_meta(BlockDriverState *bs, 
> uint64_t host_offset,
>       QLIST_INSERT_HEAD(&s->cluster_allocs, *m, next_in_flight);
>   }
>   
> +/* Returns true if writing to a cluster requires COW */
> +static bool cluster_needs_cow(BlockDriverState *bs, uint64_t l2_entry)
> +{
> +    switch (qcow2_get_cluster_type(bs, l2_entry)) {
> +    case QCOW2_CLUSTER_NORMAL:
> +        if (l2_entry & QCOW_OFLAG_COPIED) {
> +            return false;
> +        }
> +    case QCOW2_CLUSTER_UNALLOCATED:
> +    case QCOW2_CLUSTER_COMPRESSED:
> +    case QCOW2_CLUSTER_ZERO_PLAIN:
> +    case QCOW2_CLUSTER_ZERO_ALLOC:
> +        return true;
> +    default:
> +        abort();
> +    }
> +}

Not sure how much is it better than just
return !(qcow2_get_cluster_type(bs, l2_entry) == QCOW2_CLUSTER_NORMAL && 
(l2_entry & QCOW_OFLAG_COPIED));

> +
>   /*
>    * Returns the number of contiguous clusters that can be used for an 
> allocating
>    * write, but require COW to be performed (this includes yet unallocated 
> space,
> @@ -1080,25 +1098,11 @@ static int count_cow_clusters(BlockDriverState *bs, 
> int nb_clusters,
>   
>       for (i = 0; i < nb_clusters; i++) {
>           uint64_t l2_entry = be64_to_cpu(l2_slice[l2_index + i]);
> -        QCow2ClusterType cluster_type = qcow2_get_cluster_type(bs, l2_entry);
> -
> -        switch(cluster_type) {
> -        case QCOW2_CLUSTER_NORMAL:
> -            if (l2_entry & QCOW_OFLAG_COPIED) {
> -                goto out;
> -            }
> +        if (!cluster_needs_cow(bs, l2_entry)) {
>               break;
> -        case QCOW2_CLUSTER_UNALLOCATED:
> -        case QCOW2_CLUSTER_COMPRESSED:
> -        case QCOW2_CLUSTER_ZERO_PLAIN:
> -        case QCOW2_CLUSTER_ZERO_ALLOC:
> -            break;
> -        default:
> -            abort();
>           }
>       }
>   
> -out:
>       assert(i <= nb_clusters);
>       return i;
>   }
> 


-- 
Best regards,
Vladimir