On 27.04.2017 03:46, Eric Blake wrote: > From: Max Reitz <mre...@redhat.com> > > Instead of just freeing preallocated zero clusters and completely > allocating them from scratch, reuse them. > > We cannot do this in handle_copied(), however, since this is a COW > operation. Therefore, we have to add the new logic to handle_alloc() and > simply return the existing offset if it exists. The only catch is that > we have to convince qcow2_alloc_cluster_link_l2() not to free the old > clusters (because we have reused them). > > Reported-by: Eric Blake <ebl...@redhat.com> > Signed-off-by: Max Reitz <mre...@redhat.com> > Signed-off-by: Eric Blake <ebl...@redhat.com> > > --- > v10: new patch. Max hasn't posted the patch directly on list, but > did mention it here: > https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg03936.html > and that he would post it once he has tests. Well, my later patches > add a test that requires this one :) The other two patches that > he mentioned there are also good, but not essential for my series > (and I didn't want to write tests to expose the behavior difference, > because it would deprive Max of that fun).
Well, the main reason I didn't send the patches yet is because I was
tired while writing them ("Date: Sat Apr 22 01:17:52 2017 +0200") and I
wanted to take another look before sending them. I guess now's as good a
time as any.
> ---
> block/qcow2.h | 3 ++
> block/qcow2-cluster.c | 83
> +++++++++++++++++++++++++++++++++++----------------
> 2 files changed, 60 insertions(+), 26 deletions(-)
[...]
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index d1063df..db3d937 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
[...]
> @@ -1192,31 +1199,53 @@ static int handle_alloc(BlockDriverState *bs,
> uint64_t guest_offset,
> * wrong with our code. */
> assert(nb_clusters > 0);
>
> + if (!*host_offset && qcow2_get_cluster_type(entry) == QCOW2_CLUSTER_ZERO
> &&
> + (entry & L2E_OFFSET_MASK) != 0 && (entry & QCOW_OFLAG_COPIED))
*host_offset works with this, too, if
start_of_cluster(s, *host_offset) == (entry & L2E_OFFSET_MASK).
If !(entry & QCOW_OFLAG_COPIED), we should check whether the refcount
maybe is 1 and then set OFLAG_COPIED. But that is something we don't
even do for normal clusters yet, so it's something to fix another day.
> + {
> + /* Try to reuse preallocated zero clusters; contiguous normal
> clusters
> + * would be fine, too, but count_cow_clusters() above has limited
> + * nb_clusters already to a range of COW clusters */
> + int preallocated_nb_clusters =
> + count_contiguous_clusters(nb_clusters, s->cluster_size, l2_table,
s/l2_table/&l2_table[l2_index]/
> + QCOW_OFLAG_COPIED);
> +
> + if (preallocated_nb_clusters) {
preallocated_nb_clusters must be at least 1, so an assertion would be
better.
Max
> + nb_clusters = preallocated_nb_clusters;
> + alloc_cluster_offset = entry & L2E_OFFSET_MASK;
> +
> + /* We want to reuse these clusters, so
> qcow2_alloc_cluster_link_l2()
> + * should not free them. */
> + keep_old_clusters = true;
> + }
> + }
> +
> qcow2_cache_put(bs, s->l2_table_cache, (void **) &l2_table);
>
> - /* Allocate, if necessary at a given offset in the image file */
> - alloc_cluster_offset = start_of_cluster(s, *host_offset);
> - ret = do_alloc_cluster_offset(bs, guest_offset, &alloc_cluster_offset,
> - &nb_clusters);
> - if (ret < 0) {
> - goto fail;
> - }
> -
> - /* Can't extend contiguous allocation */
> - if (nb_clusters == 0) {
> - *bytes = 0;
> - return 0;
> - }
> -
> - /* !*host_offset would overwrite the image header and is reserved for "no
> - * host offset preferred". If 0 was a valid host offset, it'd trigger the
> - * following overlap check; do that now to avoid having an invalid value
> in
> - * *host_offset. */
> if (!alloc_cluster_offset) {
> - ret = qcow2_pre_write_overlap_check(bs, 0, alloc_cluster_offset,
> - nb_clusters * s->cluster_size);
> - assert(ret < 0);
> - goto fail;
> + /* Allocate, if necessary at a given offset in the image file */
> + alloc_cluster_offset = start_of_cluster(s, *host_offset);
> + ret = do_alloc_cluster_offset(bs, guest_offset,
> &alloc_cluster_offset,
> + &nb_clusters);
> + if (ret < 0) {
> + goto fail;
> + }
> +
> + /* Can't extend contiguous allocation */
> + if (nb_clusters == 0) {
> + *bytes = 0;
> + return 0;
> + }
> +
> + /* !*host_offset would overwrite the image header and is reserved for
> + * "no host offset preferred". If 0 was a valid host offset, it'd
> + * trigger the following overlap check; do that now to avoid having
> an
> + * invalid value in *host_offset. */
> + if (!alloc_cluster_offset) {
> + ret = qcow2_pre_write_overlap_check(bs, 0, alloc_cluster_offset,
> + nb_clusters *
> s->cluster_size);
> + assert(ret < 0);
> + goto fail;
> + }
> }
>
> /*
> @@ -1247,6 +1276,8 @@ static int handle_alloc(BlockDriverState *bs, uint64_t
> guest_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),
>
signature.asc
Description: OpenPGP digital signature
