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

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

On Wed, Jul 09, 2025 at 06:21:58PM +0200, Alexandre Derumier via pve-devel wrote:
> From: Alexandre Derumier <alexandre.derumier@groupe-cyllene.com>
> To: pve-devel@lists.proxmox.com
> Subject: [PATCH pve-storage 09/13] storage: add volume_support_qemu_snapshot
> Date: Wed,  9 Jul 2025 18:21:58 +0200
> Message-Id: <20250709162202.2952597-14-alexandre.derumier@groupe-cyllene.com>
> X-Mailer: git-send-email 2.39.5
> 
> Returns if the volume is supporting qemu snapshot:
>  'internal' : do the snapshot with qemu internal snapshot
>  'external' : do the snapshot with qemu external snapshot
>   undef     : does not support qemu snapshot
> 
> Signed-off-by: Alexandre Derumier <alexandre.derumier@groupe-cyllene.com>
> ---
>  ApiChangeLog                 |  8 ++++++++
>  src/PVE/Storage.pm           | 15 +++++++++++++++
>  src/PVE/Storage/DirPlugin.pm | 10 ++++++++++
>  src/PVE/Storage/LVMPlugin.pm |  7 +++++++
>  src/PVE/Storage/Plugin.pm    | 20 ++++++++++++++++++++
>  src/PVE/Storage/RBDPlugin.pm |  6 ++++++
>  6 files changed, 66 insertions(+)
> 
> diff --git a/ApiChangeLog b/ApiChangeLog
> index 12eef1f..68e94fd 100644
> --- a/ApiChangeLog
> +++ b/ApiChangeLog
> @@ -29,6 +29,14 @@ Future changes should be documented in here.
>  * Introduce rename_snapshot() plugin method
>      This method allow to rename a vm disk snapshot name to a different snapshot name.
>  
> +* 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.

I'd also argue this doesn't *specifically* have anything to do with qemu
itself (we could technically hook qcow2 files with ublk/fuse+loopdev to
use for containers as well...).

Let me try to layout my thoughts...
- "internal" means they are completely isolated within the volume
  itself, the storage has nothing to do with it. (sort of).
- "external" means the storage is used to allocate volumes, but *also*
  handles the creation of the *format*
- undef = only the storage itself can make snapshots (which is
  technically incorrect because qemu could still just use *qcow2
  snapshots*...
- The "internal" vs `undef` case is just a matter of "is the format
  snapshot-capable", which makes me wonder whether this method should
  even have 3 cases.

Conceptually, what we want is a way of creating a new volume with a
backing volume, specifically.
If we cannot do this, either the "user" needs to be able to create
snapshots *inside* the volume, or there is no snapshot support.

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 *volume*.

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.

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

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)


> +
>  ##  Version 11:
>  
>  * Allow declaring storage features via plugin data
> diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
> index e0b79fa..b796908 100755
> --- a/src/PVE/Storage.pm
> +++ b/src/PVE/Storage.pm
> @@ -2370,6 +2370,21 @@ sub rename_snapshot {
>      );
>  }
>  
> +sub volume_support_qemu_snapshot {
> +    my ($cfg, $volid) = @_;
> +
> +    my ($storeid, $volname) = parse_volume_id($volid, 1);
> +
> +    if ($storeid) {
> +        my $scfg = storage_config($cfg, $storeid);
> +
> +        my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
> +
> +        return $plugin->volume_support_qemu_snapshot($storeid, $scfg, $volname);
> +    }
> +    return undef;
> +}
> +
>  # Various io-heavy operations require io/bandwidth limits which can be
>  # configured on multiple levels: The global defaults in datacenter.cfg, and
>  # per-storage overrides. When we want to do a restore from storage A to storage
> diff --git a/src/PVE/Storage/DirPlugin.pm b/src/PVE/Storage/DirPlugin.pm
> index 10e4f70..3e92383 100644
> --- a/src/PVE/Storage/DirPlugin.pm
> +++ b/src/PVE/Storage/DirPlugin.pm
> @@ -314,4 +314,14 @@ sub get_import_metadata {
>      };
>  }
>  
> +sub volume_support_qemu_snapshot {
> +    my ($class, $storeid, $scfg, $volname) = @_;
> +
> +    my $format = ($class->parse_volname($volname))[6];
> +    return if $format ne 'qcow2';
> +
> +    my $type = $scfg->{'external-snapshots'} ? 'external' : 'internal';
> +    return $type;
> +}
> +
>  1;
> diff --git a/src/PVE/Storage/LVMPlugin.pm b/src/PVE/Storage/LVMPlugin.pm
> index 2441e59..be411e5 100644
> --- a/src/PVE/Storage/LVMPlugin.pm
> +++ b/src/PVE/Storage/LVMPlugin.pm
> @@ -861,4 +861,11 @@ sub rename_snapshot {
>      die "rename_snapshot is not implemented for $class";
>  }
>  
> +sub volume_support_qemu_snapshot {
> +    my ($class, $storeid, $scfg, $volname) = @_;
> +
> +    my $format = ($class->parse_volname($volname))[6];
> +    return 'external' if $format eq 'qcow2';
> +}
> +
>  1;
> diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
> index 6b2dc32..aab2024 100644
> --- a/src/PVE/Storage/Plugin.pm
> +++ b/src/PVE/Storage/Plugin.pm
> @@ -2262,6 +2262,26 @@ sub new_backup_provider {
>      die "implement me if enabling the feature 'backup-provider' in plugindata()->{features}\n";
>  }
>  
> +=pod
> +
> +=head3 volume_support_qemu_snapshot
> +
> +    $blockdev = $plugin->volume_support_qemu_snapshot($storeid, $scfg, $volname)
> +
> +Returns a string with the type of snapshot that qemu can do for a specific volume
> +
> +'internal' : support snapshot with qemu internal snapshot
> +'external' : support snapshot with qemu external snapshot
> +undef      : don't support qemu snapshot
> +=cut
> +
> +sub volume_support_qemu_snapshot {
> +    my ($class, $storeid, $scfg, $volname) = @_;
> +
> +    my $format = ($class->parse_volname($volname))[6];
> +    return 'internal' if $format eq 'qcow2';
> +}
> +
>  sub config_aware_base_mkdir {
>      my ($class, $scfg, $path) = @_;
>  
> diff --git a/src/PVE/Storage/RBDPlugin.pm b/src/PVE/Storage/RBDPlugin.pm
> index ee33006..45f8a7f 100644
> --- a/src/PVE/Storage/RBDPlugin.pm
> +++ b/src/PVE/Storage/RBDPlugin.pm
> @@ -1061,4 +1061,10 @@ sub rename_snapshot {
>      die "rename_snapshot is not implemented for $class";
>  }
>  
> +sub volume_support_qemu_snapshot {
> +    my ($class, $storeid, $scfg, $volname) = @_;
> +
> +    return 'internal' if !$scfg->{krbd};
> +}
> +
>  1;
> -- 
> 2.39.5


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