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