Re: [Qemu-devel] [PATCH v2 18/21] qcow2: Add function for refcount order amendment

[Eric Blake]

On 11/18/2014 11:58 AM, Max Reitz wrote:
> On 18.11.2014 18:55, Eric Blake wrote:
>> On 11/14/2014 06:06 AM, Max Reitz wrote:
>>> Add a function qcow2_change_refcount_order() which allows changing the
>>> refcount order of a qcow2 image.
>>>
>>> Signed-off-by: Max Reitz <address@hidden>
>>> ---

>>> +        if (new_allocation) {
>>> +            if (new_reftable_offset) {
>>> +                qcow2_free_clusters(bs, new_reftable_offset,
>>> +                                    allocated_reftable_size *
>>> sizeof(uint64_t),
>>> +                                    QCOW2_DISCARD_NEVER);
>> Any reason you picked QCOW2_DISCARD_NEVER instead of some other policy?
> 
> Ah, discarding is always interesting... Last year I used
> QCOW2_DISCARD_ALWAYS, then asked Kevin and he basically said never to
> use ALWAYS unless one is really sure about it. I could have used
> QCOW2_DISCARD_OTHER... But the idea behind using NEVER in cases like
> this is that the clusters may get picked up by the following allocation,
> in which case having discarded them is not a good idea (there are some
> other places in the qcow2 code which use NEVER for the same reason).
> 
> So, in this case, I think NEVER is good.

Makes sense.  Yes, for THIS case, we are probably going to reuse the
just-discarded cluster on the very next walk, so it's not worth punching
a hole just to reinstate it.


>>> +done:
>>> +    if (new_reftable) {
>>> +        /* On success, new_reftable actually points to the old
>>> reftable (and
>>> +         * new_reftable_size is the old reftable's size); but that
>>> is just
>>> +         * fine */
>>> +        for (i = 0; i < new_reftable_size; i++) {
>>> +            uint64_t offset = new_reftable[i] & REFT_OFFSET_MASK;
>>> +            if (offset) {
>>> +                qcow2_free_clusters(bs, offset, s->cluster_size,
>>> +                                    QCOW2_DISCARD_NEVER);
>> Again, why the QCOW2_DISCARD_NEVER policy?
> 
> Here, I have nothing to justify it. I'll use QCOW2_DISCARD_OTHER in v3.

Thanks, and now I know a bit more about discard policy.

> 
>> Fix the MIN vs. MAX bug, and the two dead assignment statements, and you
>> can have:
>>
>> Reviewed-by: Eric Blake <address@hidden>
> 
> I'll also use QCOW2_DISCARD_OTHER for freeing the refblocks and the
> reftable after the "done" label, if you're fine with that.

Yes, works for me.

> 
> Once again, thanks a lot!

And thank you for a mentally engaging review :)  I'm still in the middle
of an email on a possible test you can write to provoke a different
3-pass scenario thanks to all-zero refblocks, so you may want to wait
for that before posting v3...

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature