qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [Qemu-devel] [PATCH v2 25/32] qcow2: Update expand_zero


From: Eric Blake
Subject: Re: [Qemu-block] [Qemu-devel] [PATCH v2 25/32] qcow2: Update expand_zero_clusters_in_l1() to support L2 slices
Date: Tue, 16 Jan 2018 17:09:29 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2

On 12/15/2017 06:53 AM, Alberto Garcia wrote:
> expand_zero_clusters_in_l1() expands zero clusters as a necessary step
> to downgrade qcow2 images to a version that doesn't support metadata
> zero clusters. This function takes an L1 table (which may or may not
> be active) and iterates over all its L2 tables looking for zero
> clusters.
> 
> Since we'll be loading L2 slices instead of full tables we need to add
> an extra loop that iterates over all slices of each L2 table, and we
> should also use the slice size when allocating the buffer used when
> the L1 table is not active.
> 
> As a consequence of the new loop the refcount data also needs to be
> loaded before the L2 data, but this is a trivial change with no side
> effects.
> 
> This function doesn't need any additional changes so apart from that
> this patch simply updates the variable name from l2_table to l2_slice.
> 
> Signed-off-by: Alberto Garcia <address@hidden>
> ---
>  block/qcow2-cluster.c | 207 
> +++++++++++++++++++++++++++-----------------------
>  1 file changed, 110 insertions(+), 97 deletions(-)
> 

Even hairier diff than the previous patch. Would it be feasible to split
this into two or three parts - one that just adds an additional {}
scoping (but no variable renames or loop condition changes) (where git
diff -b can easily check the change in isolation); the other that
renames the variable and switches the new scope to be a loop over the
smaller slice limits (possibly as two patches, if the change in refcount
vs. L2 data load ordering deserves separation from the slice conversion)?

> -        } else {
> -            /* load inactive L2 tables from disk */
> -            ret = bdrv_read(bs->file, l2_offset / BDRV_SECTOR_SIZE,
> -                            (void *)l2_table, s->cluster_sectors);

Pre-existing...

> -        }
> -        if (ret < 0) {
> -            goto fail;
> -        }
> -
>          ret = qcow2_get_refcount(bs, l2_offset >> s->cluster_bits,
>                                   &l2_refcount);
>          if (ret < 0) {
>              goto fail;
>          }
>  
> -        for (j = 0; j < s->l2_size; j++) {
> -            uint64_t l2_entry = be64_to_cpu(l2_table[j]);
> -            int64_t offset = l2_entry & L2E_OFFSET_MASK;
> -            QCow2ClusterType cluster_type = qcow2_get_cluster_type(l2_entry);
> -
> -            if (cluster_type != QCOW2_CLUSTER_ZERO_PLAIN &&
> -                cluster_type != QCOW2_CLUSTER_ZERO_ALLOC) {
> -                continue;
> +        for (slice = 0; slice < n_slices; slice++) {
> +            uint64_t slice_offset = l2_offset + slice * slice_size;
> +            if (is_active_l1) {
> +                /* get active L2 tables from cache */
> +                ret = qcow2_cache_get(bs, s->l2_table_cache, slice_offset,
> +                                      (void **)&l2_slice);
> +            } else {
> +                /* load inactive L2 tables from disk */
> +                ret = bdrv_read(bs->file, slice_offset >> BDRV_SECTOR_BITS,
> +                                (void *)l2_slice,
> +                                slice_size >> BDRV_SECTOR_BITS);

...but is it time to convert this to use bdrv_pread[v](), for one less
use of the older sector-based interfaces?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]