[pve-devel] [PATCH v3 pve-storage 1/3] qcow2: add external snapshot support

[Fabian Grünbichler]

> DERUMIER, Alexandre <alexandre.derumier at groupe-cyllene.com> hat am 10.01.2025 10:10 CET geschrieben:
> > +    if ($scfg->{snapext}) {
> > + #technically, we could manage multibranch, we it need lot more work
> > for snapshot delete
> > + #we need to implemente block-stream from deleted snapshot to all
> > others child branchs
> 
> >>see my comments in qemu-server - I think we actually want block-
> >>stream anyway, since it has the semantics we want..
> 
> I don't agree, we don't want always, because with block-stream, you
> need to copy parent to child.
> 
> for example, you have a 1TB image,  you take a snapshot, writing 5MB in
> the snapshot, delete the snapshot,  you'll need to read/copy 1TB data
> from parent to the snapshot file.  
> I don't read your qemu-server comment yet ;)

yes, for the "first" snapshot that is true (since that one is basically the baseline data, which will often be huge compared to the snapshot delta). but streaming (rebasing) saves us the rename, which makes the error handling a lot easier/less risky. maybe we could special case the first snapshot as a performance optimization? ;)

> > @@ -1201,13 +1257,52 @@ sub volume_snapshot_delete {
> >  
> >      return 1 if $running;
> >  
> > +    my $cmd = "";
> >      my $path = $class->filesystem_path($scfg, $volname);
> >  
> > -    $class->deactivate_volume($storeid, $scfg, $volname, $snap, {});
> > +    if ($scfg->{snapext}) {
> >  
> > -    my $cmd = ['/usr/bin/qemu-img', 'snapshot','-d', $snap, $path];
> > + my $snapshots = $class->volume_snapshot_info($scfg, $storeid,
> > $volname);
> > + my $snappath = $snapshots->{$snap}->{file};
> > + return if !-e $snappath;  #already deleted ?
> 
> >>shouldn't this be an error?
> 
> This one was if we want to do retry in case of error, if we have
> multiple disks. (for example, first snapshot delete api call,  the
> first disk remove the snapshot, but a bug occur and second disk don't
> remove the snapshot). 
> 
> User could want to unlock the vm-snaphot lock and  and fix it manually
> with calling again the snapshot delete.
> 
> I'm not sure how to handle this correctly ?

I think the force parameter for snapshot deletion covers this already, and it should be fine for this to die..

> 
> > +     print"commit $childpath\n";
> > +     $cmd = ['/usr/bin/qemu-img', 'commit', $childpath];
> > +     run_command($cmd);
> > +     print"delete $childpath\n";
> > +
> > +     unlink($childpath);
> 
> this unlink can be skipped?
> 
> > +     print"rename $snappath to $childpath\n";
> > +     rename($snappath, $childpath);
> 
> >>since this will overwrite $childpath anyway.. this also reduces the
> >>chance of something going wrong:
> >>
> >>- if the commit fails halfway through, nothing bad should have
> >>happened, other than some data is now stored in two snapshots and
> >>takes up extra space
> >>- if the rename fails, then all of the data of $snap is stored twice,
> >>but the backing chain is still valid
> >>
> >>notable, there is no longer a gap where $childpath doesn't exist,
> >>which would break the backing chain!
> 
> yes you are right, better to have it atomic indeed
> 
> 
> > + } else {
> > +     print"commit $snappath\n";
> > +     $cmd = ['/usr/bin/qemu-img', 'commit', $snappath];
> 
> >>leftover from previous version? not used/overwritten below ;)
> 
> no, this is really to commit the the snapshot to parent

but it is not executed..

> 
> > +     #if we delete an intermediate snapshot, we need to link upper
> > snapshot to base snapshot
> > +     die "missing parentsnap snapshot to rebase child $childpath\n"
> > if !$parentpath;
> > +     print "link $childsnap to $parentsnap\n";
> > +     $cmd = ['/usr/bin/qemu-img', 'rebase', '-u', '-b', $parentpath,
> > '-F', 'qcow2', '-f', 'qcow2', $childpath];
> 
> >>does this work? I would read the qemu-img manpage to say that '-u' is
> >>for when you've moved/converted the backing file, and want to update
> >>the reference in its overlay, and that it doesn't copy any data.. but
> >>we need to copy the data from $snap to $childpath (we just want to
> >>delete the snapshot, we don't want to drop all its changes from the
> >>history, that would corrupt the contents of the image).
> >>note the description of the "safe" variant:
> >>
> >>"                     This  is  the  default mode and performs a real
> >>rebase operation. The new backing file may differ from the old one
> >>and qemu-img rebase will take care of keeping the
> >>                     guest-visible content of FILENAME unchanged."
> >>
> >>IMHO this is the behaviour we need here?
> 
> This is only to change the backing chain ref in the qcow2 snapshot.
> (this is the only way to do it, they was a qemu-img ammend command in
> past, but it has been removed in
> 2020 https://patchwork.kernel.org/project/qemu-devel/patch/20200403175859.863248-5-eblake@redhat.com/,
> so the rebase is the good way to do it)
> 
> The merge is done by the previous qemu-img commit. (qemu-img commit
> can't change  change automatically the backing chain of the upper
> snapshot, because it don't have any idea than an upper snapshot could
> exist).

see above and below ;)

> this is for this usecase :
> 
> A<----B<----C.
> 
> you commit B to A,  then you need to change the backing file of C to A
> (instead B)
> 
> A<----C

but this is the wrong semantics.. the writes/delta in B need to go to C (they happened after A), not to A!

> (when done it live, qemu qmp block-commit is able to change
> automatically the backing chain of the upper snapshot, because qemu
> known the whole chain)

I think it's wrong there as well, see my comments on those patches ;)

> This is how libvirt is doing too
> https://kashyapc.fedorapeople.org/virt/lc-2012/snapshots-handout.html
> see "Deleting snapshots (and 'offline commit')"
> Method (1): base <- sn1 <- sn3 (by copying sn2 into sn1)
> Method (2): base <- sn1 <- sn3 (by copying sn2 into sn3)
> (This is commit vs stream)

but they use the "wrong" (v1) naming scheme where the name of the snapshot and the content don't line up..

> I think that we should look at used space of parent vs child,
> to choose the correct direction/method.