On 2017-06-28 17:31, Pavel Butsykin wrote:
> On 28.06.2017 16:59, Max Reitz wrote:
>> 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.
> 
> Not sure.. For self-referencing refblocks, we also need to do:
> 1. check if refcount > 1

Yes, if that wasn't an error flagged by qemu-img check. :-)

(http://git.qemu.org/?p=qemu.git;a=blob;f=block/qcow2-refcount.c;h=7c06061aae90eb4f091f51df995a9e099178c0ed;hb=HEAD#l1787)

> 2. update s->free_cluster_index
> 3. call update_refcount_discard() (to in the end the fallocate
> PUNCH_HOLE was called on refblock offset)

These, yes, you'd have to do here.

> It will be practically a copy-paste from qcow2_free_clusters(), so it is
> better to avoid it. I think that if it makes sense to do
> qcow2_reftable_shrink(), it is only because we can slightly reduce image
> size.

But it would be a small copy-paste (although I may very well be wrong)
and it would help me sleep better because I could actually understand it.

Max

>>>> 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.
> 
> Yes, I agree.