Re: [RFC PATCH v2 03/26] qcow2: Process QCOW2_CLUSTER_ZERO_ALLOC clusters in handle_copied()
On Wed 30 Oct 2019 03:24:08 PM CET, Max Reitz wrote:
>> static void calculate_l2_meta(BlockDriverState *bs, uint64_t host_offset,
>>uint64_t guest_offset, uint64_t bytes,
>> - QCowL2Meta **m, bool keep_old)
>> + uint64_t *l2_slice, QCowL2Meta **m, bool
>> keep_old)
>> {
>> BDRVQcow2State *s = bs->opaque;
>> -unsigned cow_start_from = 0;
>> +int l2_index = offset_to_l2_slice_index(s, guest_offset);
>> +uint64_t l2_entry;
>> +unsigned cow_start_from, cow_end_to;
>> unsigned cow_start_to = offset_into_cluster(s, guest_offset);
>> unsigned cow_end_from = cow_start_to + bytes;
>> -unsigned cow_end_to = ROUND_UP(cow_end_from, s->cluster_size);
>> unsigned nb_clusters = size_to_clusters(s, cow_end_from);
>> QCowL2Meta *old_m = *m;
>> +QCow2ClusterType type;
>> +
>> +/* Return if there's no COW (all clusters are normal and we keep them)
>> */
>> +if (keep_old) {
>> +int i;
>> +for (i = 0; i < nb_clusters; i++) {
>> +l2_entry = be64_to_cpu(l2_slice[l2_index + i]);
>
> I’d assert somewhere that l2_index + nb_clusters - 1 won’t overflow.
>
>> +if (qcow2_get_cluster_type(bs, l2_entry) !=
>> QCOW2_CLUSTER_NORMAL) {
>
> Wouldn’t cluster_needs_cow() be better?
The semantics of cluster_needs_cow() change in this patch (which also
updates the documentation). But I should maybe change the name instead.
>> +break;
>> +}
>> +}
>> +if (i == nb_clusters) {
>> +return;
>> +}
>> +}
>
> So I understand we always need to create an L2Meta structure in all
> other cases because we at least need to turn those clusters into
> normal clusters? (Even if they’re already allocated, as in the case
> of allocated zero clusters.)
That's correct.
>> -/* Returns true if writing to a cluster requires COW */
>> +/* Returns true if the cluster is unallocated or has refcount > 1 */
>> static bool cluster_needs_cow(BlockDriverState *bs, uint64_t l2_entry)
>> {
>> switch (qcow2_get_cluster_type(bs, l2_entry)) {
>> case QCOW2_CLUSTER_NORMAL:
>> +case QCOW2_CLUSTER_ZERO_ALLOC:
>> if (l2_entry & QCOW_OFLAG_COPIED) {
>> return false;
>
> Don’t zero-allocated clusters need COW always? (Because the at the
> given host offset isn’t guaranteed to be zero.)
Yeah, hence the semantics change I described earlier. I should probably
call it cluster_needs_new_allocation() or something like that, which is
what this means now ("true if unallocated or refcount > 1").
>> - * Returns the number of contiguous clusters that can be used for an
>> allocating
>> - * write, but require COW to be performed (this includes yet unallocated
>> space,
>> - * which must copy from the backing file)
>> + * Returns the number of contiguous clusters that can be written to
>> + * using one single write request, starting from @l2_index.
>> + * At most @nb_clusters are checked.
>> + *
>> + * If @want_cow is true this counts clusters that are either
>> + * unallocated, or allocated but with refcount > 1.
>
> +(So they need to be newly allocated and COWed)?
Yes. Which in this context is the same as "newly allocated" I guess,
because every newly allocated cluster requires COW.
> (Or is the past participle of COW COWn? Or maybe CedOW?)
:-))
>> + * If @want_cow is false this counts clusters that are already
>> + * allocated and can be written to using their current locations
>
> s/using their current locations/in-place/?
Ok.
>> @@ -1475,13 +1489,14 @@ static int handle_alloc(BlockDriverState *bs,
>> uint64_t guest_offset,
>> *bytes = MIN(*bytes, nb_bytes - offset_into_cluster(s, guest_offset));
>> assert(*bytes != 0);
>>
>> -calculate_l2_meta(bs, alloc_cluster_offset, guest_offset, *bytes,
>> - m, keep_old_clusters);
>> +calculate_l2_meta(bs, alloc_cluster_offset, guest_offset, *bytes,
>> l2_slice,
>> + m, false);
>>
>> -return 1;
>> +ret = 1;
>>
>> -fail:
>> -if (*m && (*m)->nb_clusters > 0) {
>> +out:
>> +qcow2_cache_put(s->l2_table_cache, (void **) &l2_slice);
>
> Is this a bug fix?
No, we call qcow2_cache_put() later because calculate_l2_meta() needs
l2_slice.
Berto
Re: [RFC PATCH v2 03/26] qcow2: Process QCOW2_CLUSTER_ZERO_ALLOC clusters in handle_copied()
On 26.10.19 23:25, Alberto Garcia wrote:
> When writing to a qcow2 file there are two functions that take a
> virtual offset and return a host offset, possibly allocating new
> clusters if necessary:
>
>- handle_copied() looks for normal data clusters that are already
> allocated and have a reference count of 1. In those clusters we
> can simply write the data and there is no need to perform any
> copy-on-write.
>
>- handle_alloc() looks for clusters that do need copy-on-write,
> either because they haven't been allocated yet, because their
> reference count is != 1 or because they are ZERO_ALLOC clusters.
>
> The ZERO_ALLOC case is a bit special because those are clusters that
> are already allocated and they could perfectly be dealt with in
> handle_copied() (as long as copy-on-write is performed when required).
>
> In fact, there is extra code specifically for them in handle_alloc()
> that tries to reuse the existing allocation if possible and frees them
> otherwise.
>
> This patch changes the handling of ZERO_ALLOC clusters so the
> semantics of these two functions are now like this:
>
>- handle_copied() looks for clusters that are already allocated and
> which we can overwrite (NORMAL and ZERO_ALLOC clusters with a
> reference count of 1).
>
>- handle_alloc() looks for clusters for which we need a new
> allocation (all other cases).
>
> One importante difference after this change is that clusters found in
> handle_copied() may now require copy-on-write, but this will be anyway
> necessary once we add support for subclusters.
>
> Signed-off-by: Alberto Garcia
> ---
> block/qcow2-cluster.c | 177 +++---
> 1 file changed, 96 insertions(+), 81 deletions(-)
>
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index aa1010a515..ee6b46f917 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1021,7 +1021,8 @@ void qcow2_alloc_cluster_abort(BlockDriverState *bs,
> QCowL2Meta *m)
>
> /*
> * For a given write request, create a new QCowL2Meta structure and
> - * add it to @m.
> + * add it to @m. If the write request does not need copy-on-write or
> + * changes to the L2 metadata then this function does nothing.
> *
> * @host_offset points to the beginning of the first cluster.
> *
> @@ -1034,15 +1035,51 @@ void qcow2_alloc_cluster_abort(BlockDriverState *bs,
> QCowL2Meta *m)
> */
> static void calculate_l2_meta(BlockDriverState *bs, uint64_t host_offset,
>uint64_t guest_offset, uint64_t bytes,
> - QCowL2Meta **m, bool keep_old)
> + uint64_t *l2_slice, QCowL2Meta **m, bool
> keep_old)
> {
> BDRVQcow2State *s = bs->opaque;
> -unsigned cow_start_from = 0;
> +int l2_index = offset_to_l2_slice_index(s, guest_offset);
> +uint64_t l2_entry;
> +unsigned cow_start_from, cow_end_to;
> unsigned cow_start_to = offset_into_cluster(s, guest_offset);
> unsigned cow_end_from = cow_start_to + bytes;
> -unsigned cow_end_to = ROUND_UP(cow_end_from, s->cluster_size);
> unsigned nb_clusters = size_to_clusters(s, cow_end_from);
> QCowL2Meta *old_m = *m;
> +QCow2ClusterType type;
> +
> +/* Return if there's no COW (all clusters are normal and we keep them) */
> +if (keep_old) {
> +int i;
> +for (i = 0; i < nb_clusters; i++) {
> +l2_entry = be64_to_cpu(l2_slice[l2_index + i]);
I’d assert somewhere that l2_index + nb_clusters - 1 won’t overflow.
> +if (qcow2_get_cluster_type(bs, l2_entry) !=
> QCOW2_CLUSTER_NORMAL) {
Wouldn’t cluster_needs_cow() be better?
> +break;
> +}
> +}
> +if (i == nb_clusters) {
> +return;
> +}
> +}
So I understand we always need to create an L2Meta structure in all
other cases because we at least need to turn those clusters into normal
clusters? (Even if they’re already allocated, as in the case of
allocated zero clusters.)
> +
> +/* Get the L2 entry from the first cluster */
> +l2_entry = be64_to_cpu(l2_slice[l2_index]);
> +type = qcow2_get_cluster_type(bs, l2_entry);
> +
> +if (type == QCOW2_CLUSTER_NORMAL && keep_old) {
> +cow_start_from = cow_start_to;
> +} else {
> +cow_start_from = 0;
> +}
> +
> +/* Get the L2 entry from the last cluster */
> +l2_entry = be64_to_cpu(l2_slice[l2_index + nb_clusters - 1]);
> +type = qcow2_get_cluster_type(bs, l2_entry);
> +
> +if (type == QCOW2_CLUSTER_NORMAL && keep_old) {
> +cow_end_to = cow_end_from;
> +} else {
> +cow_end_to = ROUND_UP(cow_end_from, s->cluster_size);
> +}
>
> *m = g_malloc0(sizeof(**m));
> **m = (QCowL2Meta) {
> @@ -1068,18 +1105,18 @@ static void calculate_l2_meta(BlockDriverState *bs,
> uint64_t host_offset,
> QLIST_INSERT_HEAD(&s->cluster_allocs,
