Re: [PATCH v4 23/30] qcow2: Update L2 bitmap in qcow2_alloc_cluster_link_l2()

[Eric Blake]

On 3/17/20 1:16 PM, 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 <address@hidden>
> Reviewed-by: Max Reitz <address@hidden>
> ---
>   block/qcow2-cluster.c | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
>
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index ceacd91ea3..dfd8b66958 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1006,6 +1006,23 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, 
> QCowL2Meta *m)
>           assert((offset & L2E_OFFSET_MASK) == offset);
>   
>           set_l2_entry(s, l2_slice, l2_index + i, offset | QCOW_OFLAG_COPIED);
> +
> +        /* Update bitmap with the subclusters that were just written */
> +        if (has_subclusters(s)) {
> +            unsigned written_from = m->cow_start.offset;
> +            unsigned written_to = m->cow_end.offset + m->cow_end.nb_bytes ?:
> +                m->nb_clusters << s->cluster_bits;
> +            uint64_t l2_bitmap = get_l2_bitmap(s, l2_slice, l2_index + i);
> +            int sc;
> +            for (sc = 0; sc < s->subclusters_per_cluster; sc++) {
> +                int sc_off = i * s->cluster_size + sc * s->subcluster_size;
> +                if (sc_off >= written_from && sc_off < written_to) {
> +                    l2_bitmap |= QCOW_OFLAG_SUB_ALLOC(sc);
> +                    l2_bitmap &= ~QCOW_OFLAG_SUB_ZERO(sc);
> +                }
> +            }

Are there more efficient ways to set this series of bits than iterating 
one bit at a time, while still remaining legible?  For example, what if 
we had something like:

l2_bitmap = get_l2_bitmap(...);
int sc_from = OFFSET_TO_SC(written_from);
int sc_to = OFFSET_TO_SC(written_to - 1);
l2_bitmap |= QCOW_OFLAG_SUB_ALLOC_RANGE(sc_from, sc_to);
l2_bitmap &= ~QCOW_OFLAG_SUB_ZERO_RANGE(sc_from, sc_to);

which would require macros:

#define OFFSET_TO_SC(offset) (offset >> (s->cluster_bits - 6))
#define QCOW_OFLAG_SUB_ALLOC_RANGE(from, to) \
  deposit64(0, (from), (len) - (from), -1)
#define QCOW_OFLAG_SUB_ZERO_RANGE(from, to) \
  deposit64(0, (from) + 32, (len) - (from) + 32, -1)


> +            set_l2_bitmap(s, l2_slice, l2_index + i, l2_bitmap);

I'm hoping this function doesn't cause redundant I/O if the L2 entry 
didn't actually change.  But that's not the concern for this patch.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org