Re: [RFC PATCH v2 20/26] qcow2: Update L2 bitmap in qcow2_alloc_cluster_link_l2()

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2280 bytes --]

On 26.10.19 23:25, Alberto Garcia wrote:
> The L2 bitmap needs to be updated after each write to indicate what
> new subclusters are now allocated.
> 
> This needs to happen even if the cluster was already allocated and the
> L2 entry was otherwise valid.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/qcow2-cluster.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index fb6cf8df17..acb7226e03 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -980,6 +980,22 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m)
>  
>          set_l2_entry(s, l2_slice, l2_index + i, QCOW_OFLAG_COPIED |
>                       (cluster_offset + (i << s->cluster_bits)));
> +
> +        /* Update bitmap with the subclusters that were just written */
> +        if (has_subclusters(s)) {
> +            uint64_t written_from = m->cow_start.offset;
> +            uint64_t written_to = m->cow_end.offset + m->cow_end.nb_bytes;

It’s a bit strange to make these uint64_t when all the fields queried
are of type unsigned, but more on that below.

> +            uint64_t l2_bitmap = get_l2_bitmap(s, l2_slice, l2_index + i);
> +            int sc;
> +            for (sc = 0; sc < s->subclusters_per_cluster; sc++) {
> +                uint64_t sc_off = i * s->cluster_size + sc * s->subcluster_size;

It’s weird to give this a uint64_t type when all the variables in the
term are of type int.

I’m not sure whether it can overflow.  handle_alloc() limits everything
to INT_MAX, but I’m not sure about handle_copied().

Speaking of handle_copied(); both elements of Qcow2COWRegion are of type
unsigned.  handle_copied() doesn’t look like it takes any precautions to
limit the range to even UINT_MAX (and it should probably limit it to
INT_MAX).

Max

> +                if (sc_off >= written_from && sc_off < written_to) {
> +                    l2_bitmap |= QCOW_OFLAG_SUB_ALLOC(sc);
> +                    l2_bitmap &= ~QCOW_OFLAG_SUB_ZERO(sc);
> +                }
> +            }
> +            set_l2_bitmap(s, l2_slice, l2_index + i, l2_bitmap);
> +        }
>       }
>  
>  
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]