Re: [Qemu-devel] [PATCH v10 02/17] qcow2: Correctly report status of preallocated zero clusters

[Max Reitz]

On 27.04.2017 03:46, Eric Blake wrote:
> We were throwing away the preallocation information associated with
> zero clusters.  But we should be matching the well-defined semantics
> in bdrv_get_block_status(), where (BDRV_BLOCK_ZERO |
> BDRV_BLOCK_OFFSET_VALID) informs the user which offset is reserved,
> while still reminding the user that reading from that offset is
> likely to read garbage.
> 
> Making this change lets us see which portions of an image are zero
> but preallocated, when using qemu-img map --output=json.  The
> --output=human side intentionally ignores all zero clusters, whether
> or not they are preallocated.
> 
> The fact that there is no change to qemu-iotests './check -qcow2'
> merely means that we aren't yet testing this aspect of qemu-img;
> a later patch will add a test.
> 
> Signed-off-by: Eric Blake <address@hidden>
> 
> ---
> v10: new patch
> ---
>  block/qcow2-cluster.c | 32 +++++++++++++++++++++++++++-----
>  1 file changed, 27 insertions(+), 5 deletions(-)

I'd propose you split the qcow2 changes off of this series.

> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 100398c..d1063df 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -328,6 +328,10 @@ static int count_contiguous_clusters(int nb_clusters, 
> int cluster_size,
>       return i;
>  }
> 
> +/*
> + * Checks how many consecutive clusters in a given L2 table have the same
> + * cluster type with no corresponding allocation.
> + */
>  static int count_contiguous_clusters_by_type(int nb_clusters,
>                                               uint64_t *l2_table,
>                                               int wanted_type)
> @@ -335,9 +339,10 @@ static int count_contiguous_clusters_by_type(int 
> nb_clusters,
>      int i;
> 
>      for (i = 0; i < nb_clusters; i++) {
> -        int type = qcow2_get_cluster_type(be64_to_cpu(l2_table[i]));
> +        uint64_t entry = be64_to_cpu(l2_table[i]);
> +        int type = qcow2_get_cluster_type(entry);
> 
> -        if (type != wanted_type) {
> +        if (type != wanted_type || entry & L2E_OFFSET_MASK) {

This works only for wanted_type \in \{ ZERO, UNALLOCATED \} -- which is
what the comment you added describes, but this may still warrant a
renaming of this function (count_contiguous_unallocated_clusters?) and
probably an assertion that wanted_type is ZERO or UNALLOCATED.

>              break;
>          }
>      }
> @@ -559,9 +564,26 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, 
> uint64_t offset,
>              ret = -EIO;
>              goto fail;
>          }
> -        c = count_contiguous_clusters_by_type(nb_clusters, 
> &l2_table[l2_index],
> -                                              QCOW2_CLUSTER_ZERO);
> -        *cluster_offset = 0;
> +        /* Distinguish between pure zero clusters and pre-allocated ones */
> +        if (*cluster_offset & L2E_OFFSET_MASK) {
> +            c = count_contiguous_clusters(nb_clusters, s->cluster_size,
> +                                          &l2_table[l2_index], 
> QCOW_OFLAG_ZERO);

You should probably switch this patch and the next one -- or I just send
my patches myself and you base your series on top of it...

Because before the next patch, this happens:

$ ./qemu-img create -f qcow2 foo.qcow2 64M
Formatting 'foo.qcow2', fmt=qcow2 size=67108864 encryption=off
cluster_size=65536 lazy_refcounts=off refcount_bits=16
$ ./qemu-io -c 'write 0 64M' -c 'write -z 0 64M' foo.qcow2
wrote 67108864/67108864 bytes at offset 0
64 MiB, 1 ops; 0.1846 sec (346.590 MiB/sec and 5.4155 ops/sec)
wrote 67108864/67108864 bytes at offset 0
64 MiB, 1 ops; 0.0004 sec (127.551 GiB/sec and 2040.8163 ops/sec)
$ ./qemu-img map foo.qcow2
Offset          Length          Mapped to       File
qemu-img: block/qcow2-cluster.c:319: count_contiguous_clusters:
Assertion `qcow2_get_cluster_type(first_entry) == QCOW2_CLUSTER_NORMAL'
failed.
[1]    4970 abort (core dumped)  ./qemu-img map foo.qcow2

Max

> +            *cluster_offset &= L2E_OFFSET_MASK;
> +            if (offset_into_cluster(s, *cluster_offset)) {
> +                qcow2_signal_corruption(bs, true, -1, -1,
> +                                        "Preallocated zero cluster offset %#"
> +                                        PRIx64 " unaligned (L2 offset: %#"
> +                                        PRIx64 ", L2 index: %#x)",
> +                                        *cluster_offset, l2_offset, 
> l2_index);
> +                ret = -EIO;
> +                goto fail;
> +            }
> +        } else {
> +            c = count_contiguous_clusters_by_type(nb_clusters,
> +                                                  &l2_table[l2_index],
> +                                                  QCOW2_CLUSTER_ZERO);
> +            *cluster_offset = 0;
> +        }
>          break;
>      case QCOW2_CLUSTER_UNALLOCATED:
>          /* how many empty clusters ? */
>

signature.asc
Description: OpenPGP digital signature