| Message ID | 22820af5867124e4fdf3d0cb74b99f31edc1b37f.1572125022.git.berto@igalia.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add subcluster allocation to qcow2 | expand |
27.10.2019 0:25, 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> > --- > block/qcow2-cluster.c | 76 +++++++++++++++++++++++++++++-------------- > 1 file changed, 52 insertions(+), 24 deletions(-) > > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c > index 8982b7b762..6c1dcdc781 100644 > --- a/block/qcow2-cluster.c > +++ b/block/qcow2-cluster.c > @@ -1019,6 +1019,55 @@ void qcow2_alloc_cluster_abort(BlockDriverState *bs, QCowL2Meta *m) > QCOW2_DISCARD_NEVER); > } > > +/* > + * For a given write request, create a new QCowL2Meta structure and > + * add it to @m. > + * > + * @host_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_offset, > + uint64_t guest_offset, uint64_t 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); New code is easier to read! Interesting, seems QEMU_ALIGN_UP is more popular.. But comment above ROUND_UP makes sense in using it here.. > + 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_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, > @@ -1417,35 +1466,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, hmm this logic is changed.. after the patch, it would be not nb_bytes, but offset_into_cluster(s, guest_offset) + MIN(*bytes, nb_bytes - offset_into_cluster(s, guest_offset)) > - .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 26.10.19 23:25, 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> > --- > block/qcow2-cluster.c | 76 +++++++++++++++++++++++++++++-------------- > 1 file changed, 52 insertions(+), 24 deletions(-) > > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c > index 8982b7b762..6c1dcdc781 100644 > --- a/block/qcow2-cluster.c > +++ b/block/qcow2-cluster.c > @@ -1019,6 +1019,55 @@ void qcow2_alloc_cluster_abort(BlockDriverState *bs, QCowL2Meta *m) > QCOW2_DISCARD_NEVER); > } > > +/* > + * For a given write request, create a new QCowL2Meta structure and > + * add it to @m. > + * > + * @host_offset points to the beginning of the first cluster. (I intended not to comment on such things on an RFC, but here I am...) I’d call it host_cluster_offset to make clear that it points to a cluster and isn’t the host offset for @guest_offset. And now that I’ve gone this far already, I might as well say that I’d like if it the comment noted that this function not only creates the L2Meta structure but also adds it to the cluster_allocs list. > + * @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_offset, > + uint64_t guest_offset, uint64_t bytes, And now I’m so deep into non-RFC-level review territory, I might as well say I’d prefer @bytes to be an unsigned (or maybe even a plain int), because anything more wouldn’t work. (Not least because cow_end_to is an unsigned). Sorry... Max > + 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_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, > @@ -1417,35 +1466,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 Mon 28 Oct 2019 01:50:54 PM CET, Vladimir Sementsov-Ogievskiy wrote: >> - .cow_end = { >> - .offset = nb_bytes, > > hmm this logic is changed.. after the patch, it would be not nb_bytes, but > > offset_into_cluster(s, guest_offset) + MIN(*bytes, nb_bytes - offset_into_cluster(s, guest_offset)) It hasn't changed. The value of .cow_end.offset before the patch is nb_bytes (these two lines are equivalent): nb_bytes = MIN(requested_bytes, avail_bytes); nb_bytes = MIN(*bytes + offset_into_cluster, avail_bytes); After the patch we update *bytes to pass it to calculate_l2_meta(). Here is the value (again, these are all equivalent): *bytes = MIN(*bytes, nb_bytes - offset_into_cluster) *bytes = MIN(*bytes, MIN(*bytes + offset_into_cluster, avail_bytes) - offset_into_cluster) *bytes = MIN(*bytes, MIN(*bytes, avail_bytes - offset_into_cluster)) *bytes = MIN(*bytes, avail_bytes - offset_into_cluster) And here's the value of .cow_end.offset set in calculate_l2_meta(): cow_end_from = *bytes + offset_into_cluster cow_end_from = MIN(*bytes, avail_bytes - offset_into_cluster) + offset_into_cluster cow_end_from = MIN(*bytes + offset_into_cluster, avail_bytes) This last definition of cow_end_from is the same as nb_bytes as used before the patch. Berto
On Wed 30 Oct 2019 01:04:02 PM CET, Max Reitz wrote: > (I intended not to comment on such things on an RFC, but here I am...) No problem with that :-) > I’d call it host_cluster_offset to make clear that it points to a > cluster and isn’t the host offset for @guest_offset. Sure, why not. We can also accept the exact offset within the cluster and then use start_of_cluster(), but I prefer this one. > And now that I’ve gone this far already, I might as well say that I’d > like if it the comment noted that this function not only creates the > L2Meta structure but also adds it to the cluster_allocs list. I'll update the comment. >> + * @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_offset, >> + uint64_t guest_offset, uint64_t bytes, > > And now I’m so deep into non-RFC-level review territory, I might as > well say I’d prefer @bytes to be an unsigned (or maybe even a plain > int), because anything more wouldn’t work. (Not least because > cow_end_to is an unsigned). True, I'll correct that too. Thanks! Berto
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index 8982b7b762..6c1dcdc781 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -1019,6 +1019,55 @@ void qcow2_alloc_cluster_abort(BlockDriverState *bs, QCowL2Meta *m) QCOW2_DISCARD_NEVER); } +/* + * For a given write request, create a new QCowL2Meta structure and + * add it to @m. + * + * @host_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_offset, + uint64_t guest_offset, uint64_t 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_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, @@ -1417,35 +1466,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:
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> --- block/qcow2-cluster.c | 76 +++++++++++++++++++++++++++++-------------- 1 file changed, 52 insertions(+), 24 deletions(-)