[pve-devel] [PATCH pve-storage 2/5] qcow2: add external snapshot support

[Fabian Grünbichler]

> DERUMIER, Alexandre <alexandre.derumier at groupe-cyllene.com> hat am 19.05.2025 15:01 CEST geschrieben:
> 
>  
> > + #if first snapshot,as it should be bigger,  we merge child, and
> > rename the snapshot to child
> > + if(!$parentsnap) {
> > +     print "commit: merge content of $childpath into $snappath\n";
> > +     $cmd = ['/usr/bin/qemu-img', 'commit', $childpath];
> > +     eval { run_command($cmd) };
> > +     if ($@) {
> 
> >>should we add an error here about what state this leaves the snapshot
> >>in?
> >>AFAIU we've potentially written some of the data from $child to
> >>$snap, so
> >>the state of $snap is technically invalid now?
> 
> yes, if you try to clone or rollback it, the content is invalid.
> the current is still working, so impact to run the vm, but the
> user really need to delete the snapshot later.
> 
> (I'm not sure if you should have some kind of snapshot state in vm
> config, to avoid to call rollback,clone if state is invalid? as the vm
> is locked anyway, maybe some logs is enough ?)

I think we'd at least want to document the expected failure state(s) here,
either as a comment or as a warning..

e.g., we could have a comment detailing what's what (for all the cases), and
a log message recommending deletion of the snapshot that is now "invalid"
(for this particular branch here).

> 
> > +     $cmd = ['/usr/bin/qemu-img', 'rebase', '-b', $parentpath, '-F',
> > 'qcow2', '-f', 'qcow2', $childpath];
> > +     eval { run_command($cmd) };
> > +     if ($@) {
> > + die "error rebase $childpath from $parentpath; $@\n";
> 
> >>same here, but in this case $child just contains some duplicate data
> >>so nothing is
> >>really broken?
> 
> yes, no problem here.
> 
> 
> 
> > +     }
> > +     #delete the snapshot
> > +     eval { $class->free_image($storeid, $scfg, $snap_volname, 0);
> > };
> > +     if ($@) {
> 
> >>and here we just leave a stray volume around that is not part of the
> >>backing chain
> >>anymore, right?
> 
> yes, the end of rebase is switching the backing_file info in the qcow2
> metadatas.