Re: [Qemu-devel] [PATCH v2 3/4] qcow2: add shrink image support

[Max Reitz <mreitz@redhat.com>]

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

On 2017-06-27 17:06, Pavel Butsykin wrote:
> On 26.06.2017 20:47, Max Reitz wrote:
>> On 2017-06-26 17:23, Pavel Butsykin wrote:
> []
>>>
>>> Is there any guarantee that in the future this will not change? Because
>>> in this case it can be a potential danger.
>>
>> Since this behavior is not documented anywhere, there is no guarantee.
>>
>>> I can add a comment... Or add a new variable with the size of
>>> reftable_tmp, and every time count min(s->refcount_table_size,
>>> reftable_tmp_size)
>>> before accessing to s->refcount_table[]/reftable_tmp[]
>>
>> Or (1) you add an assertion that refcount_table_size doesn't change
>> along with a comment why that is the case, which also explains in detail
>> why the call to qcow2_free_clusters() should be safe: The on-disk
>> reftable differs from the one in memory. qcow2_free_clusters()and
>> update_refcount() themselves do not access the reftable, so they are
>> safe. However, update_refcount() calls alloc_refcount_block(), and that
>> function does access the reftable: Now, as long as
>> s->refcount_table_size does not shrink (which I can't see why it would),
>> refcount_table_index should always be smaller. Now we're accessing
>> s->refcount_table: This will always return an existing refblock because
>> this will either be the refblock itself (for self-referencing refblocks)
>> or another one that is not going to be freed by qcow2_shrink_reftable()
>> because this function will not free refblocks which cover other clusters
>> than themselves.
>> We will then proceed to update the refblock which is either right (if it
>> is not the refblock to be freed) or won't do anything (if it is the one
>> to be freed).
>> In any case, we will never write to the reftable and reading from the
>> basically outdated cached version will never do anything bad.
> 
> OK, SGTM.
> 
>> Or (2) you copy reftable_tmp into s->refcount_table[] *before* any call
>> to qcow2_free_clusters(). To make this work, you would need to also
>> discard all refblocks from the cache in this function here (and not in
>> update_refcount()) and then only call qcow2_free_clusters() on refblocks
>> which were not self-referencing. An alternative hack would be to simply
>> mark the image dirty and just not do any qcow2_free_clusters() call...
> 
> The main purpose of qcow2_reftable_shrink() function is discard all
> unnecessary refblocks from the file. If we do only rewrite
> refcount_table and discard non-self-referencing refblocks (which are
> actually very rare), then the meaning of the function is lost.

It would do exactly the same. The idea is that you do not need to call
qcow2_free_clusters() on self-referencing refblocks at all, since they
are freed automatically when their reftable entry is overwritten with 0.

>> Or (3) of course it would be possible to not clean up refcount
>> structures at all...
> 
> Nice solution :)

It is, because as I said refcount structures only have a small overhead.

Max


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