[PATCH v1 0/4] Introduce active disk internal snapshot support

[Or Ozeri]

> -----Original Message-----
> From: Peter Krempa <pkrempa at redhat.com>
> Sent: Friday, 13 January 2023 17:21
> To: Or Ozeri <ORO at il.ibm.com>
> Cc: libvir-list at redhat.com
> Subject: [EXTERNAL] Re: [PATCH v1 0/4] Introduce active disk internal
> snapshot support
> 
> Could you elaborate how this will be useful? Generally disk only snapshots
> are of very limited use as when reverting you get to a state equating of a
> power loss of a real box.
> 
> This means potentially corrupted files, state and other drawbacks.
> 

This will be useful for backup of the disks for disaster recovery.
i.e. once the internal snapshots are taken, they will be backed up (using diffs of course) 
to a different geographic location.

> With external disk only snapshots this was at least somewhat useful as it
> made the original image read-only and thus accessible with a different
> process.
> 
> With internal snapshots you still can't even read that image as it's still locked
> in write mode, so the utility value is even less.
> 

The use-case we're considering is using RBD disks with raw format.
These internal RBD snapshots are read-accessible even while the original disk is still being write-accessed.

> > This implementation for the qemu driver works even when there are
> > other disks which ask for external snapshot.
> > Thus we remove the restriction disallowing mixing of internal and
> > external disk snapshots for active VMs.
> > Note that mixing is still disallowed for non-active VMs, as it require
> > a bit more work in a different area of the code.
> 
> Note that a few days ago Pavel pushed his series adding support for deletion
> of external snapshots. That code heavily depends on the fact that mixing
> internal/external snapshots was forbidden.
> 
> Thus the patch in this series will not be acceptable until you fix the code that
> Pavel added.
> 

My code touches only snapshot creation.
I see that for deletion, Pavel added a check which yields:
"deletion of external and internal children disk snapshots not supported "
So I don't think any change is required, unless you want to support a new feature of deletion of mixed snapshots.
I don't think we should put a lot of effort into coding a full-matrix of support between any two features
(i.e. snapshot deletion, and active internal snapshots) if there's no client use-case justifying this work.

> Also note that Pavel is working on reversion of external snapshots so that
> work might also colide.
> 

Like snapshot deletion, this sounds like something that can work alongside by simply adding a check which
will disallow reverting in a mixed case.

> 
> This bit is potentially dangerous as based on your code you don't seem to be
> checking that the name is unique across snapshots, which can cause potential
> problems when names between snapshots collide.
> 

I actually think you can specify a name for an internal snapshot today, if you set <name> under <domainsnapshot>, so my modification only extends this to allow per-disk name, instead of a single name for all disks.
So in theory I think snapshot name can collide even in today's code.
Second, IMHO this patch already handles collision to the best extent.
If there's a name collision on one of the disks, the blockdev-snapshot-internal-sync for that disk will fail,
and the encapsulating qmp_transaction will revert all its actions.

> This is also something that I feel requires elaboration of how it will be useful.

Just like external snapshots allowing you to specify a custom path for the new file, some clients wish to use their own
naming-convention for internal snapshots (instead of using the ctime names generated by libvirt)