[pve-devel] [PATCH qemu-server 17/31] block job: add blockdev mirror

[Fiona Ebner]

Am 30.06.25 um 12:15 schrieb Fabian Grünbichler:
> On June 27, 2025 5:57 pm, Fiona Ebner wrote:
>> +    # Need to replace the node below the top node. This is not necessarily a format node, for
>> +    # example, it can also be a zeroinit node by a previous mirror! So query QEMU itself.
>> +    my $child_info = mon_cmd($vmid, 'block-node-query-file-child', 'node-name' => $device_id);
>> +    my $source_node_name = $child_info->{'node-name'};
> 
> isn't this semantically equivalent to get_node_name_below_throttle? that
> one does a few more checks and is slightly more expensive, but
> validating that the top node is a throttle node as expected might be a
> good thing here as well?
> 
> depending on how we see things, we might want to add a `$assert`
> parameter to that helper though for call sites that are only happening
> in blockdev context - to avoid the fallback in case the top node is not
> a throttle group, and instead die?

Yes, and I'll also add the assertion in v2.

>> +
>> +    # Copy original drive config (aio, cache, discard, ...):
>> +    my $dest_drive = dclone($source->{drive});
>> +    delete($dest_drive->{format}); # cannot use the source's format
>> +    $dest_drive->{file} = $dest->{volid};
>> +
>> +    my $generate_blockdev_opts = {};
>> +    $generate_blockdev_opts->{'zero-initialized'} = 1 if $dest->{'zero-initialized'};
>> +
>> +    # Note that if 'aio' is not explicitly set, i.e. default, it can change if source and target
>> +    # don't both allow or both not allow 'io_uring' as the default.
>> +    my $target_drive_blockdev = PVE::QemuServer::Blockdev::generate_drive_blockdev(
>> +        $storecfg, $dest_drive, $generate_blockdev_opts,
>> +    );
>> +    # Top node is the throttle group, must use the file child.
>> +    my $target_blockdev = $target_drive_blockdev->{file};
> 
> should we have an option for generate_drive_blockdev to skip the
> throttle group/top node? then we could just use Blockdev::attach here..
> 
> at least if we make that return the top-level node name or blockdev..

I thought about such an option, but decided against it in the end,
because this turned out to be the only user. Fleecing and TPM backup
have similar, but additional requirements. But that decision was made
before having the attach() helper. So reconsidering now, I feel like it
can be better for encapsulation and I'll go with this in v2.