Re: [pve-devel] [PATCH qemu-server v2 3/6] migration: fail when aliased volume is detected

[Fiona Ebner <f.ebner@proxmox.com>]

Am 12.05.23 um 14:40 schrieb Aaron Lauterer:
> Aliased volumes (referencing the same disk image multiple times) can
> lead to unexpected behavior in a migration.
> 
> Therefore, stop the migration in such a case.
> 
> The check works by comparing the path returned by the storage plugin.
> This means that we should be able to catch the common situations where
> it can happen:
> 
> * by referencing the same volid multiple times

This situation is not actually caught by the patch? If it's the same
volid, it will resolve to the same path, so it will just execute
$path_to_volid->{$path}->{$volid} = 1;
multiple times and there won't be any detection of multi-reference, or
what am I missing? Just incrementing the counter instead also doesn't
work, because e.g. multi-reference is fine when done by snapshots.

Maybe there should be a helper just for the currently attached disks?
E.g. iterate over the currently attached disks with
QemuConfig->foreach_volume() and for each volume, resolve the path and
count. If any path is referenced multiple times, complain. To not lose
the volids while counting, you could e.g. "count" with
push $path_to_volid->{$path}->@*, $volid;

The helper can then be called during migration additionally to your
current patch here.

And the helper can also be re-used during VM start (maybe only warn
there to allow for exotic use-cases?) and I'd expect it to already catch
most problematic situations there.

> @@ -397,6 +397,8 @@ sub scan_local_volumes {
>  	    die "owned by other VM (owner = VM $owner)\n"
>  		if !$owner || ($owner != $vmid);
>  
> +	    $path_to_volid->{$path}->{$volid} = 1;
> +
>  	    return if $attr->{is_vmstate};
>  
>  	    if (defined($snaprefs)) {
> @@ -443,6 +445,12 @@ sub scan_local_volumes {
>  	    }
>          });
>  
> +	for my $path (keys %$path_to_volid) {
> +	    my @volids = keys %{$path_to_volid->{$path}};
> +	    die "detected not supported aliased volumes: '" . join("', '", @volids) . "'"
> +		if (scalar @volids > 1);
> +	}
> +
>  	foreach my $vol (sort keys %$local_volumes) {
>  	    my $type = $replicatable_volumes->{$vol} ? 'local, replicated' : 'local';
>  	    my $ref = $local_volumes->{$vol}->{ref};