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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

07.10.2020 18:38, Alberto Garcia wrote:
> 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.

but cow_end.offset will not be zero still? I suggest it because you actually rely on this fact by dropping written_to conditional assignment.

> 
>>> -        /* 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 ------>
> 

In this picture it still seems possible to merge "write-request" to "1st QCowL2Meta",
keeping "2nd QCowL2Meta" in separate.. Or, in this case we should create additional
relation between these metas? OK, it's not so simple, thanks for the picture.

> 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
> 


-- 
Best regards,
Vladimir