| Message ID | 627da7ad090c0b166f3d0294312d956fcddc5a2a.1584468723.git.berto@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add subcluster allocation to qcow2 | expand |
17.03.2020 21:16, Alberto Garcia wrote: > handle_alloc() creates a QCowL2Meta structure in order to update the > image metadata and perform the necessary copy-on-write operations. > > This patch moves that code to a separate function so it can be used > from other places. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > Reviewed-by: Max Reitz <mreitz@redhat.com> > --- > block/qcow2-cluster.c | 77 +++++++++++++++++++++++++++++-------------- > 1 file changed, 53 insertions(+), 24 deletions(-) > > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c > index 95f04d12cc..802fc599a5 100644 > --- a/block/qcow2-cluster.c > +++ b/block/qcow2-cluster.c > @@ -1039,6 +1039,56 @@ void qcow2_alloc_cluster_abort(BlockDriverState *bs, QCowL2Meta *m) > } > } > > +/* > + * For a given write request, create a new QCowL2Meta structure, add > + * it to @m and the BDRVQcow2State.cluster_allocs list. > + * > + * @host_cluster_offset points to the beginning of the first cluster. > + * > + * @guest_offset and @bytes indicate the offset and length of the > + * request. > + * > + * If @keep_old is true it means that the clusters were already > + * allocated and will be overwritten. If false then the clusters are > + * new and we have to decrease the reference count of the old ones. > + */ > +static void calculate_l2_meta(BlockDriverState *bs, > + uint64_t host_cluster_offset, > + uint64_t guest_offset, unsigned bytes, > + QCowL2Meta **m, bool keep_old) > +{ > + BDRVQcow2State *s = bs->opaque; > + unsigned cow_start_from = 0; > + unsigned cow_start_to = offset_into_cluster(s, guest_offset); > + unsigned cow_end_from = cow_start_to + bytes; > + unsigned cow_end_to = ROUND_UP(cow_end_from, s->cluster_size); > + unsigned nb_clusters = size_to_clusters(s, cow_end_from); > + QCowL2Meta *old_m = *m; > + > + *m = g_malloc0(sizeof(**m)); > + **m = (QCowL2Meta) { > + .next = old_m, > + > + .alloc_offset = host_cluster_offset, > + .offset = start_of_cluster(s, guest_offset), > + .nb_clusters = nb_clusters, > + > + .keep_old_clusters = keep_old, > + > + .cow_start = { > + .offset = cow_start_from, > + .nb_bytes = cow_start_to - cow_start_from, > + }, > + .cow_end = { > + .offset = cow_end_from, Hmm. So, you make it equal to requested_bytes from handle_alloc(). But before your change it was MIN(requested_bytes, avail_bytes).. If avail_bytes can be less than requested_bytes the patch breaks it, if not, we'd better drop this MIN. > + .nb_bytes = cow_end_to - cow_end_from, > + }, > + }; > + > + qemu_co_queue_init(&(*m)->dependent_requests); > + QLIST_INSERT_HEAD(&s->cluster_allocs, *m, next_in_flight); > +} > + > /* > * Returns the number of contiguous clusters that can be used for an allocating > * write, but require COW to be performed (this includes yet unallocated space, > @@ -1437,35 +1487,14 @@ static int handle_alloc(BlockDriverState *bs, uint64_t guest_offset, > uint64_t requested_bytes = *bytes + offset_into_cluster(s, guest_offset); > int avail_bytes = nb_clusters << s->cluster_bits; > int nb_bytes = MIN(requested_bytes, avail_bytes); > - QCowL2Meta *old_m = *m; > - > - *m = g_malloc0(sizeof(**m)); > - > - **m = (QCowL2Meta) { > - .next = old_m, > - > - .alloc_offset = alloc_cluster_offset, > - .offset = start_of_cluster(s, guest_offset), > - .nb_clusters = nb_clusters, > - > - .keep_old_clusters = keep_old_clusters, > - > - .cow_start = { > - .offset = 0, > - .nb_bytes = offset_into_cluster(s, guest_offset), > - }, > - .cow_end = { > - .offset = nb_bytes, > - .nb_bytes = avail_bytes - nb_bytes, > - }, > - }; > - qemu_co_queue_init(&(*m)->dependent_requests); > - QLIST_INSERT_HEAD(&s->cluster_allocs, *m, next_in_flight); > > *host_offset = alloc_cluster_offset + offset_into_cluster(s, guest_offset); > *bytes = MIN(*bytes, nb_bytes - offset_into_cluster(s, guest_offset)); > assert(*bytes != 0); > > + calculate_l2_meta(bs, alloc_cluster_offset, guest_offset, *bytes, > + m, keep_old_clusters); > + > return 1; > > fail: >
On Thu 09 Apr 2020 10:30:13 AM CEST, Vladimir Sementsov-Ogievskiy wrote: >> +static void calculate_l2_meta(BlockDriverState *bs, >> + uint64_t host_cluster_offset, >> + uint64_t guest_offset, unsigned bytes, >> + QCowL2Meta **m, bool keep_old) >> +{ >> + BDRVQcow2State *s = bs->opaque; >> + unsigned cow_start_from = 0; >> + unsigned cow_start_to = offset_into_cluster(s, guest_offset); >> + unsigned cow_end_from = cow_start_to + bytes; >> + unsigned cow_end_to = ROUND_UP(cow_end_from, s->cluster_size); >> + unsigned nb_clusters = size_to_clusters(s, cow_end_from); >> + QCowL2Meta *old_m = *m; >> + >> + *m = g_malloc0(sizeof(**m)); >> + **m = (QCowL2Meta) { >> + .next = old_m, >> + >> + .alloc_offset = host_cluster_offset, >> + .offset = start_of_cluster(s, guest_offset), >> + .nb_clusters = nb_clusters, >> + >> + .keep_old_clusters = keep_old, >> + >> + .cow_start = { >> + .offset = cow_start_from, >> + .nb_bytes = cow_start_to - cow_start_from, >> + }, >> + .cow_end = { >> + .offset = cow_end_from, > > Hmm. So, you make it equal to requested_bytes from handle_alloc(). No, requested_bytes from handle_alloc is: requested_bytes = *bytes + offset_into_cluster(s, guest_offset); But *bytes is later modified before calling calculate_l2_meta(): *bytes = MIN(*bytes, nb_bytes - offset_into_cluster(s, guest_offset)); More details here: https://lists.gnu.org/archive/html/qemu-block/2019-10/msg01808.html Berto
09.04.2020 18:12, Alberto Garcia wrote: > On Thu 09 Apr 2020 10:30:13 AM CEST, Vladimir Sementsov-Ogievskiy wrote: >>> +static void calculate_l2_meta(BlockDriverState *bs, >>> + uint64_t host_cluster_offset, >>> + uint64_t guest_offset, unsigned bytes, >>> + QCowL2Meta **m, bool keep_old) >>> +{ >>> + BDRVQcow2State *s = bs->opaque; >>> + unsigned cow_start_from = 0; >>> + unsigned cow_start_to = offset_into_cluster(s, guest_offset); >>> + unsigned cow_end_from = cow_start_to + bytes; >>> + unsigned cow_end_to = ROUND_UP(cow_end_from, s->cluster_size); >>> + unsigned nb_clusters = size_to_clusters(s, cow_end_from); >>> + QCowL2Meta *old_m = *m; >>> + >>> + *m = g_malloc0(sizeof(**m)); >>> + **m = (QCowL2Meta) { >>> + .next = old_m, >>> + >>> + .alloc_offset = host_cluster_offset, >>> + .offset = start_of_cluster(s, guest_offset), >>> + .nb_clusters = nb_clusters, >>> + >>> + .keep_old_clusters = keep_old, >>> + >>> + .cow_start = { >>> + .offset = cow_start_from, >>> + .nb_bytes = cow_start_to - cow_start_from, >>> + }, >>> + .cow_end = { >>> + .offset = cow_end_from, >> >> Hmm. So, you make it equal to requested_bytes from handle_alloc(). > > No, requested_bytes from handle_alloc is: > > requested_bytes = *bytes + offset_into_cluster(s, guest_offset); > > But *bytes is later modified before calling calculate_l2_meta(): > > *bytes = MIN(*bytes, nb_bytes - offset_into_cluster(s, guest_offset)); > > More details here: > > https://lists.gnu.org/archive/html/qemu-block/2019-10/msg01808.html > Ahah, me again, sorry :)
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index 95f04d12cc..802fc599a5 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -1039,6 +1039,56 @@ void qcow2_alloc_cluster_abort(BlockDriverState *bs, QCowL2Meta *m) } } +/* + * For a given write request, create a new QCowL2Meta structure, add + * it to @m and the BDRVQcow2State.cluster_allocs list. + * + * @host_cluster_offset points to the beginning of the first cluster. + * + * @guest_offset and @bytes indicate the offset and length of the + * request. + * + * If @keep_old is true it means that the clusters were already + * allocated and will be overwritten. If false then the clusters are + * new and we have to decrease the reference count of the old ones. + */ +static void calculate_l2_meta(BlockDriverState *bs, + uint64_t host_cluster_offset, + uint64_t guest_offset, unsigned bytes, + QCowL2Meta **m, bool keep_old) +{ + BDRVQcow2State *s = bs->opaque; + unsigned cow_start_from = 0; + unsigned cow_start_to = offset_into_cluster(s, guest_offset); + unsigned cow_end_from = cow_start_to + bytes; + unsigned cow_end_to = ROUND_UP(cow_end_from, s->cluster_size); + unsigned nb_clusters = size_to_clusters(s, cow_end_from); + QCowL2Meta *old_m = *m; + + *m = g_malloc0(sizeof(**m)); + **m = (QCowL2Meta) { + .next = old_m, + + .alloc_offset = host_cluster_offset, + .offset = start_of_cluster(s, guest_offset), + .nb_clusters = nb_clusters, + + .keep_old_clusters = keep_old, + + .cow_start = { + .offset = cow_start_from, + .nb_bytes = cow_start_to - cow_start_from, + }, + .cow_end = { + .offset = cow_end_from, + .nb_bytes = cow_end_to - cow_end_from, + }, + }; + + qemu_co_queue_init(&(*m)->dependent_requests); + QLIST_INSERT_HEAD(&s->cluster_allocs, *m, next_in_flight); +} + /* * Returns the number of contiguous clusters that can be used for an allocating * write, but require COW to be performed (this includes yet unallocated space, @@ -1437,35 +1487,14 @@ static int handle_alloc(BlockDriverState *bs, uint64_t guest_offset, uint64_t requested_bytes = *bytes + offset_into_cluster(s, guest_offset); int avail_bytes = nb_clusters << s->cluster_bits; int nb_bytes = MIN(requested_bytes, avail_bytes); - QCowL2Meta *old_m = *m; - - *m = g_malloc0(sizeof(**m)); - - **m = (QCowL2Meta) { - .next = old_m, - - .alloc_offset = alloc_cluster_offset, - .offset = start_of_cluster(s, guest_offset), - .nb_clusters = nb_clusters, - - .keep_old_clusters = keep_old_clusters, - - .cow_start = { - .offset = 0, - .nb_bytes = offset_into_cluster(s, guest_offset), - }, - .cow_end = { - .offset = nb_bytes, - .nb_bytes = avail_bytes - nb_bytes, - }, - }; - qemu_co_queue_init(&(*m)->dependent_requests); - QLIST_INSERT_HEAD(&s->cluster_allocs, *m, next_in_flight); *host_offset = alloc_cluster_offset + offset_into_cluster(s, guest_offset); *bytes = MIN(*bytes, nb_bytes - offset_into_cluster(s, guest_offset)); assert(*bytes != 0); + calculate_l2_meta(bs, alloc_cluster_offset, guest_offset, *bytes, + m, keep_old_clusters); + return 1; fail: