[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v12 04/10] qcow2: Make distinction between zero
From: |
Eric Blake |
Subject: |
Re: [Qemu-block] [PATCH v12 04/10] qcow2: Make distinction between zero cluster types obvious |
Date: |
Sat, 6 May 2017 15:30:18 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.0 |
On 05/05/2017 03:51 PM, Max Reitz wrote:
> 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>
>>
>> @@ -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;
>> + }
...
>> + 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.
qcow2_get_cluster_offset() returns the cluster type on success, and
-errno on failure. So 'ret' actually makes some sense: it really is the
value we are about to return. But it may also work to have a separate
variable up front, then assign ret = cluster_type at the end; I'll play
with it and see which one looks better.
>
>> *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.
Good idea.
>
> 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.
Makes sense.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
- [Qemu-block] [PATCH v12 02/10] block: Update comments on BDRV_BLOCK_* meanings, (continued)
- [Qemu-block] [PATCH v12 01/10] qcow2: Use consistent switch indentation, Eric Blake, 2017/05/03
- [Qemu-block] [PATCH v12 05/10] qcow2: Optimize zero_single_l2() to minimize L2 churn, Eric Blake, 2017/05/03
- [Qemu-block] [PATCH v12 04/10] qcow2: Make distinction between zero cluster types obvious, Eric Blake, 2017/05/03
- [Qemu-block] [PATCH v12 07/10] iotests: Add test 179 to cover write zeroes with unmap, Eric Blake, 2017/05/03
- [Qemu-block] [PATCH v12 09/10] qcow2: Assert that cluster operations are aligned, Eric Blake, 2017/05/03
- [Qemu-block] [PATCH v12 10/10] qcow2: Discard/zero clusters by byte count, Eric Blake, 2017/05/03
- [Qemu-block] [PATCH v12 08/10] qcow2: Optimize write zero of unaligned tail cluster, Eric Blake, 2017/05/03
- [Qemu-block] [PATCH v12 06/10] iotests: Improve _filter_qemu_img_map, Eric Blake, 2017/05/03