Re: [pve-devel] [PATCH v4 pve-storage 1/5] qcow2: add external snapshot support

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

> DERUMIER, Alexandre <alexandre.derumier@groupe-cyllene.com> hat am 02.04.2025 10:01 CEST geschrieben:
> 
>  
> >  
> > @@ -716,7 +721,11 @@ sub filesystem_path {
> >  
> >      my $dir = $class->get_subdir($scfg, $vtype);
> >  
> > -    $dir .= "/$vmid" if $vtype eq 'images';
> > +    if ($scfg->{snapext} && $snapname) {
> > + $name = $class->get_snap_volname($volname, $snapname);
> > +    } else {
> > + $dir .= "/$vmid" if $vtype eq 'images';
> > +    }
> 
> >>this is a bit weird, as it mixes volnames (with the `$vmid/` prefix)
> >>and names (without), it's only called twice in this patch, and this
> >>here already has $volname parsed, so could we maybe let
> >>get_snap_volname take and return the $name part without the dir?
> 
> ok!
> 
> >  
> >      my $path = "$dir/$name";
> >  
> > @@ -873,7 +882,7 @@ sub clone_image {
> >  }
> >  
> >  sub alloc_image {
> > -    my ($class, $storeid, $scfg, $vmid, $fmt, $name, $size) = @_;
> > +    my ($class, $storeid, $scfg, $vmid, $fmt, $name, $size,
> > $backing) = @_;
> 
> >>this extends the storage API, so it should actually do that.. and
> >>probably $backing should not be an arbitrary path, but something that
> >>is resolved locally?
> 
> I'll send the $snapname as param instead

see my comments on the qemu-server side, I think it would be even better if we could just get rid of extending alloc_image like this, and instead always go via volume_snapshot..

> 
> 
> > +
> > +    if($backing) {
> > + push @$cmd, '-b', $backing, '-F', 'qcow2';
> > + push @$options, 'extended_l2=on','cluster_size=128k';
> > +    };
> > +    push @$options, preallocation_cmd_option($scfg, $fmt);
> > +    push @$cmd, '-o', join(',', @$options) if @$options > 0;
> > +    push @$cmd, '-f', $fmt, $path;
> > +    push @$cmd, "${size}K" if !$backing;
> 
> >>is this because it will automatically take the size from the backing
> >>image?
> 
> Yes, it take size from the backing.  (and refuse if you specify size
> param at the same time than backing file)

we pass a size and a backing file in qemu-server, so I guess that is wrong there? ;)

> 
> 
> > + my $path = $class->path($scfg, $volname, $storeid);
> > + my $snappath = $class->path($scfg, $volname, $storeid, $snap);
> > + #rename current volume to snap volume
> > + die "snapshot volume $snappath already exist\n" if -e $snappath;
> > + rename($path, $snappath) if -e $path;
> 
> >>this is still looking weird.. I don't think it makes sense interface
> >>wise to allow snapshotting a volume that doesn't even exist..
> 
> This is more by security, I'm still unsure of the behaviour if you have
> multiple disks, and that snapshot is dying in the middle. (1 disk
> rename, the other not renamed). 

I am not sure what we gain by this other than papering over issues.

for multi-disks what we'd need to do is:
- loop over volumes
-- take a snapshot of volume
-- as part of error handling directly in taking a snapshot, undo all steps done until the point of error
- if an error occurs, loop over all already correctly snapshotted volumes
-- undo snapshot of each such volume

> 
> > +
> > + my ($vtype, $name, $vmid, undef, undef, $isBase, $format) =
> > +     $class->parse_volname($volname);
> > +
> > + $class->alloc_image($storeid, $scfg, $vmid, 'qcow2', $name, undef,
> > $snappath);
> > + if ($@) {
> > +     eval { $class->free_image($storeid, $scfg, $volname, 0) };
> > +     warn $@ if $@;
> 
> >>missing cleanup - this should undo the rename from above
> 
> Do you have an idea how to do it with mutiple disk ?  
> (revert renaming of other disks elsewhere in the code? just keep them
> like this)? 

see above, the volume_snapshot task should clean up what it did up to the point of error, and its caller is responsible for undoing already completed volume_snapshot calls. obviously this won't always work (for example, if the snapshot fails because the storage has a problem, it's very likely that undoing things is not possible either and manual cleanup will be required)

> 
> 
> >  
> > @@ -1187,9 +1251,15 @@ sub volume_snapshot_rollback {
> >  
> >      my $path = $class->filesystem_path($scfg, $volname);
> >  
> > -    my $cmd = ['/usr/bin/qemu-img', 'snapshot','-a', $snap, $path];
> > -
> > -    run_command($cmd);
> > +    if ($scfg->{snapext}) {
> > + #simply delete the current snapshot and recreate it
> > + my $path = $class->filesystem_path($scfg, $volname);
> > + unlink($path);
> > + $class->volume_snapshot($scfg, $storeid, $volname, $snap);
> 
> >>instead of volume_snapshot, this could simply call alloc_image with
> >>the backing file? then volume_snapshot could always rename and always
> >>cleanup properly..
> 
> Yes , better like this indeed

I think alloc_image should be split into an internal helper that takes the backing file parameter, and the existing alloc_image that shouldn't get that parameter though

> 
> > 
> > + } else {
> > +     #we rebase the child image on the parent as new backing image
> 
> >>should we extend this to make it clear what this means? it means
> >>copying any parts of $snap that are not in $parent and not yet
> >>overwritten by $child into $child, right?
> >>
> yes,
> intermediate snapshot: (rebase)
> -------------------------------
> snap1 (10G)-->snap2 (1G)----current(1G)
> ---> delete snap2
> 
> rebase current on snap1
> 
> snap1(10G)----->current(2G)
> 
> 
> or
> 
> snap1 (10G)-->snap2 (1G)----> snap3 (1G)--->current(1G)
> ---> delete snap2
> 
> rebase snap3 on snap1
> 
> snap1 (10G)---> snap3 (2G)--->current(1G)
> 
> 
> 
> >>so how expensive this is depends on:
> >>- how many changes are between $parent and $snap (increases cost)
> >>- how many of those are overwritten by changes between $snap and
> >>$child (decreases cost)
> 
> 
> but yes, the final size of the child is not 100% the additional content
> of the deleted snapshot, if some blocks has already been overwriten in
> the child
> 
> 
> so, we could call it: "merge diff content of the delete snap to the
> childsnap"
> 
> 
> 
> 
> 
> >>+sub get_snap_volname {
> >>+    my ($class, $volname, $snapname) = @_;
> >>+
> >>+    my ($vtype, $name, $vmid, $basename, $basevmid, $isBase,
> >>$format) = $class->parse_volname($volname);
> +    $name = !$snapname || $snapname eq 'current' ? $volname :
> "$vmid/snap-$snapname-$name";
> 
> >>other way round would be better to group by volume first IMHO
> ($vmid/snap-$name-$snapname), as this is similar to how we encode
> >>snapshots often on the storage level (volume@snap). we also need to
> >>have some delimiter between snapshot and volume name that is not
> >>allowed in either (hard for volname since basically everything but
> >>'/' goes, but snapshots have a restricted character set (configid,
> >>which means alphanumeric, hyphen and underscore), so we could use
> >>something like '.' as delimiter? or we switch to directories and do
> >>$vmid/snap/$snap/$name?)
> 
> Personnaly, I prefer a '.' delimiter than sub directory. (better to
> have the info in the filename itself)

we can still bikeshed this later once other issue are ironed out, I just wanted to note it - it's very unfortunate that volume names currently are so unrestricted, but we have to live with it for now. I just realized that even snap-XXX.qcow2 would be recognized/returned as image, so we need something else or filter them out anyway.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel