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

[Alberto Garcia]

On Wed 07 Oct 2020 05:47:32 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
> 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.

No, cow_end.offset must not be 0 but the offset where the data region
ends, this is already enforced by this assertion in perform_cow():

    assert(start->offset + start->nb_bytes <= end->offset);

And it is explicitly mentioned in the commit message:

    Even when those regions themselves are empty their offsets must be
    correct because they are used to know the location of the middle
    region.

and also in qcow2.h:

  /**
   * The COW Region immediately after the area the guest actually
   * writes to. This (part of the) write request ends at cow_end.offset
   * (which must always be set even when cow_end.nb_bytes is 0).
   */
  Qcow2COWRegion cow_end;

Berto