Re: [PATCH 1/3] qemu: implementation of transient option for qcow2 file
Thank you for your review.
On Tue, Jul 07, 2020 at 06:36:23AM -0500, Eric Blake wrote:
On 7/7/20 2:12 AM, Peter Krempa wrote:
> You can install a qcow2 overlay on top of a raw file too. IMO the
> implications of using <transient/> allow that.
>
> As said above I'd strongly prefer if the overlay is created in qemu
> using the blockdev-create blockjob (there is already infrastructure in
> libvirt to achieve that).
Agreed. At this point, any time we call out to qemu-img as a separate
process, we are probably doing it wrong.
Got it. I'm thinking about the procedure such as followings.
Does that make sense?
1) Open the monitor with qemuProcessQMPNew()/qemuProcessQMPStart(),
and connect it.
2) Setup the transient disk with qemuDomainPrepareStorageSourceBlockdev(),
qemuBlockStorageSourceAttachApplyStorage(),
qemuBlockStorageSourceCreateGetFormatProps()
and something...
3) Run blockdev-create command with qemuMonitorBlockdevCreate(), then
close the monitor.
4) Switch the original disk to the transient disk.
5) Build the blockdev argument for qemu.
6) Run qemu
And I suppose qemuBlockStorageSourceCreate() maybe useful for the hotplug...
Thanks,
Masa
>
> Additionally there are corner cases which this patch doesn't deal with:
>
> 1) the virDomainBlockCopy operation flattens the backing chain into the
> top level only. This means that <transient/> must be stripped or the
> operation rejected, as otherwise shutting down the VM would end up
> removing the disk image completely.
If you have marked a disk transient, does it still make sense to allow
copying that disk? Rejecting the operation is easiest, as permitting it
implies that even though you already said you don't care about changes to
your disk, you still want to be able to back up that disk.
>
> 2) the same as above is used also for non-shared-storage migration where
> we use block-copy internally to transport the disks, same as above
> applies. Here again it requires careful consideration of the semantics,
> e.g whether to reject it or e.g. copy it into the original filename and
> strip <transient/> (we can't currently copy multiple layers).
The easiest solution is to make a transient disk a migration-blocker. Of
course, this may annoy people, so migration properly creating a transient
destination on top of the original base, to preserve the fact that when the
migrated guest shuts down it reverts to original contents, is a nicer (but
more complex) goal.
>
> 3) active-layer virDomainBlockCommit moves the data from the transient
> overlay into the original (now backing image). The <transient> flag is
> stored in the disk struct though, so that would mean that the original
> disk source would be removed after stopping the VM. block commit must
> clear the <transient> flag.
Why should commit be permitted, when you declared that disk contents
shouldn't change? For that matter, external snapshots should be blocked if
there is a transient disk.
>
> One nice-to-have but not required modification would be to allow
> configuration of the transient disk's overlay path.
>
>