qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Pavel Butsykin
Subject: Re: [Qemu-devel] [PATCH v2 3/4] qcow2: add shrink image support
Date: Wed, 28 Jun 2017 18:31:25 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1

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
2. update s->free_cluster_index
3. call update_refcount_discard() (to in the end the fallocate
PUNCH_HOLE was called on refblock offset)

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.

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.

Max




reply via email to

[Prev in Thread] Current Thread [Next in Thread]