On 5/5/20 12:38 PM, Alberto Garcia wrote:
The logic of this function remains pretty much the same, except that
it uses count_contiguous_subclusters(), which combines the logic of
count_contiguous_clusters() / count_contiguous_clusters_unallocated()
and checks individual subclusters.
Maybe mention that qcow2_cluster_to_subcluster_type() is now inlined
into its lone remaining caller.
Signed-off-by: Alberto Garcia <be...@igalia.com>
---
block/qcow2.h | 38 +++++-------
block/qcow2-cluster.c | 141 ++++++++++++++++++++----------------------
2 files changed, 82 insertions(+), 97 deletions(-)
+++ b/block/qcow2-cluster.c
@@ -376,66 +376,58 @@ fail:
+static int count_contiguous_subclusters(BlockDriverState *bs, int nb_clusters,
+ unsigned sc_index, uint64_t *l2_slice,
+ int l2_index)
{
+
+ assert(type != QCOW2_SUBCLUSTER_INVALID); /* The caller should check this
*/
+ assert(l2_index + nb_clusters <= s->l2_size);
+
+ if (type == QCOW2_SUBCLUSTER_COMPRESSED) {
+ /* Compressed clusters are always processed one by one */
+ return s->subclusters_per_cluster - sc_index;
Should this assert(sc_index == 0)?
for (i = 0; i < nb_clusters; i++) {
- uint64_t entry = get_l2_entry(s, l2_slice, l2_index + i);
- QCow2ClusterType type = qcow2_get_cluster_type(bs, entry);
-
- if (type != wanted_type) {
- break;
+ l2_entry = get_l2_entry(s, l2_slice, l2_index + i);
+ l2_bitmap = get_l2_bitmap(s, l2_slice, l2_index + i);
+ if (check_offset && expected_offset != (l2_entry & L2E_OFFSET_MASK)) {
+ return count;
+ }
+ for (j = (i == 0) ? sc_index : 0; j < s->subclusters_per_cluster; j++)
{
+ if (qcow2_get_subcluster_type(bs, l2_entry, l2_bitmap, j) != type)
{
+ return count;
+ }
This really is checking that sub-clusters have the exact same type.
@@ -604,24 +604,17 @@ int qcow2_get_host_offset(BlockDriverState *bs, uint64_t
offset,
ret = -EIO;
goto fail;
}
- /* Compressed clusters can only be processed one by one */
- c = 1;
*host_offset = l2_entry & L2E_COMPRESSED_OFFSET_SIZE_MASK;
break;
- case QCOW2_CLUSTER_ZERO_PLAIN:
- case QCOW2_CLUSTER_UNALLOCATED:
- /* how many empty clusters ? */
- c = count_contiguous_clusters_unallocated(bs, nb_clusters,
- l2_slice, l2_index, type);
The old code was counting how many contiguous clusters has similar
types, coalescing ranges of two different cluster types into one
nb_clusters result.
+ case QCOW2_SUBCLUSTER_ZERO_PLAIN:
+ case QCOW2_SUBCLUSTER_UNALLOCATED_PLAIN:
*host_offset = 0;
break;
- case QCOW2_CLUSTER_ZERO_ALLOC:
- case QCOW2_CLUSTER_NORMAL: {
+ case QCOW2_SUBCLUSTER_ZERO_ALLOC:
+ case QCOW2_SUBCLUSTER_NORMAL:
+ case QCOW2_SUBCLUSTER_UNALLOCATED_ALLOC: {
uint64_t host_cluster_offset = l2_entry & L2E_OFFSET_MASK;
*host_offset = host_cluster_offset + offset_in_cluster;
- /* how many allocated clusters ? */
- c = count_contiguous_clusters(bs, nb_clusters, s->cluster_size,
- l2_slice, l2_index, QCOW_OFLAG_ZERO);
and here coalescing three different cluster types into one nb_clusters
result.
if (offset_into_cluster(s, host_cluster_offset)) {
qcow2_signal_corruption(bs, true, -1, -1,
"Cluster allocation offset %#"
@@ -647,9 +640,11 @@ int qcow2_get_host_offset(BlockDriverState *bs, uint64_t
offset,
abort();
}
+ sc = count_contiguous_subclusters(bs, nb_clusters, sc_index,
+ l2_slice, l2_index);
But the new code is stopping at the first different subcluster type,
rather than trying to coalesce as many compatible types into one larger
nb_clusters. When coupled with patch 19, that factors into my concern
over whether patch 19 needed to check for INVALID clusters in the
middle, or whether its fail: label was unreachable. But it also means
that you are potentially fragmenting the write in more places (every
time a subcluster status changes) rather than coalescing similar status,
the way the old code used to operate.
I don't think the extra fragmentation causes any correctness issues, but
it may cause performance issues.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
