Re: [RFC PATCH v2 13/26] qcow2: Add subcluster support to calculate_l2_meta()

[Max Reitz]

On 26.10.19 23:25, Alberto Garcia wrote:
> If an image has subclusters then there are more copy-on-write
> scenarios that we need to consider. Let's say we have a write request
> from the middle of subcluster #3 until the end of the cluster:
> 
>    - If the cluster is new, then subclusters #0 to #3 from the old
>      cluster must be copied into the new one.

You mean for snapshots?

(That isn’t quite clear, and I only guess this based on the next bullet
point which differentiates based on “the old cluster was unallocated”.
That’s weird, too, because what does that mean, old cluster and new
cluster?  I suppose it’s abstract and it just means “There was no old
cluster and now we’ve allocated something”.  I can only understand the
concept of old and new clusters for COW inside of an image, i.e. for
snapshots and compressed clusters (theoretically).)

>    - If the cluster is new but the old cluster was unallocated, then
>      only subcluster #3 needs copy-on-write. #0 to #2 are marked as
>      unallocated in the bitmap of the new L2 entry.
> 
>    - If we are overwriting an old cluster and subcluster #3 is
>      unallocated or has the all-zeroes bit set then we need
>      copy-on-write on subcluster #3.
> 
>    - If we are overwriting an old cluster and subcluster #3 was
>      allocated then there is no need to copy-on-write.
> 
> Signed-off-by: Alberto Garcia <address@hidden>
> ---
>  block/qcow2-cluster.c | 136 +++++++++++++++++++++++++++++++++---------
>  1 file changed, 108 insertions(+), 28 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 1f509bda15..990bc070af 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1034,14 +1034,16 @@ void qcow2_alloc_cluster_abort(BlockDriverState *bs, 
> QCowL2Meta *m)
>   * If @keep_old is true it means that the clusters were already
>   * allocated and will be overwritten. If false then the clusters are
>   * new and we have to decrease the reference count of the old ones.
> + *
> + * Returns 1 on success, -errno on failure.

I think there should be a note here on why this doesn’t follow the
general 0/-errno schema (i.e., “, because that is what callers generally
expect”).

>   */

[...]

> +    if (!keep_old) {
> +        switch (type) {
> +        case QCOW2_CLUSTER_NORMAL:
> +        case QCOW2_CLUSTER_COMPRESSED:
> +        case QCOW2_CLUSTER_ZERO_ALLOC:
> +        case QCOW2_CLUSTER_UNALLOCATED_SUBCLUSTER:
> +            cow_start_from = 0;

Somehow (I don’t know why) I find this a bit tough to understand.

Wouldn’t it work to let cow_start start from the first subcluster for
ZERO_ALLOC and UNALLOCATED_SUBCLUSTER?  We don’t need to COW those, it
should be sufficient to just make the subclusters before that zero or
unallocated, respectively.

(Same for cow_end)

Max

> +            break;
> +        case QCOW2_CLUSTER_ZERO_PLAIN:
> +        case QCOW2_CLUSTER_UNALLOCATED:
> +            cow_start_from = sc_index << s->subcluster_bits;
> +            break;
> +        default:
> +            g_assert_not_reached();
> +        }

signature.asc
Description: OpenPGP digital signature