[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v10 02/17] qcow2: Correctly report status of pre
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [PATCH v10 02/17] qcow2: Correctly report status of preallocated zero clusters |
Date: |
Fri, 28 Apr 2017 19:35:30 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.0 |
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
- [Qemu-block] [PATCH v10 00/17] add blkdebug tests, Eric Blake, 2017/04/26
- [Qemu-block] [PATCH v10 02/17] qcow2: Correctly report status of preallocated zero clusters, Eric Blake, 2017/04/26
- Re: [Qemu-block] [PATCH v10 02/17] qcow2: Correctly report status of preallocated zero clusters,
Max Reitz <=
- [Qemu-block] [PATCH v10 01/17] block: Update comments on BDRV_BLOCK_* meanings, Eric Blake, 2017/04/26
- [Qemu-block] [PATCH v10 03/17] qcow2: Reuse preallocated zero clusters, Eric Blake, 2017/04/26
- [Qemu-block] [PATCH v10 04/17] qcow2: Optimize zero_single_l2() to minimize L2 churn, Eric Blake, 2017/04/26
- [Qemu-block] [PATCH v10 06/17] qemu-io: Don't open-code QEMU_IS_ALIGNED, Eric Blake, 2017/04/26