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 <ebl...@redhat.com> > > --- > 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
