[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v2 3/4] qcow2: add shrink image support
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [PATCH v2 3/4] qcow2: add shrink image support |
Date: |
Wed, 28 Jun 2017 15:59:19 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.0 |
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
signature.asc
Description: OpenPGP digital signature
- [Qemu-block] [PATCH v2 2/4] qcow2: add qcow2_cache_discard, (continued)
- [Qemu-block] [PATCH v2 2/4] qcow2: add qcow2_cache_discard, Pavel Butsykin, 2017/06/13
- [Qemu-block] [PATCH v2 3/4] qcow2: add shrink image support, Pavel Butsykin, 2017/06/13
- Re: [Qemu-block] [PATCH v2 3/4] qcow2: add shrink image support, Max Reitz, 2017/06/21
- Re: [Qemu-block] [PATCH v2 3/4] qcow2: add shrink image support, Pavel Butsykin, 2017/06/22
- Re: [Qemu-block] [PATCH v2 3/4] qcow2: add shrink image support, Max Reitz, 2017/06/23
- Re: [Qemu-block] [PATCH v2 3/4] qcow2: add shrink image support, Pavel Butsykin, 2017/06/26
- Re: [Qemu-block] [PATCH v2 3/4] qcow2: add shrink image support, Max Reitz, 2017/06/26
- Re: [Qemu-block] [PATCH v2 3/4] qcow2: add shrink image support, Pavel Butsykin, 2017/06/27
- Re: [Qemu-block] [PATCH v2 3/4] qcow2: add shrink image support,
Max Reitz <=
- Re: [Qemu-block] [PATCH v2 3/4] qcow2: add shrink image support, Pavel Butsykin, 2017/06/28
- Re: [Qemu-block] [PATCH v2 3/4] qcow2: add shrink image support, Max Reitz, 2017/06/28
[Qemu-block] [PATCH v2 4/4] qemu-iotests: add shrinking image test, Pavel Butsykin, 2017/06/13