Re: [pve-devel] [PATCH v4 qemu-server 10/11] blockdev: add backing_chain support

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

> Alexandre Derumier via pve-devel <pve-devel@lists.proxmox.com> hat am 11.03.2025 11:29 CET geschrieben:

> We need to define name-nodes for all backing chain images,
> to be able to live rename them with blockdev-reopen
> 
> For linked clone, we don't need to definebase image(s) chain.
> They are auto added with #block nodename.
> 
> Signed-off-by: Alexandre Derumier <alexandre.derumier@groupe-cyllene.com>
> ---
>  PVE/QemuServer.pm       | 26 ++----------------
>  PVE/QemuServer/Drive.pm | 60 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 62 insertions(+), 24 deletions(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index d6aa5730..60481acc 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -54,7 +54,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);
> -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);
> +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 do_snapshots_with_qemu);
>  use PVE::QemuServer::Machine;
>  use PVE::QemuServer::Memory qw(get_current_memory);
>  use PVE::QemuServer::MetaInfo;
> @@ -3765,6 +3765,7 @@ sub config_to_command {
>  	# extra protection for templates, but SATA and IDE don't support it..
>  	$drive->{ro} = 1 if drive_is_read_only($conf, $drive);
>  	my $blockdev = generate_drive_blockdev($storecfg, $vmid, $drive, $live_blockdev_name);
> +	#FIXME: verify if external snapshot backing chain is matching config

this is rather important (also checking for loops?)

>  	push @$devices, '-blockdev', JSON->new->canonical->allow_nonref->encode($blockdev) if $blockdev;
>  	push @$devices, '-device', print_drivedevice_full(
>  	    $storecfg, $conf, $vmid, $drive, $bridges, $arch, $machine_type);
> @@ -7559,29 +7560,6 @@ 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/;
> -
> -    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);
> -
> -    if ($qemu_snap_storage->{$scfg->{type}} && !$scfg->{krbd}){
> -	return 1;
> -    }
> -
> -    if ($volid =~ m/\.(qcow2|qed)$/){
> -	return 1;
> -    }
> -
> -    return;
> -}
> -
>  sub qga_check_running {
>      my ($vmid, $nowarn) = @_;
>  
> diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
> index 5b281616..51513546 100644
> --- a/PVE/QemuServer/Drive.pm
> +++ b/PVE/QemuServer/Drive.pm
> @@ -18,6 +18,7 @@ our @EXPORT_OK = qw(
>  is_valid_drivename
>  checked_parse_volname
>  checked_volume_format
> +do_snapshots_with_qemu
>  drive_is_cloudinit
>  drive_is_cdrom
>  drive_is_read_only
> @@ -1230,6 +1231,32 @@ sub generate_file_blockdev {
>      return $blockdev;
>  }
>  
> +my $qemu_snap_storage = {
> +    rbd => 1,
> +};
> +
> +sub do_snapshots_with_qemu {
> +    my ($storecfg, $volid, $deviceid) = @_;
> +
> +    return if $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);
> +
> +    if ($qemu_snap_storage->{$scfg->{type}} && !$scfg->{krbd}){
> +        return 1;
> +    }
> +
> +    return 2 if $scfg->{snapext} || $scfg->{type} eq 'lvm' && $volid =~ m/\.(qcow2)/;
> +
> +    if ($volid =~ m/\.(qcow2|qed)$/){
> +        return 1;
> +    }

I wonder whether we want to delegate this to the storage plugin via volume_has_feature? we already tell the plugin there which volname and whether the guest is running or not.. and for example, the base Plugin will return 1 when deleting a snapshot if the guest is running, or when resizing a volume, the RBDPlugin will return 1 when resizing if librbd is used and the guest is running..

might make more sense to have a good interface and give control to the plugin rather than special casing this here and excluding external plugins?

> +
> +    return;
> +}
> +
>  sub generate_format_blockdev {
>      my ($storecfg, $drive, $file, $snap, $nodename) = @_;
>  
> @@ -1272,6 +1299,37 @@ sub generate_format_blockdev {
>      return $blockdev;
>  }
>  
> +sub generate_backing_blockdev {
> +    my ($storecfg, $snapshots, $deviceid, $drive, $snap_id) = @_;

$deviceid is only used to pass it to the recursive invocation below..

> +
> +    my $snapshot = $snapshots->{$snap_id};
> +    my $parentid = $snapshot->{parent};
> +
> +    my $volid = $drive->{file};
> +
> +    my $snap_file_blockdev = generate_file_blockdev($storecfg, $drive, $snap_id);
> +    $snap_file_blockdev->{filename} = $snapshot->{file};
> +    $drive->{ro} = 1;
> +    my $snap_fmt_blockdev = generate_format_blockdev($storecfg, $drive, $snap_file_blockdev, $snap_id);
> +    $snap_fmt_blockdev->{backing} = generate_backing_blockdev($storecfg, $snapshots, $deviceid, $drive, $parentid) if $parentid;
> +    return $snap_fmt_blockdev;
> +}
> +
> +sub generate_backing_chain_blockdev {
> +    my ($storecfg, $deviceid, $drive) = @_;
> +
> +    my $volid = $drive->{file};
> +    my $do_snapshots_with_qemu = do_snapshots_with_qemu($storecfg, $volid, $deviceid);
> +    return if !$do_snapshots_with_qemu || $do_snapshots_with_qemu != 2;
> +
> +    my $chain_blockdev = undef;
> +    PVE::Storage::activate_volumes($storecfg, [$volid]);
> +    my $snapshots = PVE::Storage::volume_snapshot_info($storecfg, $volid);
> +    my $parentid = $snapshots->{'current'}->{parent};
> +    $chain_blockdev = generate_backing_blockdev($storecfg, $snapshots, $deviceid, $drive, $parentid) if $parentid;
> +    return $chain_blockdev;
> +}
> +
>  sub generate_drive_blockdev {
>      my ($storecfg, $vmid, $drive, $live_restore_name) = @_;
>  
> @@ -1293,6 +1351,8 @@ sub generate_drive_blockdev {
>  
>      my $blockdev_file = generate_file_blockdev($storecfg, $drive);
>      my $blockdev_format = generate_format_blockdev($storecfg, $drive, $blockdev_file);
> +    my $backing_chain  = generate_backing_chain_blockdev($storecfg, "drive-$drive_id", $drive);
> +    $blockdev_format->{backing} = $backing_chain if $backing_chain;
>  
>      my $blockdev_live_restore = undef;
>      if ($live_restore_name) {
> -- 
> 2.39.5


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