all lists on lists.proxmox.com
 help / color / mirror / Atom feed
From: "Fabian Grünbichler" <f.gruenbichler@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH v4 qemu-server 10/11] blockdev: add backing_chain support
Date: Wed, 2 Apr 2025 10:10:37 +0200 (CEST)	[thread overview]
Message-ID: <456274725.4388.1743581437085@webmail.proxmox.com> (raw)
In-Reply-To: <mailman.950.1741688968.293.pve-devel@lists.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


  reply	other threads:[~2025-04-02  8:11 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20250311102905.2680524-1-alexandre.derumier@groupe-cyllene.com>
2025-03-11 10:28 ` [pve-devel] [PATCH v4 pve-qemu 1/1] add block-commit-replaces option patch Alexandre Derumier via pve-devel
2025-03-11 10:28 ` [pve-devel] [PATCH v4 qemu-server 01/11] blockdev: cmdline: convert drive to blockdev syntax Alexandre Derumier via pve-devel
2025-03-11 10:28 ` [pve-devel] [PATCH v4 pve-storage 1/5] qcow2: add external snapshot support Alexandre Derumier via pve-devel
2025-04-01 13:50   ` Fabian Grünbichler
2025-04-02  8:01     ` DERUMIER, Alexandre via pve-devel
     [not found]     ` <0e2cd118f35aa8d4c410d362fea1a1b366df1570.camel@groupe-cyllene.com>
2025-04-02  8:28       ` Fabian Grünbichler
2025-04-03  4:27         ` DERUMIER, Alexandre via pve-devel
2025-03-11 10:28 ` [pve-devel] [PATCH v4 qemu-server 02/11] blockdev : convert qemu_driveadd && qemu_drivedel Alexandre Derumier via pve-devel
2025-03-11 10:28 ` [pve-devel] [PATCH v4 pve-storage 2/5] lvmplugin: add qcow2 snapshot Alexandre Derumier via pve-devel
2025-04-01 13:50   ` Fabian Grünbichler
2025-03-11 10:28 ` [pve-devel] [PATCH v4 qemu-server 03/11] replace qemu_block_set_io_throttle with qom-set throttlegroup limits Alexandre Derumier via pve-devel
2025-03-11 10:28 ` [pve-devel] [PATCH v4 pve-storage 3/5] storage: vdisk_free: remove external snapshots Alexandre Derumier via pve-devel
2025-04-01 13:50   ` Fabian Grünbichler
2025-04-07 11:02     ` DERUMIER, Alexandre via pve-devel
2025-04-07 11:29     ` DERUMIER, Alexandre via pve-devel
2025-03-11 10:28 ` [pve-devel] [PATCH v4 qemu-server 04/11] blockdev: vm_devices_list : fix block-query Alexandre Derumier via pve-devel
2025-04-02  8:10   ` Fabian Grünbichler
2025-04-11 17:32     ` DERUMIER, Alexandre via pve-devel
2025-03-11 10:28 ` [pve-devel] [PATCH v4 pve-storage 4/5] lvm: lvrename helper: allow path Alexandre Derumier via pve-devel
2025-04-01 13:50   ` Fabian Grünbichler
2025-03-11 10:28 ` [pve-devel] [PATCH v4 qemu-server 05/11] blockdev: convert cdrom media eject/insert Alexandre Derumier via pve-devel
2025-03-11 10:28 ` [pve-devel] [PATCH v4 pve-storage 5/5] lvm: add lvremove helper Alexandre Derumier via pve-devel
2025-04-01 13:50   ` Fabian Grünbichler
2025-03-11 10:29 ` [pve-devel] [PATCH v4 qemu-server 06/11] blockdev: block_resize: convert to blockdev Alexandre Derumier via pve-devel
2025-03-11 10:29 ` [pve-devel] [PATCH v4 qemu-server 07/11] blockdev: nbd_export: block-export-add : use drive-$id for nodename Alexandre Derumier via pve-devel
2025-03-11 10:29 ` [pve-devel] [PATCH v4 qemu-server 08/11] blockdev: convert drive_mirror to blockdev_mirror Alexandre Derumier via pve-devel
2025-03-11 10:29 ` [pve-devel] [PATCH v4 qemu-server 09/11] blockdev: change aio on target if io_uring is not default Alexandre Derumier via pve-devel
2025-03-11 10:29 ` [pve-devel] [PATCH v4 qemu-server 10/11] blockdev: add backing_chain support Alexandre Derumier via pve-devel
2025-04-02  8:10   ` Fabian Grünbichler [this message]
2025-03-11 10:29 ` [pve-devel] [PATCH v4 qemu-server 11/11] qcow2: add external snapshot support Alexandre Derumier via pve-devel
2025-04-02  8:10   ` Fabian Grünbichler
2025-04-03  4:51     ` DERUMIER, Alexandre via pve-devel
2025-04-04 11:31     ` DERUMIER, Alexandre via pve-devel
     [not found]     ` <3e516016a970e52e5a1014dbcd6cf9507581da74.camel@groupe-cyllene.com>
2025-04-04 11:37       ` Fabian Grünbichler
2025-04-04 13:02         ` DERUMIER, Alexandre via pve-devel

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=456274725.4388.1743581437085@webmail.proxmox.com \
    --to=f.gruenbichler@proxmox.com \
    --cc=pve-devel@lists.proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal