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

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

07.10.2020 14:52, Alberto Garcia wrote:
> The QCowL2Meta structure is used to store information about a part of
> a write request that touches clusters that need changes in their L2
> entries. This happens with newly-allocated clusters or subclusters.
> 
> This structure has changed a bit since it was first created and its
> current documentation is not quite up-to-date.
> 
> A write request can span a region consisting on a combination of
> clusters of different types, and qcow2_alloc_host_offset() can
> repeatedly call handle_copied() and handle_alloc() to add more
> clusters to the mix as long as they all are contiguous on the image
> file.
> 
> Because of this a write request has a list of QCowL2Meta structures,
> one for each part of the request that needs changes in the L2
> metadata.
> 
> Each one of them spans nb_clusters and has two copy-on-write regions
> located immediately before and after the middle region that that part
> of the write request touches. Even when those regions themselves are
> empty their offsets must be correct because they are used to know the
> location of the middle region.
> 
> This was not always the case but it is not a problem anymore
> because the only two places where QCowL2Meta structures are created
> (calculate_l2_meta() and qcow2_co_truncate()) ensure that the
> copy-on-write regions are correctly defined, and so do assertions like
> the ones in perform_cow().
> 
> The conditional initialization of the 'written_to' variable is
> therefore unnecessary and is removed by this patch.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>   block/qcow2.h         | 19 +++++++++++--------
>   block/qcow2-cluster.c |  5 +++--
>   block/qcow2.c         | 19 +++++++++++++++----
>   3 files changed, 29 insertions(+), 14 deletions(-)
> 
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 125ea9679b..2e0272a7b8 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -435,17 +435,18 @@ typedef struct Qcow2COWRegion {
>   
>   /**
>    * Describes an in-flight (part of a) write request that writes to clusters
> - * that are not referenced in their L2 table yet.
> + * that need to have their L2 table entries updated (because they are
> + * newly allocated or need changes in their L2 bitmaps)
>    */
>   typedef struct QCowL2Meta
>   {
> -    /** Guest offset of the first newly allocated cluster */
> +    /** Guest offset of the first updated cluster */
>       uint64_t offset;
>   
> -    /** Host offset of the first newly allocated cluster */
> +    /** Host offset of the first updated cluster */
>       uint64_t alloc_offset;
>   
> -    /** Number of newly allocated clusters */
> +    /** Number of updated clusters */
>       int nb_clusters;
>   
>       /** Do not free the old clusters */
> @@ -458,14 +459,16 @@ typedef struct QCowL2Meta
>       CoQueue dependent_requests;
>   
>       /**
> -     * 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

>        */
>       Qcow2COWRegion cow_start;
>   
>       /**
> -     * The COW Region between the area the guest actually writes to and the
> -     * end of the last allocated cluster.
> +     * The COW Region immediately after the area the guest actually
> +     * writes to. This (part of the) write request ends at cow_end.offset

same here

> +     * (which must always be set even when cow_end.nb_bytes is 0).
>        */
>       Qcow2COWRegion cow_end;
>   
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index aa87d3e99b..485b4cb92e 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -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?

>       for (i = 0; i < m->nb_clusters; i++) {
>           uint64_t offset = cluster_offset + ((uint64_t)i << s->cluster_bits);
>           /* if two concurrent writes happen to the same unallocated cluster
> @@ -1070,8 +1072,7 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m)
>           if (has_subclusters(s) && !m->prealloc) {
>               uint64_t l2_bitmap = get_l2_bitmap(s, l2_slice, l2_index + i);
>               unsigned written_from = m->cow_start.offset;
> -            unsigned written_to = m->cow_end.offset + m->cow_end.nb_bytes ?:
> -                m->nb_clusters << s->cluster_bits;
> +            unsigned written_to = m->cow_end.offset + m->cow_end.nb_bytes;
>               int first_sc, last_sc;
>               /* Narrow written_from and written_to down to the current cluster */
>               written_from = MAX(written_from, i << s->cluster_bits);
> diff --git a/block/qcow2.c b/block/qcow2.c
> index b05512718c..e7b27fdf25 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2361,15 +2361,26 @@ static bool merge_cow(uint64_t offset, unsigned bytes,
>               continue;
>           }
>   
> -        /* The data (middle) region must be immediately after the
> -         * start region */
> +        /*
> +         * The write request should start immediately after the first
> +         * COW region. This does not always happen because the area
> +         * touched by the request can be larger than the one defined
> +         * by @m (a single request can span an area consisting of a
> +         * mix of previously unallocated and allocated clusters, that
> +         * is why @l2meta is a list).
> +         */
>           if (l2meta_cow_start(m) + m->cow_start.nb_bytes != offset) {
> +            /* In this case the request starts before this region */
> +            assert(offset < l2meta_cow_start(m));
> +            assert(m->cow_start.nb_bytes == 0);
>               continue;
>           }
>   
> -        /* 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?

So, maybe we can write instead of these two ifs:

If (!m->cow_start.nb_bytes && !m->cow_end.nb_bytes) {
    /* No cow regions */
    continue;
}

if (m->cow_start.nb_bytes) {
   assert(l2meta_cow_start(m) + m->cow_start.nb_bytes == offset)
}

if (m->cow_end.nb_bytes) {
   assert(m->offset + m->cow_end.offset != offset + bytes);
}

But it may be done in separate.

I'm not really good in l2meta list related things, but patch seems OK to me:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir