On Mon 04 Nov 2019 03:21:41 PM CET, Max Reitz 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?
Yes, perhaps the terminology is a bit unclear.
When I say "new cluster" is this context I mean that a write request
requires that a new cluster is allocated in the qcow2 file.
Then the "old cluster" would be what was there before the write (i.e. a
cluster with refcount > 1 or an unallocated cluster). Where we are doing
the copy-on-write from.
>> * 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”).
Good idea.
>> + 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.
Here's one good example why I should probably add a QCow2SubclusterType
different from the existing QCow2ClusterType.
In this context, 'type' is the type of the subcluster, and because of
that _ZERO_ALLOC means that the subcluster reads as zeros but the
cluster itself is allocated. Other subcluster may contain data and
that's why we have to copy all of them.
Berto
