Re: [PATCH] qcow2: Allow resize of images with internal snapshots

[Eric Blake <eblake@redhat.com>]

On 4/23/20 12:21 PM, Max Reitz wrote:

>>> The previous comment was incorrect as well, but actually
>>> qcow2_truncate_bitmaps_check() doesn’t return an error when there is any
>>> bitmap, but only if there are non-active bitmaps, or active bitmaps that
>>> cannot be modified.  So for non-disabled bitmaps, we generally do
>>> happily proceed.
>>
>> The comment change is collateral (only because I noticed it in the
>> diff); but I could indeed reword it slightly more accurately as:
>>
>> Check if bitmaps prevent a resize.  Although bitmaps can be resized,
>> there are situations where we don't know whether to set or clear new
>> bits, so for now it's easiest to just prevent resize in those cases.
> 
> But I don’t know whether that explanation is actually correct.  I mean,
> that would apply for enabled active bitmaps just as well.  But we do
> allow resizing an image with such bitmaps so it seems clear that we have
> some idea on what to do.  (Or at least we pretend we do, for better or
> worse).
> 
> (Which is that bdrv_dirty_bitmap_truncate() just calls
> hbitmap_truncate(), which fills the new space with zeroes.)
> 
> The real reason we can’t resize with certain kinds of bitmaps present
> seems more like:
> (1) There are some bitmaps that can’t be written to, but we’d have to if
> we wanted to resize the image and they’re persistent,
> (2) The second case are bitmaps that are persistent but present in
> memory; we just haven’t implemented that case, I presume.
> 
> So it seems less like a case of “We don’t know what to do”, but more a
> combination of “We can’t“ and “We haven‘t implemented this case, but
> it’s clear what to do if we did”.

Indeed. So definitely a reason to split this change to a separate patch 
(and/or fix the code to finally implement it)


>>> Speaking of the image size.  Is it intentional that the size is changed
>>> to 32 MB?  Should amend work more like a transaction, in that we should
>>> at least do a loose check on whether the options can be changed before
>>> we touch the image?
>>
>> Yes, the commit message tried to call it out.  It's a pre-existing
>> problem - during amend, we DO make changes to the disk in one step, with
>> no way to roll back those changes, even if a later step fails.
>>
>> Pre-patch, if you request an upgrade to v3 as well as a resize, but
>> resize fails, you still end up with the image being changed to v3.
>> That's no different from post-patch where if you request a resize and a
>> downgrade to v2, the resize happens but not the downgrade.  On the
>> bright side, our current failure scenarios at least leave the resulting
>> image viable, even if it is not the same as it was pre-attempt.
> 
> Yes.  OK.

Okay, v2 will have a better commit message.


> Yeah.  I don’t think anyone even would have realistic use for
> transactional amends.  I suppose all users can easily split their amend
> calls with multiple options into multiple amends in the order that would
> be most likely reversible, if something went wrong along the way.  (And
> that also works.  I.e., downgrading/upgrading is probably the most easy
> to revert, but maybe you can only downgrade if your image has the
> correct size, so you potentially need to truncate it first.  OTOH, I
> can’t imagine many people actually use qemu-img amend to downgrade qcow2
> images anyway...)

Indeed - any time that you worry that an interaction of multiple changes 
might fail half-way through, you can always serialize those changes 
yourself instead of hoping the tool sequences them correctly ;)


> I feel very much reminded of the LUKS keyslot discussion...
> 
> (That is to say, my thoughts on this have little to do with this
> specific patch at this point.)

Too true !

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org