Re: [Qemu-devel] [PATCH v10 04/17] qcow2: Optimize zero_single_l2() to minimize L2 churn

[Eric Blake <eblake@redhat.com>]

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

On 04/28/2017 01:00 PM, Max Reitz wrote:
> On 27.04.2017 03:46, Eric Blake wrote:
>> Similar to discard_single_l2(), we should try to avoid dirtying
>> the L2 cache when the cluster we are changing already has the
>> right characteristics.
>>

>> -        /* Update L2 entries */
>> +        /*
>> +         * Minimize L2 changes if the cluster already reads back as
>> +         * zeroes with correct allocation.
>> +         */
>> +        if (qcow2_get_cluster_type(old_offset) == QCOW2_CLUSTER_ZERO &&
>> +            !(unmap && old_offset & L2E_OFFSET_MASK)) {
>> +            continue;
>> +        }
>> +
>>          qcow2_cache_entry_mark_dirty(bs, s->l2_table_cache, l2_table);
>> -        if (old_offset & QCOW_OFLAG_COMPRESSED || flags & BDRV_REQ_MAY_UNMAP) {
>> +        if (old_offset & QCOW_OFLAG_COMPRESSED || unmap) {
> 
> Works, but I'd like it better to store the cluster type somewhere and
> then use it here instead of checking the COMPRESSED flag.

Indeed, qcow2_get_cluster_type() checks QCOW_OFLAG_COMPRESSED first -
but I like the idea of storing it in a temporary variable now that we
are using the cluster type in more than one place.

> 
> But it works, so it's up to you:
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> 
>>              l2_table[l2_index + i] = cpu_to_be64(QCOW_OFLAG_ZERO);
>>              qcow2_free_any_clusters(bs, old_offset, 1, QCOW2_DISCARD_REQUEST);
>>          } else {
>>
> 
> 

-- 
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: 604 bytes --]