Re: [PATCH] qcow2: Document and enforce the QCowL2Meta invariants

[Alberto Garcia]

On Wed 07 Oct 2020 04:42:37 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
>>       /**
>> -     * The COW Region between the start of the first allocated cluster and 
>> the
>> -     * area the guest actually writes to.
>> +     * The COW Region immediately before the area the guest actually
>> +     * writes to. This (part of the) write request starts at
>> +     * cow_start.offset + cow_start.nb_bytes.
>
> "starts at" is a bit misleading.. As here is not guest offset, not
> host offset, but offset relative to QCowL2Meta.offset

I thought it was clear by the context since Qcow2COWRegion.offset is
defined to be relative to QCowL2Meta.offset

>> @@ -1049,6 +1049,8 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, 
>> QCowL2Meta *m)
>>       qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_slice);
>>   
>>       assert(l2_index + m->nb_clusters <= s->l2_slice_size);
>> +    assert(m->cow_end.offset + m->cow_end.nb_bytes <=
>> +           m->nb_clusters << s->cluster_bits);
>
> should we also assert that cow_end.offset + cow_end.nb_bytes is not
> zero?

No, a write request that fills a complete cluster has no COW but still
needs to call qcow2_alloc_cluster_link_l2() to update the L2 metadata.

>> -        /* The end region must be immediately after the data (middle)
>> -         * region */
>> +        /* The write request should end immediately before the second
>> +         * COW region (see above for why it does not always happen) */
>>           if (m->offset + m->cow_end.offset != offset + bytes) {
>> +            assert(offset + bytes > m->offset + m->cow_end.offset);
>> +            assert(m->cow_end.nb_bytes == 0);
>>               continue;
>>           }
>
> Then it's interesting, why not to merge if we have this zero-length
> cow region? What's the problem with it?

We do merge the request and the COW if one of the COW regions has zero
length, visually:

 1)
    <>                      <--- cow_end --->
    <--- write request ---->

 2)
    <--- cow_start --->                      <>
                       <--- write request ---->

In this case however the problem is not that the region is empty, but
that the request starts *before* (or after) that region and that there's
probably another QCowL2Meta earlier (or later):

    <----  1st QCowL2Meta  ---->         <---- 2nd QCowL2Meta ---->
    <--- cow_start --->       <>         <>       <--- cow_end --->
                       <---- write request ------>

What we would need here is to merge all QCowL2Meta into one, but I don't
think that we want to do that because it looks like complicating the
code for a scenario that does not happen very often.

Berto