[pve-devel] [PATCH qemu-server 2/3] Include vmstate and unused volumes in foreach_volid

[Fabian Ebner]

On 16.04.20 14:54, Fabian Ebner wrote:
> and refactor the test_volid closure. Like this get_replicatable_volumes doesn't
> need a separate loop for unused volumes anymore. For get_vm_volumes, which is used
> for activation/deactivation of volumes at migration and deactivation in vm_stop_cleanup,
> includes those volumes now. For migration it's an improvement, because those volumes
> might need to be migrated and for vm_stop_cleanup it shouldn't hurt. The last user
> of foreach_volid is check_vm_disks_local used by migrate_vm_precondition,
> where information about the additional volumes doesn't hurt either.
> 
> Note that replicate is (still) set by default, so the behavior for
> get_replicatable_volumes for unused volumes should not change.
> 
> Hibernation vmstate files are now also included and recognized as 'is_vmstate'.
> The 'size' attribute will not be overwritten by subsequent iterations for the
> same volid anymore (a volid may appear both in the config and in snapshots),
> so the size from the current config is now preferred.
> 
> Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
> ---
> 
> Feel free to ignore this one until after the release if it's too
> big of a change/too much to review.
> 
>   PVE/QemuConfig.pm  |  7 -------
>   PVE/QemuMigrate.pm |  1 +
>   PVE/QemuServer.pm  | 33 +++++++++++++++++++++++----------
>   3 files changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
> index 7874081..240fc06 100644
> --- a/PVE/QemuConfig.pm
> +++ b/PVE/QemuConfig.pm
> @@ -162,13 +162,6 @@ sub get_replicatable_volumes {
>   
>       PVE::QemuServer::foreach_volid($conf, $test_volid);
>   
> -    # add 'unusedX' volumes to volhash
> -    foreach my $key (keys %$conf) {
> -	if ($key =~ m/^unused/) {
> -	    $test_volid->($conf->{$key}, { replicate => 1 });
> -	}
> -    }
> -
>       return $volhash;
>   }
>   
> diff --git a/PVE/QemuMigrate.pm b/PVE/QemuMigrate.pm
> index 2e2e430..bac4e67 100644
> --- a/PVE/QemuMigrate.pm
> +++ b/PVE/QemuMigrate.pm
> @@ -388,6 +388,7 @@ sub sync_disks {
>   	    return if $scfg->{shared};
>   
>   	    $local_volumes->{$volid}->{ref} = $attr->{referenced_in_config} ? 'config' : 'snapshot';
> +	    $local_volumes->{$volid}->{ref} = 'storage' if $attr->{is_unused};
>   
>   	    $local_volumes->{$volid}->{is_vmstate} = $attr->{is_vmstate} ? 1 : 0;
>   
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 5a42532..e9784b0 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -4292,39 +4292,52 @@ sub foreach_volid {
>       my $volhash = {};
>   
>       my $test_volid = sub {
> -	my ($volid, $is_cdrom, $replicate, $shared, $snapname, $size) = @_;
> +	my ($key, $drive, $snapname) = @_;
>   
> +	my $volid = $drive->{file};
>   	return if !$volid;
>   
>   	$volhash->{$volid}->{cdrom} //= 1;
> -	$volhash->{$volid}->{cdrom} = 0 if !$is_cdrom;
> +	$volhash->{$volid}->{cdrom} = 0 if !drive_is_cdrom($drive);
>   
> +	my $replicate = $drive->{replicate} // 1;
>   	$volhash->{$volid}->{replicate} //= 0;
>   	$volhash->{$volid}->{replicate} = 1 if $replicate;
>   
>   	$volhash->{$volid}->{shared} //= 0;
> -	$volhash->{$volid}->{shared} = 1 if $shared;
> +	$volhash->{$volid}->{shared} = 1 if $drive->{shared};
>   
>   	$volhash->{$volid}->{referenced_in_config} //= 0;
>   	$volhash->{$volid}->{referenced_in_config} = 1 if !defined($snapname);
>   
>   	$volhash->{$volid}->{referenced_in_snapshot}->{$snapname} = 1
>   	    if defined($snapname);
> -	$volhash->{$volid}->{size} = $size if $size;
> +
> +	my $size = $drive->{size};
> +	$volhash->{$volid}->{size} //= $size if $size;
> +
> +	$volhash->{$volid}->{is_vmstate} //= 0;
> +	$volhash->{$volid}->{is_vmstate} = 1 if $key eq 'vmstate';
> +
> +	$volhash->{$volid}->{is_unused} //= 0;
> +	$volhash->{$volid}->{is_unused} = 1 if $key =~ /^unused\d+$/;
>       };
>   
> -    PVE::QemuConfig->foreach_volume($conf, sub {
> +    my $include_opts = {
> +	extra_keys => ['vmstate'],
> +	include_unused => 1,
> +    };
> +
> +    PVE::QemuConfig->foreach_volume_full($conf, $include_opts, sub {
>   	my ($ds, $drive) = @_;
> -	$test_volid->($drive->{file}, drive_is_cdrom($drive), $drive->{replicate} // 1, $drive->{shared}, undef, $drive->{size});
> +	$test_volid->($ds, $drive);
>       });
>   
>       foreach my $snapname (keys %{$conf->{snapshots}}) {
>   	my $snap = $conf->{snapshots}->{$snapname};
> -	$test_volid->($snap->{vmstate}, 0, 1, 0, $snapname);
> -	$volhash->{$snap->{vmstate}}->{is_vmstate} = 1 if $snap->{vmstate};
> -	PVE::QemuConfig->foreach_volume($snap, sub {
> +	PVE::QemuConfig->foreach_volume_full($snap, $include_opts, sub {
>   	    my ($ds, $drive) = @_;
> -	    $test_volid->($drive->{file}, drive_is_cdrom($drive), $drive->{replicate} // 1, $drive->{shared}, $snapname);
> +	    $test_volid->($ds, $drive, $snapname);
>           });
>       }
>   
>
Small improvement to this patch I noticed later: the mini-closures 
calling test_volid can now be replaced by test_volid itself, i.e. the 
iteration calls become
PVE::QemuConfig->foreach_volume_full($conf, $include_opts, $test_volid);
and
PVE::QemuConfig->foreach_volume_full($snap, $include_opts, $test_volid, 
$snapname);
respectively.