[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [RFC PATCH v2 13/26] qcow2: Add subcluster support to calculate_l2_m

From: Alberto Garcia
Subject: Re: [RFC PATCH v2 13/26] qcow2: Add subcluster support to calculate_l2_meta()
Date: Fri, 08 Nov 2019 16:18:13 +0100
User-agent: Notmuch/0.18.2 (http://notmuchmail.org) Emacs/24.4.1 (i586-pc-linux-gnu)

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


reply via email to

[Prev in Thread] Current Thread [Next in Thread]