Re: [Qemu-devel] [PATCH v5 07/12] qemu-img: Empty images after commit

[Eric Blake]

On 04/22/2014 12:01 PM, Max Reitz wrote:

>> Thinking about it more, the ability to empty an image after commit
>> should be optional, and exposed as an option for the user to control.
>>

>> In other words, I'm arguing that the ONLY time that leaving the active
>> image empty after completing a commit is when we are committing ONLY one
>> file deep - the moment multiple files are involved, we have ALREADY
>> invalidated the intermediate files, so wasting time on them to make them
>> empty is pointless.  If I wanted my entire chain to be consistent, I
>> would have already done it in stages (commit snap2 into snap1, then
>> commit that into base).
> 
> You're right. Great, then I can drop this ugly list for keeping the
> backing chain.

So sounds like we want something like the following modes, when starting
with a chain 'base <- mid <- top':

qemu-img commit top
  - existing behavior: top committed to mid, then top is emptied (on
assumption that client will still use top)

qemu-img commit -d top
  - new boolean flag -d (mnemonic for drop) which states that the user
intends to drop 'top' after the operation, and instead use 'mid' as the
new active image.  Bypasses the attempt to empty top.

qemu-img commit -b mid top
qemu-img commit -d -b mid top
  - use of -b in isolation always implies -d; because the user
explicitly specified the base, even if the base is only one deep, we
don't bother emptying top

qemu-img commit -b base top
qemu-img commit -d -b base top
  - again, use of -b implies -d; here, since the commit crosses
boundaries, mid will be corrupted, so there is no point in emptying top
or mid

> I took a look into rebase because I wondered how it could discard
> sectors in a way they don't read back as zero but fall through to the
> backing file (which is what a generic implementation of
> bdrv_make_empty() would require and which I would have missed so far, if
> it existed). As far as I see it, rebase does not discard sectors, it
> only writes differing currently unallocated sectors.

I haven't looked closely at what rebase is actually doing, to know which
of the two scenarios you described actually happens.  And given this
thread, maybe it's worth a future patch to give the user the option for
both behaviors, as it sounds like a use case could be made for each.
But obviously not for this patch series.

> 
> And if I have missed something and rebase actually does discard sectors,
> it is still a rather expensive operation, as it has to compare the whole
> file basically to itself, in this case. commit leaving the image empty
> (after specifying the appropriate option, which should in my opinion
> default to true, as this is the current mode of operation) is not only
> syntactic sugar, but faster as well.

Yes, I agree that doing the discard as part of the commit operation can
indeed be faster than doing it in two separate steps, even if the end
result is the same.  And I also agree that existing behavior of emptying
the image as the default when no other option is specified is the best
way to preserve command-line compatibility; so my proposal is merely
limited to adding a new boolean flag (I chose -d) to explicitly request
no emptying, as well as make the new -b option imply the -d flag.

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

signature.asc
Description: OpenPGP digital signature