Re: [PATCH qemu-server 2/4] fix #8209: clone disk: do not ignore failure to query size of target

["Elias Huhsovitz" <e.huhsovitz@proxmox.com>]

Comments inline

(This is not a full review yet)

On Thu Oct 22, 2026 at 11:38 AM CEST, Fiona Ebner wrote:
> Since commit 3bae384f ("clone disk: avoid errors after disk was moved
> by QEMU"), failure to query the target volume's size after clone is
> ignored. It's sensible not to fail hard at this stage, because in case
> of live mirror for the same VM with 'complete' completion mode, the
> drive was already switched. Also, the error might be a harmless
> timeout. Still, as reported in bug #8209, there could be an actual
> issue, so log a warning and communicate a failure to the caller, so it
> can avoid destructive action.
>
> Some storage plugin implementations of the volume_size_info() method
> do not fail with a hard error. In particular, the native file-based
> plugins use file_size_info() which returns undef upon failure. Thus,
> explicitly check the size.
>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  src/PVE/API2/Qemu.pm  | 21 ++++++++++++++-------
>  src/PVE/QemuServer.pm | 14 +++++++++++++-
>  2 files changed, 27 insertions(+), 8 deletions(-)
>
> diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
> index f4dbd2a1..8e7ece52 100644
> --- a/src/PVE/API2/Qemu.pm
> +++ b/src/PVE/API2/Qemu.pm
> @@ -432,7 +432,7 @@ my $import_from_volid = sub {
>          $cloned = $clonefn->();
>      }
>  
> -    return $cloned->@{qw(file size)};
> +    return $cloned->{drive}->@{qw(file size)};
>  };
>  
>  my sub prohibit_tpm_version_change {
> @@ -4703,7 +4703,7 @@ __PACKAGE__->register_method({
>                      $dest_info->{efisize} = PVE::QemuServer::get_efivars_size($oldconf)
>                          if $opt eq 'efidisk0';
>  
> -                    my $newdrive = PVE::QemuServer::clone_disk(
> +                    my $clone_result = PVE::QemuServer::clone_disk(
>                          $storecfg,
>                          $source_info,
>                          $dest_info,
> @@ -4715,7 +4715,7 @@ __PACKAGE__->register_method({
>                          $clonelimit,
>                      );
>  
> -                    $newconf->{$opt} = PVE::QemuServer::print_drive($newdrive);
> +                    $newconf->{$opt} = PVE::QemuServer::print_drive($clone_result->{drive});
>  
>                      PVE::QemuConfig->write_config($newid, $newconf);
>                      $i++;
> @@ -4900,6 +4900,7 @@ __PACKAGE__->register_method({
>          my $target_disk = extract_param($param, 'target-disk') // $disk;
>          my $storeid = extract_param($param, 'storage');
>          my $format = extract_param($param, 'format');
> +        my $do_delete = extract_param($param, 'delete');
>  
>          my $storecfg = PVE::Storage::config();
>  
> @@ -4932,7 +4933,7 @@ __PACKAGE__->register_method({
>                  $storecfg, $conf, $disk, $old_volid,
>              );
>              die "you can't move a disk with snapshots and delete the source\n"
> -                if $snapshotted && $param->{delete};
> +                if $snapshotted && $do_delete;
>  
>              return ($conf, $drive, $oldstoreid, $snapshotted);
>          };
> @@ -4986,7 +4987,7 @@ __PACKAGE__->register_method({
>                  $dest_info->{efisize} = PVE::QemuServer::get_efivars_size($conf)
>                      if $disk eq 'efidisk0';
>  
> -                my $newdrive = PVE::QemuServer::clone_disk(
> +                my $clone_result = PVE::QemuServer::clone_disk(
>                      $storecfg,
>                      $source_info,
>                      $dest_info,
> @@ -4997,9 +4998,15 @@ __PACKAGE__->register_method({
>                      $conf->{agent},
>                      $movelimit,
>                  );
> +                my $newdrive = $clone_result->{drive};
>                  $conf->{$disk} = PVE::QemuServer::print_drive($newdrive);
>  
> -                if (!$param->{delete}) {
> +                if ($do_delete && $clone_result->{'after-copy-error'}) {
> +                    warn "skipping delete of source because of error after copy\n";
> +                    $do_delete = 0;

Since we now set $clone_result->{'after-copy-error'}, if e.g., the
target NFS share returns an error, we set $do_delete = 0 to prevent the
source disk from being deleted.

> +                }
> +
> +                if (!$do_delete) {
>                      my $unused_key = PVE::QemuConfig->add_unused_volume($conf, $old_volid);

So now in the case that $clone_result->{'after-copy-error'} is set, we
always call add_unused_volume(). But add_unused_volume() can fail with

"Too many unused volumes - please delete them first.\n"

according to AbstractConfig::add_unused_volume()

Couldn't this kill the disk during a live migration, if we are unable to
add the unused volume?

In Drive.pm the maximum number for unused disks is set to:

our $MAX_UNUSED_DISKS = 256;

IMO being unable to add a new disk in a production system, might be a
real possibility.

>                      print("adding source volume '$old_volid' as '$unused_key' to config\n");
>                  }
> @@ -5030,7 +5037,7 @@ __PACKAGE__->register_method({
>                  die "storage migration failed: $err";
>              }
>  
> -            if ($param->{delete}) {
> +            if ($do_delete) {
>                  print("removing source volume '$old_volid' after successful copy\n");
>                  eval {
>                      PVE::Storage::deactivate_volumes($storecfg, [$old_volid]);
> diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
> index 1efeeb66..8f062466 100644
> --- a/src/PVE/QemuServer.pm
> +++ b/src/PVE/QemuServer.pm
> @@ -7929,14 +7929,26 @@ sub clone_disk {
>          }
>      }
>  
> +    my $result = {};
>      my $size = eval { PVE::Storage::volume_size_info($storecfg, $newvolid, 10) };
> +    my $err = $@;
> +    $err = "no size in result" if !$err && !$size;
> +    if ($err) {
> +        # The failure might be a harmless timeout. In case of live mirror for the same VM with
> +        # 'complete' completion mode, the drive was already switched. Thus, avoid a hard error at
> +        # this stage. Still, there could be an actual issue, so the caller can avoid destructive
> +        # action based on the presence of 'after-copy-error'.
> +        log_warn("unable to query size for '$newvolid' after copy - $err");
> +        $result->{'after-copy-error'} = 1;
> +    }
>  
>      my $disk = dclone($drive);
>      delete $disk->{format};
>      $disk->{file} = $newvolid;
>      $disk->{size} = $size if defined($size) && !$unused;
>  
> -    return $disk;
> +    $result->{drive} = $disk;
> +    return $result;
>  }
>  
>  sub qemu_use_old_bios_files {