[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v12 04/10] qcow2: Make distinction between zero
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [PATCH v12 04/10] qcow2: Make distinction between zero cluster types obvious |
Date: |
Fri, 5 May 2017 22:51:16 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.0.1 |
On 04.05.2017 05:07, Eric Blake wrote:
> Treat plain zero clusters differently from allocated ones, so that
> we can simplify the logic of checking whether an offset is present.
> Do this by splitting QCOW2_CLUSTER_ZERO into two new enums,
> QCOW2_CLUSTER_ZERO_PLAIN and QCOW2_CLUSTER_ZERO_ALLOC.
>
> I tried to arrange the enum so that we could use
> 'ret <= QCOW2_CLUSTER_ZERO_PLAIN' for all unallocated types, and
> 'ret >= QCOW2_CLUSTER_ZERO_ALLOC' for allocated types, although
> I didn't actually end up taking advantage of the layout.
>
> In many cases, this leads to simpler code, by properly combining
> cases (sometimes, both zero types pair together, other times,
> plain zero is more like unallocated while allocated zero is more
> like normal).
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v12: new patch
> ---
> block/qcow2.h | 8 +++++--
> block/qcow2-cluster.c | 65
> ++++++++++++++++++--------------------------------
> block/qcow2-refcount.c | 40 +++++++++++++------------------
> block/qcow2.c | 9 ++++---
> 4 files changed, 51 insertions(+), 71 deletions(-)
I have to admit I was rather skeptic of this idea at first (because I
thought we would have more places which treat both ZERO types the same
than those that separate it), but you have comprehensively proven me wrong.
Some nit picks below, I'll leave it completely up to you whether you
want to address them:
Reviewed-by: Max Reitz <address@hidden>
[...]
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index f3bfce6..14e2086 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
[...]
> @@ -558,52 +557,32 @@ int qcow2_get_cluster_offset(BlockDriverState *bs,
> uint64_t offset,
> assert(nb_clusters <= INT_MAX);
>
> ret = qcow2_get_cluster_type(*cluster_offset);
> + if (s->qcow_version < 3 && (ret == QCOW2_CLUSTER_ZERO_PLAIN ||
> + ret == QCOW2_CLUSTER_ZERO_ALLOC)) {
> + qcow2_signal_corruption(bs, true, -1, -1, "Zero cluster entry found"
> + " in pre-v3 image (L2 offset: %#" PRIx64
> + ", L2 index: %#x)", l2_offset, l2_index);
> + ret = -EIO;
> + goto fail;
> + }
> switch (ret) {
> case QCOW2_CLUSTER_COMPRESSED:
> /* Compressed clusters can only be processed one by one */
> c = 1;
> *cluster_offset &= L2E_COMPRESSED_OFFSET_SIZE_MASK;
> break;
> - case QCOW2_CLUSTER_ZERO:
> - if (s->qcow_version < 3) {
> - qcow2_signal_corruption(bs, true, -1, -1, "Zero cluster entry
> found"
> - " in pre-v3 image (L2 offset: %#" PRIx64
> - ", L2 index: %#x)", l2_offset, l2_index);
> - ret = -EIO;
> - goto fail;
> - }
> - /* 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);
> - *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_unallocated(nb_clusters,
> - &l2_table[l2_index],
> - QCOW2_CLUSTER_ZERO);
> - *cluster_offset = 0;
> - }
> - break;
> + case QCOW2_CLUSTER_ZERO_PLAIN:
> case QCOW2_CLUSTER_UNALLOCATED:
> /* how many empty clusters ? */
> c = count_contiguous_clusters_unallocated(nb_clusters,
> - &l2_table[l2_index],
> - QCOW2_CLUSTER_UNALLOCATED);
> + &l2_table[l2_index], ret);
Nit pick: Using ret here is a bit weird (because it's such a meaningless
name). It would be good if we had a separate cluster_type variable.
> *cluster_offset = 0;
> break;
> + case QCOW2_CLUSTER_ZERO_ALLOC:
> case QCOW2_CLUSTER_NORMAL:
> /* how many allocated clusters ? */
> c = count_contiguous_clusters(nb_clusters, s->cluster_size,
> - &l2_table[l2_index], QCOW_OFLAG_ZERO);
> + &l2_table[l2_index], QCOW_OFLAG_ZERO);
> *cluster_offset &= L2E_OFFSET_MASK;
> if (offset_into_cluster(s, *cluster_offset)) {
> qcow2_signal_corruption(bs, true, -1, -1, "Data cluster offset
> %#"
Well, preallocated zero clusters are not exactly data clusters... Not
that any user cared, but s/Data cluster/Cluster allocation/ would be
more correct.
By the way, allow me to state just how much I love this hunk: Very much.
Looks great! It gets a place on my list of favorite hunks of this year
at least.
[...]
> @@ -1760,7 +1740,8 @@ static int expand_zero_clusters_in_l1(BlockDriverState
> *bs, uint64_t *l1_table,
> int cluster_type = qcow2_get_cluster_type(l2_entry);>
> bool preallocated = offset != 0;
I could get behind removing this variable and replacing all
"if (!preallocated)" instances by
"if (cluster_type == QCOW2_CLUSTER_ZERO_PLAIN)". Up to you, though.
Max
>
> - if (cluster_type != QCOW2_CLUSTER_ZERO) {
> + if (cluster_type != QCOW2_CLUSTER_ZERO_PLAIN &&
> + cluster_type != QCOW2_CLUSTER_ZERO_ALLOC) {
> continue;
> }
>
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH v12 03/10] qcow2: Correctly report status of preallocated zero clusters, (continued)
- [Qemu-devel] [PATCH v12 01/10] qcow2: Use consistent switch indentation, Eric Blake, 2017/05/03
- [Qemu-devel] [PATCH v12 05/10] qcow2: Optimize zero_single_l2() to minimize L2 churn, Eric Blake, 2017/05/03
- [Qemu-devel] [PATCH v12 04/10] qcow2: Make distinction between zero cluster types obvious, Eric Blake, 2017/05/03
- Re: [Qemu-devel] [PATCH v12 04/10] qcow2: Make distinction between zero cluster types obvious,
Max Reitz <=
- [Qemu-devel] [PATCH v12 09/10] qcow2: Assert that cluster operations are aligned, Eric Blake, 2017/05/03
- [Qemu-devel] [PATCH v12 06/10] iotests: Improve _filter_qemu_img_map, Eric Blake, 2017/05/03
[Qemu-devel] [PATCH v12 10/10] qcow2: Discard/zero clusters by byte count, Eric Blake, 2017/05/03
[Qemu-devel] [PATCH v12 07/10] iotests: Add test 179 to cover write zeroes with unmap, Eric Blake, 2017/05/03