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

[Vladimir Sementsov-Ogievskiy]

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