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

[Max Reitz]

On 2017-06-26 17:23, Pavel Butsykin wrote:
> On 23.06.2017 18:46, Max Reitz wrote:
>> On 2017-06-22 15:57, Pavel Butsykin wrote:
>>>
>>> On 22.06.2017 01:55, Max Reitz wrote:
>>>> On 2017-06-13 14:16, Pavel Butsykin wrote:
> []
>>>>> +        }
>>>>> +        qcow2_cache_put(bs, s->refcount_block_cache, &refblock);
>>>>> +
>>>>> +        reftable_tmp[i] = unused_block ? 0 :
>>>>> cpu_to_be64(s->refcount_table[i]);
>>>>> +    }
>>>>> +
>>>>> +    ret = bdrv_pwrite_sync(bs->file, s->refcount_table_offset,
>>>>> reftable_tmp,
>>>>> +                           sizeof(uint64_t) *
>>>>> s->refcount_table_size);
>>>>> +    if (ret < 0) {
>>>>> +        goto out;
>>>>> +    }
>>>>> +
>>>>> +    for (i = 0; i < s->refcount_table_size; i++) {
>>>>> +        if (s->refcount_table[i] && !reftable_tmp[i]) {
>>>>> +            qcow2_free_clusters(bs, s->refcount_table[i] &
>>>>> REFT_OFFSET_MASK,
>>>>> +                                s->cluster_size,
>>>>> QCOW2_DISCARD_ALWAYS);
>>>>
>>>> This doesn't feel like a very good idea. The bdrv_pwrite_sync() before
>>>> has brought the on-disk refcount structures into a different state than
>>>> what we have cached.
>>>
>>> It is for this inside qcow2_free_clusters()->update_refcount() the cache
>>> is discarded by qcow2_cache_discard().
>>
>> This doesn't change the fact that the in-memory reftable is different
>> from the on-disk reftable and that qcow2_free_clusters() may trip up on
>> that; the main issue is the allocate_refcount_block() call before.

*alloc_refcount_block(), sorry.

> before what?

Before qcow2_cache_discard() is called.

> If we are talking about allocate_refcount_block() calls after
> bdrv_pwrite_sync(), then... Inside allocate_refcount_block() will always
> be called load_refcount_block(), what actually is not so dangerous even
> if refcount_block_cache is empty. Because the refblock offset will
> always be taken from s->refcount_table.

Well, yes, and this offset has not been cleared yet, so it still points
to the old refblock (but the on-disk reftable does not, and this worries
me).

>> So we need a guarantee that update_refcount() won't touch the reftable
>> if the refcount is decreased. It will call alloc_refcount_block() and
>> that should definitely find the respective refblock to already exist
>> because of course it has a refcount already.
> 
> We don't touch the refblocks which contain references to other
> refblocks, this ensures that update_refcount() will not try to raise
> the discarded refblock.

It may ensure this in practice, yes, but I found proving this to be
rather difficult.

>> But here's an issue: It tries to read from s->refcount_table[], and you
>> are slowly overwriting it in the same loop here. So it may not actually
>> find the refcount (if a refblock is described by an earlier one).
>> (After more than an hour of debugging, I realized this is not true: You
>> will only zero reftable entries if the refblock describes nothing or
>> only themselves. So overwriting one reftable entry cannot have effects
>> on other refblocks. Or at least it should not.)
> 
> As you've noticed, here uses a simple approach:

Well, if it is a simple approach, I'm just very dense.

> We discard only refblocks that contain nothing or own reference. If we
> have a refblock that is actually empty, but contains a reference to
> another empty refblock, we don't touch this refblock. Maybe it's not the
> best solution, but at least it's simple and secure.

The theory is simple, yes, which is why I found it fine until the point
you decide to call usual refcount management functions with the on-disk
data differing from what is cached as s->refcount_table.

> There is another approach that can be applied here:
> 
> 1. decrease the refcounts for all refblocks

I think this will leave a broken image at this point, so I'd rather
avoid it.

> 2. find all empty refblocks
> 3. increase the refcounts for all refblocks
> 4. rewrite the refcount_table on disk (with the empty reftable entries)
> 5. release all the emptt reblocks in reverse order (start at the end of
> the s->refcount_table)
> 
> This will certainly allow us to get rid of all empty reblocks, but the
> code will be less welcoming :) Also the case when the refblock contains
> a reference to another refblock is quite rare.

Another way would be to invoke the function for dropping empty refblocks
repeatedly until no refblocks can be dropped anymore. This wouldn't
cover cyclic references, but I think that's fine.

In any case, though, I agree that we don't need to put too much work
into this. Refblocks by default just use around 0.03 % of what's needed
for data, so...

>> Another potential issue is that you're assuming s->refcount_table_size
>> to be constant. I cannot find a way for it not to be, but investigating
>> this is painful and I can't claim I know for sure that it is constant.
>> If it isn't, you may get overflows when accessing reftable_tmp[].
>>
>> (Yes, it may be constant; but the reader of this code has to read
>> through qcow2_free_clusters(), allocate_refcount_block() and
>> update_refcount() to know (or at least to guess) that's the case.)
> 
> 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.

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...

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

Max

signature.asc
Description: OpenPGP digital signature