Re: [Qemu-devel] [PATCH v2 16/32] qcow2: Update l2_allocate() to support L2 slices

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2175 bytes --]

On 12/15/2017 06:53 AM, Alberto Garcia wrote:
> This patch updates l2_allocate() to support the qcow2 cache returning
> L2 slices instead of full L2 tables.
> 
> The old code simply gets an L2 table from the cache and initializes it
> with zeroes or with the contents of an existing table. With a cache
> that returns slices instead of tables the idea remains the same, but
> the code must now iterate over all the slices that are contained in an
> L2 table.
> 
> Since now we're operating with slices the function can no longer
> return the newly-allocated table, so it's up to the caller to retrieve
> the appropriate L2 slice after calling l2_allocate() (note that with
> this patch the caller is still loading full L2 tables, but we'll deal
> with that in a separate patch).
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/qcow2-cluster.c | 86 +++++++++++++++++++++++++++++++--------------------
>  1 file changed, 52 insertions(+), 34 deletions(-)
> 

> @@ -299,42 +300,50 @@ static int l2_allocate(BlockDriverState *bs, int l1_index, uint64_t **table)
>  
>      /* allocate a new entry in the l2 cache */
>  
> +    slice_size = s->l2_slice_size * sizeof(uint64_t);

Would this read any better if the earlier patch named it
s->l2_slice_entries?  (When I see size, I try to think bytes, even
though the earlier patch made l2_slice_size be the number of entries;
worse, you are multiplying it back to a local variable slice_size that
IS in bytes).


> +
> +        /* write the l2 slice to the file */
> +        BLKDBG_EVENT(bs->file, BLKDBG_L2_ALLOC_WRITE);
>  
> -    /* write the l2 table to the file */
> -    BLKDBG_EVENT(bs->file, BLKDBG_L2_ALLOC_WRITE);
> +        trace_qcow2_l2_allocate_write_l2(bs, l1_index);
> +        qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_slice);
> +        ret = qcow2_cache_flush(bs, s->l2_table_cache);

As Anton pointed out, flushing the cache seems like you could do it once
after the loop rather than on each iteration.

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


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