Re: [Qemu-block] [PATCH] qcow2: Optimize the refcount-block overlap check

[Max Reitz]

On 01.02.2017 11:39, Alberto Garcia wrote:
> On Wed 01 Feb 2017 02:46:20 AM CET, Max Reitz <address@hidden> wrote:
> 
>>> The problem with the refcount table is that since it always occupies
>>> complete clusters its size is usually very big.
>>
>> Actually the main problem is that BDRVQcow2State.refcount_table_size
>> is updated very generously as opposed to BDRVQcow2State.l1_size. The
>> latter is usually exactly as large as it needs to be (because the L1
>> table size usually doesn't change), whereas the refcount_table_size is
>> just bumped up every time the image gets bigger until it reaches or
>> exceeds the value it needs to be.
> 
> That's actually a good point.
> 
> One reason why this happens is that the size of the L1 table is static
> and the qcow2 header stores the actual number of entries, whereas for
> the refcount table it stores the number of clusters.

The other reason is what I said: The image size changes, so the refcount
table size needs to be bumped up from time to time. The L1 table size is
pretty much static, so it makes sense to keep it exactly as large as it
needs to be to represent all of the virtual disk.

> Maybe we can have refcount_table_size store the actual size of the
> table. The number of clusters can be calculated from that after all, and
> we would get rid of max_refcount_table_index.

refcount_table_size as in the one in BDRVQcow2State? That is the number
of entries already. I don't think making it "exact" is worth it, because
see above (you need to bump it up any time an allocation is made, and
you don't want to reallocate the refcount table all the time).

Also, you lose all of the accuracy once the image is closed and reopened
because the field in the qcow2 header only stores number of clusters.
Changing that would not be backwards compatible. You could add a new
field which stores the number of entries, but I don't see the point, as
it is very easy to just recalculate it once the image is opened (which
doesn't take *that* much time if you do it just once).

> The change might be a bit more difficult, though, I'll take a look.

Well, I won't hold you back, but I don't think it's worth it.

Max

>>> @@ -439,6 +450,7 @@ static int alloc_refcount_block(BlockDriverState *bs,
>>>          }
>>>  
>>>          s->refcount_table[refcount_table_index] = new_block;
>>> +        s->max_refcount_table_index = refcount_table_index;
>>
>> Should be MAX(s->max_refcount_table_index, refcount_table_index) or
>> this won't support refcount tables with holes in them.
> 
> Good catch.
> 
> Berto
>

signature.asc
Description: OpenPGP digital signature