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 <mre...@redhat.com> >>> ---
>>> + 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 <ebl...@redhat.com> > > 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