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

[Eric Blake]

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