Re: [pve-devel] [PATCH v3 qemu-server 01/11] blockdev: cmdline: convert drive to blockdev syntax

["DERUMIER, Alexandre via pve-devel" <pve-devel@lists.proxmox.com>]

[-- Attachment #1: Type: message/rfc822, Size: 19012 bytes --]

From: "DERUMIER, Alexandre" <alexandre.derumier@groupe-cyllene.com>
To: "pve-devel@lists.proxmox.com" <pve-devel@lists.proxmox.com>, "f.gruenbichler@proxmox.com" <f.gruenbichler@proxmox.com>
Subject: Re: [pve-devel] [PATCH v3 qemu-server 01/11] blockdev: cmdline: convert drive to blockdev syntax
Date: Fri, 10 Jan 2025 13:50:33 +0000
Message-ID: <b4cfffe2a2bfe601affef4f5aab63f6beb72cb97.camel@groupe-cyllene.com>


> - $device .= ",drive=drive-$drive_id,id=$drive_id";
> + $device .= ",id=$drive_id";
> + $device .= ",drive=drive-$drive_id" if $device_type ne 'cd' ||
> $drive->{file} ne 'none';

>>is this just because you remove the whole drive when ejecting? not
>>sure whether that is really needed..

with blockdev, no drive (no disc inserted in the cdrom device), it's
really no blockdev defined.
So we don't pass drive/cdrom media to the cdrom device.


>  
> -sub print_drive_commandline_full {
> -    my ($storecfg, $vmid, $drive, $live_restore_name, $io_uring) =
> @_;
> +sub print_drive_throttle_group {
> +    my ($drive) = @_;
> +    #command line can't use the structured json limits option,
> +    #so limit params need to use with x- as it's unstable api

>>this comment should be below the early return, or above the whole
>>sub.
ok

> +    return if drive_is_cdrom($drive) && $drive->{file} eq 'none';

>>is this needed if we keep empty cdrom drives around like before? I
>>know throttling practically makes no sense in that case, but it might
>>make the code in general more simple?

yes, this is to keep-it like before, but I can put it behind a
throttle-group, no problem. 

> 
> +sub generate_file_blockdev {
> +    my ($storecfg, $drive, $nodename) = @_;
> +
> +    my $volid = $drive->{file};
>      my ($storeid, $volname) = PVE::Storage::parse_volume_id($volid,
> 1);
> -    my $scfg = $storeid ? PVE::Storage::storage_config($storecfg,
> $storeid) : undef;
>  
> -    if (drive_is_cdrom($drive)) {
> - $path = get_iso_path($storecfg, $vmid, $volid);
> - die "$drive_id: cannot back cdrom drive with a live restore
> image\n" if $live_restore_name;
> +    my $scfg = undef;
> +    my $path = $volid;

I think this should only happen if the parse_volume_id above told us
this is an absolute path and not a PVE-managed volume..

> +    if($storeid && $storeid ne 'nbd') {

>>this is wrong.. I guess it's also somewhat wrong in the old
>>qemu_drive_mirror code.. we should probably check using a more
>>specific RE that the "volid" is an NBD URI, and not attempt to parse
>>it as a regular volid in that case..

ok. I'm already parsing the nbd uri later, I'll adapt the code.




> +    my $format = $drive->{format};
> +    $format //= "raw";

>>the format handling here is very sensitive, and I think this broke
>>it. see the big comment this patch removed ;)
>>
>>short summary: for PVE-managed volumes we want the format from the
>>storage layer (via checked_volume_format). if the drive has a format
>>set that disagrees, that is a hard error. for absolute paths we us
>>the format from the drive with a fallback to raw.

yes, I have seen the commits during my rebase before sending patches.
I'll fix that.


>  
> -    if ($live_restore_name) {
> - $format = "rbd" if $is_rbd;
> - die "$drive_id: Proxmox Backup Server backed drive cannot auto-
> detect the format\n"
> -     if !$format;
> - $opts .= ",format=alloc-track,file.driver=$format";
> -    } elsif ($format) {
> - $opts .= ",format=$format";
> +    my ($storeid, $volname) = PVE::Storage::parse_volume_id($volid,
> 1);

>>so I guess this should never be called with nbd-URI-volids?

until we want to live restore to an nbd uri, no ^_^


> +    my $readonly = defined($drive->{ro}) || $force_readonly ?
> JSON::true : JSON::false;
> +
> +    #libvirt define cache option on both format && file
>      my $cache_direct = drive_uses_cache_direct($drive, $scfg);
> +    my $cache = {};
> +    $cache->{direct} = $cache_direct ? JSON::true : JSON::false;
> +    $cache->{'no-flush'} = $drive->{cache} && $drive->{cache} eq
> 'unsafe' ? JSON::true : JSON::false;

>>so we have the same code in two places? should probably be a helper
>>then to not have them go out of sync..

Ah, yes, forgot to do the helper. Libvirt define it at both file &&
format blockdev, not sure why exactly,.


> 
> -    # my $file_param = $live_restore_name ? "file.file.filename" :
> "file";
> -    my $file_param = "file";
> +    my $file_nodename = "file-drive-$drive_id";
> +    my $blockdev_file = generate_file_blockdev($storecfg, $drive,
> $file_nodename);
> +    my $fmt_nodename = "fmt-drive-$drive_id";
> +    my $blockdev_format = generate_format_blockdev($storecfg,
> $drive, $fmt_nodename, $blockdev_file, $force_readonly);
> +
> +    my $blockdev_live_restore = undef;
>      if ($live_restore_name) {
> - # non-rbd drivers require the underlying file to be a separate
> block
> - # node, so add a second .file indirection
> - $file_param .= ".file" if !$is_rbd;
> - $file_param .= ".filename";
> +        die "$drive_id: Proxmox Backup Server backed drive cannot
> auto-detect the format\n"
> +            if !$format;

>>for this check, but it is not actually set anywhere here.. so is
>>something missing or can the check go?

can be remove, this is the older code that I forget to remove.
(I don't have tested the backup/restore yet, ad backup is not working)

> 



[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel