| Message ID | 6932c2ddfe19a564cad7c54246290e166525fc46.1572125022.git.berto@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add subcluster allocation to qcow2 | expand |
On 26.10.19 23:25, 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. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > block/qcow2-cluster.c | 111 ++++++++++++++++++++---------------------- > 1 file changed, 52 insertions(+), 59 deletions(-) > > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c > index 990bc070af..e67559152f 100644 > --- a/block/qcow2-cluster.c > +++ b/block/qcow2-cluster.c > @@ -372,66 +372,51 @@ fail: > } > > /* > - * Checks how many clusters in a given L2 slice are contiguous in the image > - * file. As soon as one of the flags in the bitmask stop_flags changes compared > - * to the first cluster, the search is stopped and the cluster is not counted > - * as contiguous. (This allows it, for example, to stop at the first compressed > - * cluster which may require a different handling) > + * Return the number of contiguous subclusters of the exact same type > + * in a given L2 slice, starting from cluster @l2_index, subcluster > + * @sc_index. At most @nb_clusters are checked. Allocated clusters are I’d add a note that reassures that @nb_clusters really is a number of clusters, not subclusters. (Because if the variable were named “x” (i.e., it’d be “At most @x are checked”), you’d assume it refers to a number of subclusters.) OTOH, what I don’t like so far about this series is that the “cluster logic” is still everywhere when I think it should just be about subclusters now. (Except in few places where it must be about clusters as in something that can have a distinct host offset and/or has an own L2 entry.) So maybe the parameter should really be @nb_subclusters. But I’m not sure. For how this function is written right now, it makes sense for it to be @nb_clusters, but I think it could be changed so it would work with @nb_subclusters, too. (Just a single loop over the subclusters and then loading l2_entry+l2_bitmap and checking the offset at every cluster boundary.) > + * also required to be contiguous in the image file. > */ > -static int count_contiguous_clusters(BlockDriverState *bs, int nb_clusters, > - int cluster_size, uint64_t *l2_slice, int l2_index, uint64_t stop_flags) > +static int count_contiguous_subclusters(BlockDriverState *bs, int nb_clusters, > + unsigned sc_index, uint64_t *l2_slice, > + int l2_index) > { > BDRVQcow2State *s = bs->opaque; > - int i; > - QCow2ClusterType first_cluster_type; > - uint64_t mask = stop_flags | L2E_OFFSET_MASK | QCOW_OFLAG_COMPRESSED; > - uint64_t first_entry = get_l2_entry(s, l2_slice, l2_index); > - uint64_t offset = first_entry & mask; > - > - first_cluster_type = qcow2_get_cluster_type(bs, first_entry); > - if (first_cluster_type == QCOW2_CLUSTER_UNALLOCATED) { > - return 0; > + int i, j, count = 0; > + uint64_t l2_entry = get_l2_entry(s, l2_slice, l2_index); > + uint64_t l2_bitmap = get_l2_bitmap(s, l2_slice, l2_index); > + uint64_t expected_offset = l2_entry & L2E_OFFSET_MASK; > + bool check_offset = true; > + QCow2ClusterType type = > + qcow2_get_subcluster_type(bs, l2_entry, l2_bitmap, sc_index); > + > + assert(type != QCOW2_CLUSTER_INVALID); /* The caller should check this */ > + > + if (type == QCOW2_CLUSTER_COMPRESSED) { > + return 1; /* Compressed clusters are always counted one by one */ Hm, yes, but cluster by cluster, not subcluster by subcluster, so this should be s->subclusters_per_cluster, and perhaps sc_index should be asserted to be 0. (Or it should be s->subclusters_per_cluster - sc_index.) [...] > @@ -514,8 +499,8 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset, I suppose this is get_subcluster_offset now. [...] > @@ -587,6 +574,13 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset, > goto fail; > } > switch (type) { > + case QCOW2_CLUSTER_INVALID: > + qcow2_signal_corruption(bs, true, -1, -1, "Invalid cluster entry found " > + " (L2 offset: %#" PRIx64 ", L2 index: %#x)", > + l2_offset, l2_index); > + ret = -EIO; > + goto fail; > + break; No need to break here. > case QCOW2_CLUSTER_COMPRESSED: > if (has_data_file(bs)) { > qcow2_signal_corruption(bs, true, -1, -1, "Compressed cluster " > @@ -602,16 +596,15 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset, > break; Here (QCOW2_CLUSTER_COMPRESSED), c is 1, even though it should count subclusters. Max > 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); > + c = count_contiguous_subclusters(bs, nb_clusters, sc_index, > + l2_slice, l2_index); > *cluster_offset = 0; > break;
On Mon 04 Nov 2019 03:58:57 PM CET, Max Reitz wrote: > OTOH, what I don’t like so far about this series is that the “cluster > logic” is still everywhere when I think it should just be about > subclusters now. (Except in few places where it must be about > clusters as in something that can have a distinct host offset and/or > has an own L2 entry.) So maybe the parameter should really be > @nb_subclusters. > But I’m not sure. For how this function is written right now, it > makes sense for it to be @nb_clusters, but I think it could be changed > so it would work with @nb_subclusters, too. I'm still reviewing your (much appreciated) feedback, but one thing I can tell you is that my initial versions were doing everything with subclusters because of the reasons you mention (i.e. there was @nb_subclusters and all that). Later when I started to tidy things up I realized that most of those places only needed the number of clusters after all, and in some cases the necessary changes were really minimal (like in handle_copied()). >> +static int count_contiguous_subclusters(BlockDriverState *bs, int nb_clusters, >> + unsigned sc_index, uint64_t *l2_slice, >> + int l2_index) >> { /* ... */ >> + if (type == QCOW2_CLUSTER_COMPRESSED) { >> + return 1; /* Compressed clusters are always counted one by one */ > > Hm, yes, but cluster by cluster, not subcluster by subcluster, so this > should be s->subclusters_per_cluster, and perhaps sc_index should be > asserted to be 0. (Or it should be s->subclusters_per_cluster - > sc_index.) Right, that's a bug, it forces the caller to decompress the cluster 32 times in order to read it completely! Thanks! (in reality this is not used because this function doesn't get called for compressed clusters but the same problem happens in the calling function, as you correctly point out. Maybe I should assert here instead) >> @@ -514,8 +499,8 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset, > > I suppose this is get_subcluster_offset now. Hmmm no, this returns the actual host cluster offset, then the caller uses offset_into_cluster() to get the final value (which doesn't need to be subcluster-aligned anyway). Berto
On 08.11.19 16:42, Alberto Garcia wrote: > On Mon 04 Nov 2019 03:58:57 PM CET, Max Reitz wrote: [...] >>> @@ -514,8 +499,8 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset, >> >> I suppose this is get_subcluster_offset now. > > Hmmm no, this returns the actual host cluster offset, then the caller > uses offset_into_cluster() to get the final value (which doesn't need to > be subcluster-aligned anyway). It still returns the subcluster type, though. Which makes the whole thing kind of a weird chimera... Maybe it would work better if the subcluster type would be stored in a pointer parameter, so it’d be clear? But does it have actually have any advantages to on one hand return a cluster-aligned host offset and on the other return a bytes count that starts at the mid-cluster offset? That’s weird already. Maybe the offset that’s returned should just not be cluster-aligned and the whole function be called qcow2_get_host_offset(). I suppose one reason (maybe the only one?) to return aligned offsets is for compressed clusters. It would be another kind of magic to return aligned offsets only for them. But maybe it’s worth it? (Judging from a quick glance, it doesn’t look too difficult to me to modify the callers to accept a mid-cluster host offset.) (Another thing I just noticed is that the comment above qcow2_get_cluster_offset() needs some fixing, because it still refers to whole clusters.) Max
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index 990bc070af..e67559152f 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -372,66 +372,51 @@ fail: } /* - * Checks how many clusters in a given L2 slice are contiguous in the image - * file. As soon as one of the flags in the bitmask stop_flags changes compared - * to the first cluster, the search is stopped and the cluster is not counted - * as contiguous. (This allows it, for example, to stop at the first compressed - * cluster which may require a different handling) + * Return the number of contiguous subclusters of the exact same type + * in a given L2 slice, starting from cluster @l2_index, subcluster + * @sc_index. At most @nb_clusters are checked. Allocated clusters are + * also required to be contiguous in the image file. */ -static int count_contiguous_clusters(BlockDriverState *bs, int nb_clusters, - int cluster_size, uint64_t *l2_slice, int l2_index, uint64_t stop_flags) +static int count_contiguous_subclusters(BlockDriverState *bs, int nb_clusters, + unsigned sc_index, uint64_t *l2_slice, + int l2_index) { BDRVQcow2State *s = bs->opaque; - int i; - QCow2ClusterType first_cluster_type; - uint64_t mask = stop_flags | L2E_OFFSET_MASK | QCOW_OFLAG_COMPRESSED; - uint64_t first_entry = get_l2_entry(s, l2_slice, l2_index); - uint64_t offset = first_entry & mask; - - first_cluster_type = qcow2_get_cluster_type(bs, first_entry); - if (first_cluster_type == QCOW2_CLUSTER_UNALLOCATED) { - return 0; + int i, j, count = 0; + uint64_t l2_entry = get_l2_entry(s, l2_slice, l2_index); + uint64_t l2_bitmap = get_l2_bitmap(s, l2_slice, l2_index); + uint64_t expected_offset = l2_entry & L2E_OFFSET_MASK; + bool check_offset = true; + QCow2ClusterType type = + qcow2_get_subcluster_type(bs, l2_entry, l2_bitmap, sc_index); + + assert(type != QCOW2_CLUSTER_INVALID); /* The caller should check this */ + + if (type == QCOW2_CLUSTER_COMPRESSED) { + return 1; /* Compressed clusters are always counted one by one */ } - /* must be allocated */ - assert(first_cluster_type == QCOW2_CLUSTER_NORMAL || - first_cluster_type == QCOW2_CLUSTER_ZERO_ALLOC); - - for (i = 0; i < nb_clusters; i++) { - uint64_t l2_entry = get_l2_entry(s, l2_slice, l2_index + i) & mask; - if (offset + (uint64_t) i * cluster_size != l2_entry) { - break; - } + if (type == QCOW2_CLUSTER_UNALLOCATED || type == QCOW2_CLUSTER_ZERO_PLAIN) { + check_offset = false; } - return i; -} - -/* - * Checks how many consecutive unallocated clusters in a given L2 - * slice have the same cluster type. - */ -static int count_contiguous_clusters_unallocated(BlockDriverState *bs, - int nb_clusters, - uint64_t *l2_slice, - int l2_index, - QCow2ClusterType wanted_type) -{ - BDRVQcow2State *s = bs->opaque; - int i; - - assert(wanted_type == QCOW2_CLUSTER_ZERO_PLAIN || - wanted_type == QCOW2_CLUSTER_UNALLOCATED); 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)) { + goto out; + } + 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) { + goto out; + } + count++; } + expected_offset += s->cluster_size; } - return i; +out: + return count; } static int coroutine_fn do_perform_cow_read(BlockDriverState *bs, @@ -514,8 +499,8 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset, unsigned int *bytes, uint64_t *cluster_offset) { BDRVQcow2State *s = bs->opaque; - unsigned int l2_index; - uint64_t l1_index, l2_offset, *l2_slice; + unsigned int l2_index, sc_index; + uint64_t l1_index, l2_offset, *l2_slice, l2_bitmap; int c; unsigned int offset_in_cluster; uint64_t bytes_available, bytes_needed, nb_clusters; @@ -569,7 +554,9 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset, /* find the cluster offset for the given disk offset */ l2_index = offset_to_l2_slice_index(s, offset); + sc_index = offset_to_sc_index(s, offset); *cluster_offset = get_l2_entry(s, l2_slice, l2_index); + l2_bitmap = get_l2_bitmap(s, l2_slice, l2_index); nb_clusters = size_to_clusters(s, bytes_needed); /* bytes_needed <= *bytes + offset_in_cluster, both of which are unsigned @@ -577,7 +564,7 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset, * true */ assert(nb_clusters <= INT_MAX); - type = qcow2_get_cluster_type(bs, *cluster_offset); + type = qcow2_get_subcluster_type(bs, *cluster_offset, l2_bitmap, sc_index); if (s->qcow_version < 3 && (type == QCOW2_CLUSTER_ZERO_PLAIN || type == QCOW2_CLUSTER_ZERO_ALLOC)) { qcow2_signal_corruption(bs, true, -1, -1, "Zero cluster entry found" @@ -587,6 +574,13 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset, goto fail; } switch (type) { + case QCOW2_CLUSTER_INVALID: + qcow2_signal_corruption(bs, true, -1, -1, "Invalid cluster entry found " + " (L2 offset: %#" PRIx64 ", L2 index: %#x)", + l2_offset, l2_index); + ret = -EIO; + goto fail; + break; case QCOW2_CLUSTER_COMPRESSED: if (has_data_file(bs)) { qcow2_signal_corruption(bs, true, -1, -1, "Compressed cluster " @@ -602,16 +596,15 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset, 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); + c = count_contiguous_subclusters(bs, nb_clusters, sc_index, + l2_slice, l2_index); *cluster_offset = 0; break; case QCOW2_CLUSTER_ZERO_ALLOC: case QCOW2_CLUSTER_NORMAL: - /* how many allocated clusters ? */ - c = count_contiguous_clusters(bs, nb_clusters, s->cluster_size, - l2_slice, l2_index, QCOW_OFLAG_ZERO); + case QCOW2_CLUSTER_UNALLOCATED_SUBCLUSTER: + c = count_contiguous_subclusters(bs, nb_clusters, sc_index, + l2_slice, l2_index); *cluster_offset &= L2E_OFFSET_MASK; if (offset_into_cluster(s, *cluster_offset)) { qcow2_signal_corruption(bs, true, -1, -1, @@ -640,7 +633,7 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset, qcow2_cache_put(s->l2_table_cache, (void **) &l2_slice); - bytes_available = (int64_t)c * s->cluster_size; + bytes_available = ((int64_t)c + sc_index) * s->subcluster_size; out: if (bytes_available > bytes_needed) {
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. Signed-off-by: Alberto Garcia <berto@igalia.com> --- block/qcow2-cluster.c | 111 ++++++++++++++++++++---------------------- 1 file changed, 52 insertions(+), 59 deletions(-)