Re: [PATCH v4 17/30] qcow2: Add subcluster support to calculate_l2_meta()

[Alberto Garcia]

On Wed 15 Apr 2020 10:39:26 AM CEST, Vladimir Sementsov-Ogievskiy wrote:
>> + * Returns 1 on success, -errno on failure (in order to match the
>> + * return value of handle_copied() and handle_alloc()).
>
> Hmm, honestly, I don't like this idea. handle_copied and handle_alloc
> has special return code semantics. Here no reason for special
> semantics, just classic error/success.

Right, the only reason is to avoid adding something like this after all
callers:

        if (ret == 0) {
            ret = 1;
        }

But you have a point, maybe I change it after all.

>> +        case QCOW2_SUBCLUSTER_NORMAL:
>> +        case QCOW2_SUBCLUSTER_COMPRESSED:
>> +        case QCOW2_SUBCLUSTER_ZERO_ALLOC:
>> +        case QCOW2_SUBCLUSTER_UNALLOCATED_ALLOC:
>> +            cow_end_to = ROUND_UP(cow_end_from, s->cluster_size);
>
> Hmm. Interesting, actually, we don't need to COW
> QCOW2_SUBCLUSTER_UNALLOCATED_ALLOC subclusters in cow-area.. But this
> need more modifications to cow-handling.

True, if there are more unallocated subclusters in the cow area we could
make the copy operation smaller. I'm not sure if it's worth adding extra
code for this, but maybe I can leave a comment.

>> +            break;
>> +        case QCOW2_SUBCLUSTER_ZERO_PLAIN:
>> +        case QCOW2_SUBCLUSTER_UNALLOCATED_PLAIN:
>> +            cow_end_to = ROUND_UP(cow_end_from, s->subcluster_size);
>
>
> This is because in new cluster we can made previous subclusters
> unallocated, and don't copy from backing.
> Hmm, actually, we should not just make them unallocated, but copy part
> of bitmap from original l2-entry.. I need to keep it in mind for next
> patches.

The bitmap is always copied from the original L2 entry, you can see it
in the patch "qcow2: Update L2 bitmap in qcow2_alloc_cluster_link_l2()"

Berto