| Message ID | 20201007161323.4667-1-berto@igalia.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] qcow2: Document and enforce the QCowL2Meta invariants | expand |
ping On Wed 07 Oct 2020 06:13:23 PM CEST, 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 of 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 touched by that > part of the write request. 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> > Reviewed-by: Eric Blake <eblake@redhat.com> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
On 07.10.20 18:13, 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 of 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 touched by that > part of the write request. 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> > Reviewed-by: Eric Blake <eblake@redhat.com> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/qcow2.h | 19 +++++++++++-------- > block/qcow2-cluster.c | 5 +++-- > block/qcow2.c | 19 +++++++++++++++---- > 3 files changed, 29 insertions(+), 14 deletions(-) Thanks, applied to my block branch: https://git.xanclic.moe/XanClic/qemu/commits/branch/block Max
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. */ 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 + * (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); 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; }