Re: [pve-devel] [PATCH pve-storage 09/13] storage: add volume_support_qemu_snapshot

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Tue, Jul 15, 2025 at 01:59:33PM +0000, DERUMIER, Alexandre wrote:
> 
> > +* Introduce volume_support_qemu_snapshot() plugin method
> > +    This method is used to known if the a snapshot need to be done
> > by qemu
> > +    or by the storage api.
> > +    returned values are :
> > +    'internal' : support snapshot with qemu internal snapshot
> > +    'external' : support snapshot with qemu external snapshot
> > +    undef      : don't support qemu snapshot
> 
> >>The naming, description and returned values seem a bit of a mixed bag
> >>here.
> >>Also because "internal", from the POV of the storage, happens
> >>"outside" of the storage.
> yes, indeed, I was a little bit out of idea for the naming, sorry :/
> 
> 
> 
> >>Also consider that technically the *beginning* of a snapshot chain
> >>can
> >>even have a raw format, so I also wouldn't specifically pin the
> >>ability
> >>to create a new volume using a backing volume on the current *v
> >olume*.
> 
> yes, Fabian wanted to use only qcow2 for the base image for now, to
> avoid to mix both format in the chain
> 
> 
> >>As a side note: Looking at the LVM plugin, it now reports "qcow2"
> >>support, but refuses it unless an option is set. The list of which
> >>formats are supported is currently not storage-config dependent,
> >>which
> >>is a bit unfortunate there.
> 
> do you mean my last follow-up about "external-snapshots" option for lvm
> ? Because Fabian && Thomas wanted it as safe-guard until it's still
> experimental
> 
> 
> 
> >>How about one of these (a bit long but bear with me):
> >>  supports_snapshots_as_backed_volumes($scfg)
> >>    The storage:
> >>    - Can create volumes with backing volumes (required for NOT-
> >>running
> >>      VMs)
> >>    - Allows renaming a volume into a snapshot such that calling
> >>      `volume_snapshot` then recreates the original name... (BUT! See
> >>      note [1] below for why I think this should not be a
> >>requirement!)
> >>      (for running VMs)
> >>combined with
> >>
> >>  is_format_available($scfg, "qcow2")
> >>
> >>(to directly check for the optional support as in the LVM plugin
> early)
> 
> >>qemu-server's do_snapshot_type() then does:
> >>
> >>    "storage" if
> >>       is tpmstate
> >>       OR is not running
> >>       OR format isn't qcow2 (this case would be new in qemu-server)
> >>    "external" if
> >>       supports_snapshots_as_backed_volumes() is true
> >>       AND is_format_available($scfg, "qcow2")
> >>    "internal" otherwise
> 
> you also have the case for rbd, where krbd need to be done at storage
> level, but rbd need to be done by qemu when it's running.
> 
> So I think it's more something like :
> --       OR format isn't qcow2 (this case would be new in qemu-server)
> ++       OR qemu block driver support .bdrv_snapshot_create 
> (currently it's only qcow2 && rbd :
> https://github.com/search?q=repo%3Aqemu%2Fqemu%20%20.bdrv_snapshot_create%20&type=code)

Right, I forgot about that.

Okay, so I guess we'd keep this qemu-related. In that case:

    sub qemu_snapshot_responsibility($scfg, $volname)
      -> qemu     (previously "internal")
      -> storage  (previously undef)
      -> mixed    (previously "external")

We should probably note in the API documentation that "mixed" is
experimental and the expected behavior may change, because, as I
mentioned further down previously, I think the behavior could be a bit
less awkward on the storage side.

> 
> 
> 
> 
> 
> >>Notes:
> >>[1] Do we really need to rename the volume *outside* of the storage?
> >>Why does eg. the LVM plugin need to distinguish between running and
> >>not
> >>running at all? All it does is shove some `/dev/` names around, after
> >>all. We should be able to go through the change-file/reopen code in
> >>qemu-server regardless of when/who/where renames the files, no?
> >>
> >>Taking a snapshot of a running VM does:
> >>  1. rename volume "into" snapshot from out of qemu-server
> >>  2. tell qemu to reopen the files under the new names
> >>  3. call volume_snapshot which:
> >>    - creates volume with the original name and the snapshot as
> >>backing
> >>  4. have qemu open the disk by the original name for blockdev-
> snapshot
> 
> 
> Can't we make this:
> >>  1. tell qemu to reopen the original files with different *node ids*
> >>     (since the clashing of those is the only issue we run into when
> >>     simply dropping the $running case and skipping the early 
> >>rename...)
> >>  2. call volume_snapshot without a $running parameter
> >>  3. continue as before (point 4 above)
> 
> ok, so we could try to 
> 1) reopen the current volume (vm-disk-..) with nodename=volname+snap
> before renaming it,
> 2) do the volume_snapshot in storage (rename the current volume to snap
> , create a new current volume with the snap backing)
> 3) add the new current volume blockdev + reopen it with blockdev-
> snapshot.
> 
> Here, I'm not sure if it's working, because AFAIR, when you call
> blockdev-snapshot, 
>     mon_cmd( 
>         $vmid, 'blockdev-snapshot',
>         node => $snap_fmt_blockdev->{'node-name'},
>         overlay => $new_fmt_blockdev->{'node-name'},
>     );
> 
> it's reading the filename in the snap blockdev node (and in this case,
> it's still vm-disk-...) and write it in the backing_file info of the
> new overlay node.
> 
> I think I have tried in past, but I'm not 100% sure. I'll try to test
> it again this week.

Yeah I tried some quick tests and it seems to be a bit tricky. Or maybe
I just missed something.

> 
> 
> 
> I'm going on holiday for 3 weeks this Friday, so I'll not have time to
> sent patch before nest, but feel free to change|improve|rewrite my code
> by something better :)


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