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
