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