hi,
On Thu, May 14, 2020 at 09:27:31AM +0200, Thomas Lamprecht wrote:
>
> please always include the bug/feature # somewhere as reference, e.g. a
> "fix #2255: ..." ideally in the subject, or at least in the commit message
> would be good.
will do for v2
>
> On 5/13/20 5:36 PM, Oguz Bektas wrote:
> > now we can add nvme drives;
> >
> > nvme0: local-lvm:vm-103-disk-0,size=32G
>
> An example I can use end to end is nicer for a reviewer, e.g., something like:
> qm set 100 --nvme0 local-lvm:32
>
>
> > max number is 8
>
> The "hows" and especially the "whys" are missing a bit here, I know them as I
> answered question to you directly during development of this, but in a month
> most is forgot, git commit messages are eternal ;)
>
> Nicer would be something like:
> "allow maximal 8 drives, most real hardware provides normally 1 to 3 slots and
> PCIe can host possibly a few more. For the default case 8 should be enough to
> mirror common HW, and more can be added easily if a real usecase comes up.
>
> Also, decreasing it later is impossible without breaking setups"
roger
>
>
> >
> > Signed-off-by: Oguz Bektas <o.bek...@proxmox.com>
> > ---
> > PVE/QemuServer.pm | 20 +++++++++++++++++---
> > PVE/QemuServer/Drive.pm | 21 +++++++++++++++++++++
> > 2 files changed, 38 insertions(+), 3 deletions(-)
> >
> > diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> > index dcf05df..441d209 100644
> > --- a/PVE/QemuServer.pm
> > +++ b/PVE/QemuServer.pm
> > @@ -406,7 +406,7 @@ EODESC
> > optional => 1,
> > type => 'string', format => 'pve-qm-bootdisk',
> > description => "Enable booting from specified disk.",
> > - pattern => '(ide|sata|scsi|virtio)\d+',
> > + pattern => '(ide|sata|scsi|virtio|nvme)\d+',
> > },
> > smp => {
> > optional => 1,
> > @@ -1424,7 +1424,11 @@ sub print_drivedevice_full {
> > $device .= ",rotation_rate=1";
> > }
> > $device .= ",wwn=$drive->{wwn}" if $drive->{wwn};
> > -
> > + } elsif ($drive->{interface} eq 'nvme') {
> > + my $maxdev = $PVE::QemuServer::Drive::MAX_NVME_DISKS;
>
> not used here
right
>
> > + my $path = $drive->{file};
> > + $drive->{serial} = "$drive->{interface}$drive->{index}"; # serial is
> > mandatory for nvme
>
> hmm, but this doesn't allows for users setting there own serial...
right, changed to your suggestion
> Maybe:
>
> $drive->{serial} //= "$drive->{interface}$drive->{index}";
>
>
> > + $device = "nvme,drive=drive-$drive->{interface}$drive->{index}";
> > } elsif ($drive->{interface} eq 'ide' || $drive->{interface} eq
> > 'sata') {
> > my $maxdev = ($drive->{interface} eq 'sata') ?
> > $PVE::QemuServer::Drive::MAX_SATA_DISKS : 2;
> > my $controller = int($drive->{index} / $maxdev);
> > @@ -2157,7 +2161,7 @@ sub parse_vm_config {
> > } else {
> > $key = 'ide2' if $key eq 'cdrom';
> > my $fmt = $confdesc->{$key}->{format};
> > - if ($fmt && $fmt =~ /^pve-qm-(?:ide|scsi|virtio|sata)$/) {
> > + if ($fmt && $fmt =~ /^pve-qm-(?:ide|scsi|virtio|sata|nvme)$/) {
> > my $v = parse_drive($key, $value);
> > if (my $volid = filename_to_volume_id($vmid, $v->{file},
> > $v->{media})) {
> > $v->{file} = $volid;
> > @@ -3784,7 +3788,17 @@ sub vm_deviceplug {
> > warn $@ if $@;
> > die $err;
> > }
> > + } elsif ($deviceid =~ m/^(nvme)(\d+)$/) {
> > +
> > + qemu_driveadd($storecfg, $vmid, $device);
> >
> > + my $devicefull = print_drivedevice_full($storecfg, $conf, $vmid,
> > $device, $arch, $machine_type);
> > + eval { qemu_deviceadd($vmid, $devicefull); };
> > + if (my $err = $@) {
> > + eval { qemu_drivedel($vmid, $deviceid); };
> > + warn $@ if $@;
> > + die $err;
> > + }
> > } elsif ($deviceid =~ m/^(net)(\d+)$/) {
> >
> > return undef if !qemu_netdevadd($vmid, $conf, $arch, $device,
> > $deviceid);
> > diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm
> > index f84333f..b8a553a 100644
> > --- a/PVE/QemuServer/Drive.pm
> > +++ b/PVE/QemuServer/Drive.pm
> > @@ -27,6 +27,7 @@
> > PVE::JSONSchema::register_standard_option('pve-qm-image-format', {
> >
> > my $MAX_IDE_DISKS = 4;
> > my $MAX_SCSI_DISKS = 31;
> > +my $MAX_NVME_DISKS = 8;
> > my $MAX_VIRTIO_DISKS = 16;
> > our $MAX_SATA_DISKS = 6;
> > our $MAX_UNUSED_DISKS = 256;
> > @@ -275,6 +276,20 @@ my $scsidesc = {
> > };
> > PVE::JSONSchema::register_standard_option("pve-qm-scsi", $scsidesc);
> >
> > +my $nvme_fmt = {
> > + %drivedesc_base,
> > + %ssd_fmt,
> > + %wwn_fmt,
> > +};
> > +
> > +my $nvmedesc = {
> > + optional => 1,
> > + type => 'string', format => $nvme_fmt,
> > + description => "Use volume as NVME disk (n is 0 to " .
> > ($MAX_NVME_DISKS -1) . ").",
> > +};
> > +
> > +PVE::JSONSchema::register_standard_option("pve-qm-nvme", $nvmedesc);
>
> format is registered but not used in any of the code, or is there some
> funky auto use or the like?
this was the first thing i added since i didn't know where to start, and
it seems like none of them are being called explicitly anywhere, so
probably somewhere they're autoused (ide_fmt, scsi_fmt, and so on)
i couldn't find where this happens but probably somewhere while parsing
the drive
>
> > +
> > my $sata_fmt = {
> > %drivedesc_base,
> > %ssd_fmt,
> > @@ -364,6 +379,11 @@ for (my $i = 0; $i < $MAX_SCSI_DISKS; $i++) {
> > $drivedesc_hash->{"scsi$i"} = $scsidesc;
> > }
> >
> > +for (my $i = 0; $i < $MAX_NVME_DISKS; $i++) {
> > + $drivedesc_hash->{"nvme$i"} = $nvmedesc;
> > +}
>
> OK, as all is like that, but we could transform it maybe to
>
> $drivedesc_hash->{"nvme$_"} for (0..$MAX_SCSI_DISKS);
>
> lines, made it compacter and more concise without losing readability.
> But nothing for this series, just got to my mind now.
ok maybe a followup then at the end of the series
>
> > +
> > +
> > for (my $i = 0; $i < $MAX_VIRTIO_DISKS; $i++) {
> > $drivedesc_hash->{"virtio$i"} = $virtiodesc;
> > }
> > @@ -380,6 +400,7 @@ sub valid_drive_names {
> > (map { "scsi$_" } (0 .. ($MAX_SCSI_DISKS - 1))),
> > (map { "virtio$_" } (0 .. ($MAX_VIRTIO_DISKS - 1))),
> > (map { "sata$_" } (0 .. ($MAX_SATA_DISKS - 1))),
> > + (map { "nvme$_" } (0 .. ($MAX_NVME_DISKS - 1))),
> > 'efidisk0');
> > }
> >
> >
>
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
