-----Original Message----- From: Peter Krempa <pkrempa@redhat.com> Sent: Friday, 13 January 2023 17:21 To: Or Ozeri <ORO@il.ibm.com> Cc: libvir-list@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)