Re: [Qemu-devel] [PATCH v10 03/17] qcow2: Reuse preallocated zero clusters

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5942 bytes --]

On 27.04.2017 03:46, Eric Blake wrote:
> From: Max Reitz <mreitz@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 <eblake@redhat.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Eric Blake <eblake@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),
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]