From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pve-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
by lore.proxmox.com (Postfix) with ESMTPS id 6C5A01FF164
for <inbox@lore.proxmox.com>; Fri, 9 May 2025 12:30:30 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 7F5A83A94C;
Fri, 9 May 2025 12:30:49 +0200 (CEST)
Date: Fri, 9 May 2025 12:30:07 +0200 (CEST)
From: =?UTF-8?Q?Fabian_Gr=C3=BCnbichler?= <f.gruenbichler@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Message-ID: <1569166554.12919.1746786607464@webmail.proxmox.com>
In-Reply-To: <mailman.30.1745322744.394.pve-devel@lists.proxmox.com>
References: <20250422115141.808427-1-alexandre.derumier@groupe-cyllene.com>
<mailman.30.1745322744.394.pve-devel@lists.proxmox.com>
MIME-Version: 1.0
X-Priority: 3
Importance: Normal
X-Mailer: Open-Xchange Mailer v7.10.6-Rev75
X-Originating-Client: open-xchange-appsuite
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.046 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
Subject: Re: [pve-devel] [PATCH qemu-server 14/14] qcow2: add external
snapshot support
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.proxmox.com>
> Alexandre Derumier via pve-devel <pve-devel@lists.proxmox.com> hat am 22.04.2025 13:51 CEST geschrieben:
no commit message..
> Signed-off-by: Alexandre Derumier <alexandre.derumier@groupe-cyllene.com>
> ---
> PVE/QemuConfig.pm | 4 +-
> PVE/QemuServer.pm | 237 +++++++++++++++++++++++++++++++++++-----
> PVE/QemuServer/Drive.pm | 39 ++++---
> 3 files changed, 239 insertions(+), 41 deletions(-)
>
> diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
> index 2609542c..785c84a2 100644
> --- a/PVE/QemuConfig.pm
> +++ b/PVE/QemuConfig.pm
> @@ -378,7 +378,7 @@ sub __snapshot_create_vol_snapshot {
>
> print "snapshotting '$device' ($drive->{file})\n";
>
> - PVE::QemuServer::qemu_volume_snapshot($vmid, $device, $storecfg, $volid, $snapname);
> + PVE::QemuServer::qemu_volume_snapshot($vmid, $device, $storecfg, $drive, $snapname);
> }
>
> sub __snapshot_delete_remove_drive {
> @@ -415,7 +415,7 @@ sub __snapshot_delete_vol_snapshot {
> my $storecfg = PVE::Storage::config();
> my $volid = $drive->{file};
>
> - PVE::QemuServer::qemu_volume_snapshot_delete($vmid, $storecfg, $volid, $snapname);
> + PVE::QemuServer::qemu_volume_snapshot_delete($vmid, $storecfg, $drive, $snapname);
>
> push @$unused, $volid;
> }
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 5cce7094..aff430df 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -55,7 +55,7 @@ use PVE::QemuServer::Helpers qw(config_aware_timeout min_version kvm_user_versio
> use PVE::QemuServer::Cloudinit;
> use PVE::QemuServer::CGroup;
> use PVE::QemuServer::CPUConfig qw(print_cpu_device get_cpu_options get_cpu_bitness is_native_arch get_amd_sev_object get_amd_sev_type);
> -use PVE::QemuServer::Drive qw(is_valid_drivename checked_volume_format drive_is_cloudinit drive_is_cdrom drive_is_read_only parse_drive print_drive print_drive_throttle_group generate_drive_blockdev generate_throttle_group);
> +use PVE::QemuServer::Drive qw(is_valid_drivename checked_volume_format drive_is_cloudinit drive_is_cdrom drive_is_read_only parse_drive print_drive print_drive_throttle_group generate_drive_blockdev generate_throttle_group generate_blockdev_throttle generate_file_blockdev generate_format_blockdev);
> use PVE::QemuServer::Machine;
> use PVE::QemuServer::Memory qw(get_current_memory);
> use PVE::QemuServer::MetaInfo;
> @@ -4518,20 +4518,193 @@ sub qemu_block_resize {
> }
>
> sub qemu_volume_snapshot {
> - my ($vmid, $deviceid, $storecfg, $volid, $snap) = @_;
> + my ($vmid, $deviceid, $storecfg, $drive, $snap) = @_;
>
> + my $volid = $drive->{file};
> my $running = check_running($vmid);
>
> - if ($running && do_snapshots_with_qemu($storecfg, $volid, $deviceid)) {
> - mon_cmd($vmid, 'blockdev-snapshot-internal-sync', device => $deviceid, name => $snap);
> + my $do_snapshots_with_qemu = do_snapshots_with_qemu($storecfg, $volid, $deviceid);
> +
> + if ($running && $do_snapshots_with_qemu) {
> + if ($do_snapshots_with_qemu == 2) {
> + print "internal qemu snapshot\n";
> + mon_cmd($vmid, 'blockdev-snapshot-internal-sync', device => $deviceid, name => $snap);
> + } elsif ($do_snapshots_with_qemu == 3) {
> + my $storeid = (PVE::Storage::parse_volume_id($volid))[0];
> + my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
> + print "external qemu snapshot\n";
> + my $snapshots = PVE::Storage::volume_snapshot_info($storecfg, $volid);
> + my $parent_snap = $snapshots->{'current'}->{parent};
> + blockdev_rename($storecfg, $vmid, $deviceid, $drive, 'current', $snap, $parent_snap);
> + eval { blockdev_external_snapshot($storecfg, $vmid, $deviceid, $drive, $snap) };
> + if ($@) {
> + print "error creating snapshot. Revert rename\n";
> + eval { blockdev_rename($storecfg, $vmid, $deviceid, $drive, $snap, 'current', $parent_snap) };
> + }
> + }
this is a very weird interface, because do_snapshots_with_qemu only returns either 2 or 3
or undef.. see the other comment regarding volume_has_features on patch #12
> } else {
> PVE::Storage::volume_snapshot($storecfg, $volid, $snap);
> }
> }
>
> +sub blockdev_external_snapshot {
> + my ($storecfg, $vmid, $deviceid, $drive, $snap, $size) = @_;
> +
> + my $volid = $drive->{file};
> +
> + #preallocate add a new current file with reference to backing-file
> + PVE::Storage::volume_snapshot($storecfg, $volid, $snap, 1);
> +
> + #be sure to add drive in write mode
> + delete($drive->{ro});
> +
> + my $new_file_blockdev = generate_file_blockdev($storecfg, $drive);
> + my $new_fmt_blockdev = generate_format_blockdev($storecfg, $drive, $new_file_blockdev);
> +
> + my $snap_file_blockdev = generate_file_blockdev($storecfg, $drive, $snap);
> + my $snap_fmt_blockdev = generate_format_blockdev($storecfg, $drive, $snap_file_blockdev, $snap);
> +
> + #backing need to be forced to undef in blockdev, to avoid reopen of backing-file on blockdev-add
> + $new_fmt_blockdev->{backing} = undef;
> +
> + PVE::QemuServer::Monitor::mon_cmd($vmid, 'blockdev-add', %$new_fmt_blockdev);
> +
> + mon_cmd($vmid, 'blockdev-snapshot', node => $snap_fmt_blockdev->{'node-name'}, overlay => $new_fmt_blockdev->{'node-name'});
> +}
> +
> +sub blockdev_delete {
> + my ($storecfg, $vmid, $drive, $file_blockdev, $fmt_blockdev, $snap) = @_;
> +
> + #add eval as reopen is auto removing the old nodename automatically only if it was created at vm start in command line argument
> + eval { mon_cmd($vmid, 'blockdev-del', 'node-name' => $file_blockdev->{'node-name'}) };
> + eval { mon_cmd($vmid, 'blockdev-del', 'node-name' => $fmt_blockdev->{'node-name'}) };
> +
> + #delete the file (don't use vdisk_free as we don't want to delete all snapshot chain)
> + print"delete old $file_blockdev->{filename}\n";
> +
> + my $storage_name = PVE::Storage::parse_volume_id($drive->{file});
> +
> + my $volid = $drive->{file};
> + PVE::Storage::volume_snapshot_delete($storecfg, $volid, $snap, 1);
> +}
> +
> +sub blockdev_rename {
> + my ($storecfg, $vmid, $deviceid, $drive, $src_snap, $target_snap, $parent_snap) = @_;
> +
> + print "rename $src_snap to $target_snap\n";
> +
> + my $volid = $drive->{file};
> +
> + my $src_file_blockdev = generate_file_blockdev($storecfg, $drive, $src_snap);
> + my $src_fmt_blockdev = generate_format_blockdev($storecfg, $drive, $src_file_blockdev, $src_snap);
> + my $target_file_blockdev = generate_file_blockdev($storecfg, $drive, $target_snap);
> + my $target_fmt_blockdev = generate_format_blockdev($storecfg, $drive, $target_file_blockdev, $target_snap);
> +
> + #rename volume image
> + PVE::Storage::rename_volume($storecfg, $volid, $vmid, undef, $src_snap, $target_snap);
> +
> + if($target_snap eq 'current' || $src_snap eq 'current') {
> + #rename from|to current
> +
> + #add backing to target
> + if ($parent_snap) {
> + my $parent_fmt_nodename = encode_nodename('fmt', $volid, $parent_snap);
> + $target_fmt_blockdev->{backing} = $parent_fmt_nodename;
> + }
> + PVE::QemuServer::Monitor::mon_cmd($vmid, 'blockdev-add', %$target_fmt_blockdev);
> +
> + #reopen the current throttlefilter nodename with the target fmt nodename
> + my $throttle_blockdev = generate_blockdev_throttle($drive, $target_fmt_blockdev->{'node-name'});
> + PVE::QemuServer::Monitor::mon_cmd($vmid, 'blockdev-reopen', options => [$throttle_blockdev]);
> + } else {
> + rename($src_file_blockdev->{filename}, $target_file_blockdev->{filename});
> +
> + #intermediate snapshot
> + PVE::QemuServer::Monitor::mon_cmd($vmid, 'blockdev-add', %$target_fmt_blockdev);
> +
> + #reopen the parent node with the new target fmt backing node
> + my $parent_file_blockdev = generate_file_blockdev($storecfg, $drive, $parent_snap);
> + my $parent_fmt_blockdev = generate_format_blockdev($storecfg, $drive, $parent_file_blockdev, $parent_snap);
> + $parent_fmt_blockdev->{backing} = $target_fmt_blockdev->{'node-name'};
> + PVE::QemuServer::Monitor::mon_cmd($vmid, 'blockdev-reopen', options => [$parent_fmt_blockdev]);
> +
> + #change backing-file in qcow2 metadatas
> + PVE::QemuServer::Monitor::mon_cmd($vmid, 'change-backing-file', device => $deviceid, 'image-node-name' => $parent_fmt_blockdev->{'node-name'}, 'backing-file' => $target_file_blockdev->{filename});
> + }
> +
> + # delete old file|fmt nodes
> + # add eval as reopen is auto removing the old nodename automatically only if it was created at vm start in command line argument
> + eval { PVE::QemuServer::Monitor::mon_cmd($vmid, 'blockdev-del', 'node-name' => $src_file_blockdev->{'node-name'})};
> + eval { PVE::QemuServer::Monitor::mon_cmd($vmid, 'blockdev-del', 'node-name' => $src_fmt_blockdev->{'node-name'})};
> +}
> +
> +sub blockdev_commit {
> + my ($storecfg, $vmid, $deviceid, $drive, $src_snap, $target_snap) = @_;
> +
> + my $volid = $drive->{file};
> +
> + print "block-commit $src_snap to base:$target_snap\n";
> +
> + my $target_file_blockdev = generate_file_blockdev($storecfg, $drive, $target_snap);
> + my $target_fmt_blockdev = generate_format_blockdev($storecfg, $drive, $target_file_blockdev, $target_snap);
> +
> + my $src_file_blockdev = generate_file_blockdev($storecfg, $drive, $src_snap);
> + my $src_fmt_blockdev = generate_format_blockdev($storecfg, $drive, $src_file_blockdev, $src_snap);
> +
> + my $job_id = "commit-$deviceid";
> + my $jobs = {};
> + my $opts = { 'job-id' => $job_id, device => $deviceid };
> +
> + my $complete = undef;
> + if ($src_snap && $src_snap ne 'current') {
> + $complete = 'auto';
> + $opts->{'top-node'} = $src_fmt_blockdev->{'node-name'};
> + $opts->{'base-node'} = $target_fmt_blockdev->{'node-name'};
> + } else {
> + $complete = 'complete';
> + $opts->{'base-node'} = $target_fmt_blockdev->{'node-name'};
> + $opts->{replaces} = $src_fmt_blockdev->{'node-name'};
> + }
> +
> + mon_cmd($vmid, "block-commit", %$opts);
> + $jobs->{$job_id} = {};
> + qemu_drive_mirror_monitor($vmid, undef, $jobs, $complete, 0, 'commit');
> +
> + blockdev_delete($storecfg, $vmid, $drive, $src_file_blockdev, $src_fmt_blockdev, $src_snap);
> +}
> +
> +sub blockdev_stream {
> + my ($storecfg, $vmid, $deviceid, $drive, $snap, $parent_snap, $target_snap) = @_;
> +
> + my $volid = $drive->{file};
> + $target_snap = undef if $target_snap eq 'current';
> +
> + my $parent_file_blockdev = generate_file_blockdev($storecfg, $drive, $parent_snap);
> + my $parent_fmt_blockdev = generate_format_blockdev($storecfg, $drive, $parent_file_blockdev, $parent_snap);
> +
> + my $target_file_blockdev = generate_file_blockdev($storecfg, $drive, $target_snap);
> + my $target_fmt_blockdev = generate_format_blockdev($storecfg, $drive, $target_file_blockdev, $target_snap);
> +
> + my $snap_file_blockdev = generate_file_blockdev($storecfg, $drive, $snap);
> + my $snap_fmt_blockdev = generate_format_blockdev($storecfg, $drive, $snap_file_blockdev, $snap);
> +
> + my $job_id = "stream-$deviceid";
> + my $jobs = {};
> + my $options = { 'job-id' => $job_id, device => $target_fmt_blockdev->{'node-name'} };
> + $options->{'base-node'} = $parent_fmt_blockdev->{'node-name'};
> + $options->{'backing-file'} = $parent_file_blockdev->{filename};
> +
> + mon_cmd($vmid, 'block-stream', %$options);
> + $jobs->{$job_id} = {};
> + qemu_drive_mirror_monitor($vmid, undef, $jobs, 'auto', 0, 'stream');
> +
> + blockdev_delete($storecfg, $vmid, $drive, $snap_file_blockdev, $snap_fmt_blockdev, $snap);
> +}
> +
> sub qemu_volume_snapshot_delete {
> - my ($vmid, $storecfg, $volid, $snap) = @_;
> + my ($vmid, $storecfg, $drive, $snap) = @_;
>
> + my $volid = $drive->{file};
> my $running = check_running($vmid);
> my $attached_deviceid;
>
> @@ -4543,13 +4716,36 @@ sub qemu_volume_snapshot_delete {
> });
> }
>
> + my $do_snapshots_with_qemu = do_snapshots_with_qemu($storecfg, $volid, $attached_deviceid);
> +
> if ($attached_deviceid && do_snapshots_with_qemu($storecfg, $volid, $attached_deviceid)) {
> - mon_cmd(
> - $vmid,
> - 'blockdev-snapshot-delete-internal-sync',
> - device => $attached_deviceid,
> - name => $snap,
> - );
> + if ($do_snapshots_with_qemu == 2) {
> + mon_cmd(
> + $vmid,
> + 'blockdev-snapshot-delete-internal-sync',
> + device => $attached_deviceid,
> + name => $snap,
> + );
> + } elsif ($do_snapshots_with_qemu == 3) {
> + print "delete qemu external snapshot\n";
> +
> + my $path = PVE::Storage::path($storecfg, $volid);
> + my $snapshots = PVE::Storage::volume_snapshot_info($storecfg, $volid);
> + my $parentsnap = $snapshots->{$snap}->{parent};
> + my $childsnap = $snapshots->{$snap}->{child};
> +
> + # if we delete the first snasphot, we commit because the first snapshot original base image, it should be big.
> + # improve-me: if firstsnap > child : commit, if firstsnap < child do a stream.
> + if(!$parentsnap) {
> + print"delete first snapshot $snap\n";
> + blockdev_commit($storecfg, $vmid, $attached_deviceid, $drive, $childsnap, $snap);
> + blockdev_rename($storecfg, $vmid, $attached_deviceid, $drive, $snap, $childsnap, $snapshots->{$childsnap}->{child});
> + } else {
> + #intermediate snapshot, we always stream the snapshot to child snapshot
> + print"stream intermediate snapshot $snap to $childsnap\n";
> + blockdev_stream($storecfg, $vmid, $attached_deviceid, $drive, $snap, $parentsnap, $childsnap);
> + }
> + }
> } else {
> PVE::Storage::volume_snapshot_delete(
> $storecfg, $volid, $snap, $attached_deviceid ? 1 : undef);
> @@ -7778,27 +7974,16 @@ sub foreach_storage_used_by_vm {
> }
> }
>
> -my $qemu_snap_storage = {
> - rbd => 1,
> -};
> sub do_snapshots_with_qemu {
> my ($storecfg, $volid, $deviceid) = @_;
>
> - return if $deviceid =~ m/tpmstate0/;
> + return if $deviceid && $deviceid =~ m/tpmstate0/;
>
> - my $storage_name = PVE::Storage::parse_volume_id($volid);
> - my $scfg = $storecfg->{ids}->{$storage_name};
> - die "could not find storage '$storage_name'\n" if !defined($scfg);
> + my $snapshot_type = PVE::Storage::volume_has_feature($storecfg, 'snapshot', $volid);
>
> - if ($qemu_snap_storage->{$scfg->{type}} && !$scfg->{krbd}){
> - return 1;
> - }
> + return $snapshot_type if $snapshot_type == 2 || $snapshot_type == 3;
>
> - if ($volid =~ m/\.(qcow2|qed)$/){
> - return 1;
> - }
> -
> - return;
> + return undef;
previously this returned 1 for rbd and not krbd, and also external plugins with qcow2 or
qed formatted volumes.. now it returns 2 for rbd but not krbd, and undef for such external
plugins?
> }
>
> sub qga_check_running {
> diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
> index 0737034d..93903a59 100644
> --- a/PVE/QemuServer/Drive.pm
> +++ b/PVE/QemuServer/Drive.pm
> @@ -27,6 +27,9 @@ print_drive
> print_drive_throttle_group
> generate_drive_blockdev
> generate_throttle_group
> +generate_blockdev_throttle
> +generate_format_blockdev
> +generate_file_blockdev
> );
>
> our $QEMU_FORMAT_RE = qr/raw|qcow|qcow2|qed|vmdk|cloop/;
> @@ -1074,6 +1077,8 @@ sub print_drive_throttle_group {
> sub generate_file_blockdev {
> my ($storecfg, $drive, $snap, $nodename) = @_;
>
> + $snap = undef if $snap && $snap eq 'current';
> +
> my $volid = $drive->{file};
> my $driveid = get_drive_id($drive);
>
> @@ -1209,6 +1214,8 @@ sub generate_file_blockdev {
> sub generate_format_blockdev {
> my ($storecfg, $drive, $file, $snap, $nodename) = @_;
>
> + $snap = undef if $snap && $snap eq 'current';
> +
> my $volid = $drive->{file};
> #nbd don't support format blockdev, return the fileblockdev
> return $file if $volid =~ /^nbd:/;
> @@ -1280,6 +1287,15 @@ sub generate_backing_chain_blockdev {
> return $chain_blockdev;
> }
>
> +sub generate_blockdev_throttle {
> + my ($drive, $blockdev_file) = @_;
> +
> + my $drive_id = get_drive_id($drive);
> + #this is the topfilter entry point, use $drive-drive_id as nodename
> + my $blockdev_throttle = { driver => "throttle", 'node-name' => "drive-$drive_id", 'throttle-group' => "throttle-drive-$drive_id", 'file' => $blockdev_file };
> + return $blockdev_throttle;
> +}
> +
> sub generate_drive_blockdev {
> my ($storecfg, $drive, $live_restore_name) = @_;
>
> @@ -1303,22 +1319,19 @@ sub generate_drive_blockdev {
> #pflash0 don't support throttle-filter
> return $blockdev_format if $drive_id eq 'pflash0';
>
> - my $blockdev_live_restore = undef;
> - if ($live_restore_name) {
> - die "$drive_id: Proxmox Backup Server backed drive cannot auto-detect the format\n"
> - if !$drive->{format};
> + return generate_blockdev_throttle($drive, $blockdev_format) if !$live_restore_name;
>
> - $blockdev_live_restore = { 'node-name' => "liverestore-drive-$drive_id",
> - backing => $live_restore_name,
> - 'auto-remove' => 'on', format => "alloc-track",
> - file => $blockdev_format };
> - }
> + die "$drive_id: Proxmox Backup Server backed drive cannot auto-detect the format\n"
> + if !$drive->{format};
>
> - #this is the topfilter entry point, use $drive-drive_id as nodename
> - my $blockdev_throttle = { driver => "throttle", 'node-name' => "drive-$drive_id", 'throttle-group' => "throttle-drive-$drive_id" };
> #put liverestore filter between throttle && format filter
> - $blockdev_throttle->{file} = $live_restore_name ? $blockdev_live_restore : $blockdev_format;
> - return $blockdev_throttle,
> + my $blockdev_live_restore = { 'node-name' => "liverestore-drive-$drive_id",
> + backing => $live_restore_name,
> + 'auto-remove' => 'on', format => "alloc-track",
> + file => $blockdev_format };
> +
> + return generate_blockdev_throttle($drive, $blockdev_live_restore);
> +
> }
>
> sub encode_base62 {
> --
> 2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel