[libvirt] [RFC PATCH] Use disable-modern=on for disk device='lun'
https://bugzilla.redhat.com/show_bug.cgi?id=1365823 For virtio-blk, scsi=on has been deprecated in virtio-1, so <disk type='block' device='lun'> no longer works with with a virtio-blk-pci device with machine types newer than 2.7: https://bugzilla.redhat.com/show_bug.cgi?id=1245453 http://git.qemu.org/?p=qemu.git;a=commitdiff;h=9a4c0e22 Add disable-modern=on on the QEMU command line to prolong the suffering for a while longer. --- The alternative would be using the QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY capability to report an error, instead of waiting until QEMU fails with an error, since it's the first commit when the following error should be reachable via libvirt-generated command line: http://git.qemu.org/?p=qemu.git;a=commitdiff;h=efb8206c src/qemu/qemu_command.c | 12 +++++ .../qemuxml2argv-virtio-lun-legacy.args | 30 +++++++++++++ .../qemuxml2argv-virtio-lun-legacy.xml | 52 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 3 ++ 4 files changed, 97 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-lun-legacy.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-lun-legacy.xml diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 051a0bc..03dc20e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2075,6 +2075,18 @@ qemuBuildDriveDevStr(const virDomainDef *def, virBufferAddLit(&opt, "virtio-blk-pci"); if (disk->iothread) virBufferAsprintf(&opt, ",iothread=iothread%u", disk->iothread); + + /* + * SCSI command passthrough was deprecated in virtio 1.0, + * see https://bugzilla.redhat.com/show_bug.cgi?id=1365823 + */ + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_BLK_SCSI) && + disk->device == VIR_DOMAIN_DISK_DEVICE_LUN && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY)) { + VIR_WARN("lun type devices are deprecated for virtio-blk-pci devices, " + "consider using disks on a virtio-scsi controller"); + virBufferAddLit(&opt, ",disable-modern=on"); + } } qemuBuildIoEventFdStr(&opt, disk->ioeventfd, qemuCaps); if (disk->event_idx && diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-lun-legacy.args b/tests/qemuxml2argvdata/qemuxml2argv-virtio-lun-legacy.args new file mode 100644 index 0000000..d1ff31c --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-lun-legacy.args @@ -0,0 +1,30 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu \ +-name test \ +-S \ +-M pc-0.13 \ +-m 1024 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid bba65c0e-c049-934f-b6aa-4e2c0582acdf \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-test/monitor.sock,server,nowait \ +-no-acpi \ +-boot dc \ +-device virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x6 \ +-usb \ +-drive file=/dev/sdfake,format=qcow2,if=none,id=drive-virtio-disk0 \ +-device virtio-blk-pci,disable-modern=on,scsi=on,bus=pci.0,addr=0x4,\ +drive=drive-virtio-disk0,id=virtio-disk0 \ +-drive file=/dev/sdfake2,format=qcow2,if=none,id=drive-virtio-disk1 \ +-device virtio-blk-pci,disable-modern=on,scsi=on,bus=pci.0,addr=0x5,\ +drive=drive-virtio-disk1,id=virtio-disk1 \ +-device virtio-net-pci,vlan=0,id=net0,mac=52:54:00:e5:48:58,bus=pci.0,addr=0x3 \ +-net user,vlan=0,name=hostnet0 \ +-serial pty \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x7 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-virtio-lun-legacy.xml b/tests/qemuxml2argvdata/qemuxml2argv-virtio-lun-legacy.xml new file mode 100644 index 0000000..63dbfda --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-virtio-lun-legacy.xml @@ -0,0 +1,52 @@ +<domain type='qemu'> + <name>test</name> + <uuid>bba65c0e-c049-934f-b6aa-4e2c0582acdf</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-0.13'>hvm</type> + <boot dev='cdrom'/> + <boot dev='hd'/> + <bootmenu enable='yes'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>restart</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='lun' rawio='yes'> + <driver name='qemu' type='qcow2'/> + <source dev='/dev/sdfake'/> + <target dev='vda' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </disk> + <disk type='block' device='lun'> + <driver name='qemu' type='qcow2'/> + <source dev='/dev/sdfake2'/> + <target dev='vdb' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='virtio-serial' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/> + </controller> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <interface type='user'> + <mac address='52:54:00:e5:48:58'/> + <model type='virtio'/> + <driver name='vhost' event_idx='off'/> + </interface> + <serial type='pty'> + <target port='0'/> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + </console> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index e854077..afe3073 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -931,6 +931,9 @@ mymain(void) QEMU_CAPS_VIRTIO_BLK_SCSI); DO_TEST("virtio-lun", QEMU_CAPS_VIRTIO_BLK_SCSI); + DO_TEST("virtio-lun-legacy", + QEMU_CAPS_VIRTIO_BLK_SCSI, + QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY); DO_TEST("disk-scsi-lun-passthrough", QEMU_CAPS_SCSI_BLOCK, QEMU_CAPS_SCSI_LSI, QEMU_CAPS_VIRTIO_SCSI); -- 2.7.3
On Tue, Sep 20, 2016 at 02:21:49PM +0200, Ján Tomko wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1365823
For virtio-blk, scsi=on has been deprecated in virtio-1, so <disk type='block' device='lun'> no longer works with with a virtio-blk-pci device with machine types newer than 2.7: https://bugzilla.redhat.com/show_bug.cgi?id=1245453 http://git.qemu.org/?p=qemu.git;a=commitdiff;h=9a4c0e22
Add disable-modern=on on the QEMU command line to prolong the suffering for a while longer. --- The alternative would be using the QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY capability to report an error, instead of waiting until QEMU fails with an error, since it's the first commit when the following error should be reachable via libvirt-generated command line: http://git.qemu.org/?p=qemu.git;a=commitdiff;h=efb8206c
My preference would be for us to come to some conclusion of the way we will represent disable-modern/disable-legacy in the XML before we try to fix this device=lun problem. In general I'm not a fan of silently changing configs behind the users back. IOW, if we have XML for setting disable-modern explicitly, then I would not want to see us silently setting disable-modern ourselves. Leave it upto the app to decide to set it if they really want to continue using virtio-blk instead of virtio-scsi.
src/qemu/qemu_command.c | 12 +++++ .../qemuxml2argv-virtio-lun-legacy.args | 30 +++++++++++++ .../qemuxml2argv-virtio-lun-legacy.xml | 52 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 3 ++ 4 files changed, 97 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-lun-legacy.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-lun-legacy.xml
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 051a0bc..03dc20e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2075,6 +2075,18 @@ qemuBuildDriveDevStr(const virDomainDef *def, virBufferAddLit(&opt, "virtio-blk-pci"); if (disk->iothread) virBufferAsprintf(&opt, ",iothread=iothread%u", disk->iothread); + + /* + * SCSI command passthrough was deprecated in virtio 1.0, + * see https://bugzilla.redhat.com/show_bug.cgi?id=1365823 + */ + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_BLK_SCSI) && + disk->device == VIR_DOMAIN_DISK_DEVICE_LUN && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY)) { + VIR_WARN("lun type devices are deprecated for virtio-blk-pci devices, " + "consider using disks on a virtio-scsi controller");
Adding warnings like this is fairly pointless since they've invisible to the person who actually needs to see them (the app developer), and annoying to the person who does see them (the host administrator).
+ virBufferAddLit(&opt, ",disable-modern=on"); + } }
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 09/20/2016 08:57 AM, Daniel P. Berrange wrote:
On Tue, Sep 20, 2016 at 02:21:49PM +0200, Ján Tomko wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1365823
For virtio-blk, scsi=on has been deprecated in virtio-1, so <disk type='block' device='lun'> no longer works with with a virtio-blk-pci device with machine types newer than 2.7: https://bugzilla.redhat.com/show_bug.cgi?id=1245453 http://git.qemu.org/?p=qemu.git;a=commitdiff;h=9a4c0e22
Add disable-modern=on on the QEMU command line to prolong the suffering for a while longer. --- The alternative would be using the QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY capability to report an error, instead of waiting until QEMU fails with an error, since it's the first commit when the following error should be reachable via libvirt-generated command line: http://git.qemu.org/?p=qemu.git;a=commitdiff;h=efb8206c My preference would be for us to come to some conclusion of the way we will represent disable-modern/disable-legacy in the XML before we try to fix this device=lun problem.
Another open question is whether or not this is a problem if you start an existing domain that uses a pre-virtio-1.0 machinetype but you're using a qemu binary that does have virtio-1.0. I'm guessing that it should work with no extra "disable-modern=on" required (that's the whole point of versioned machinetypes after all). If so, then existing configs will continue to work properly with no change, and for new configs that use virtio-1.0-capable machinetypes maybe it will be more merciful to immediately complain when somebody tries to use device='lun', thus forcing people who want the "lun" functionality to switch to virtio-scsi right away, rather than propagating its use with virtio-blk-pci even further into the future (which of course will lead to it living *forever*). Trying to restate it more clearly: the problem is that the only way for libvirt to detect whether or not qemu is going to fail is by querying for presence of virtio-1.0 (which we do indirectly by looking for "disable-legacy" in the options), and that only tells us whether or not the qemu binary supports it, *not* whether a particular machinetype supported by the qemu binary supports it. The same binary might support several machinetypes that support virtio-1.0 (and thus don't support device='lun' without adding in disable-modern=on) and several machinetypes that *don't* support virtio-1.0 (and thus should have no problem with device='lun'), and libvirt has no way of knowing which is which, so libvirt *can't* be the one to complain about this, or even to automatically add the "disable-modern=on" (what happens if you add "disable-modern=on" to the virtio-blk-pci device of a machinetype that is too old for virtio-1.0? Is it ignored? Or does qemu give an error and exit? I haven't tried it, so don't know). In the end, until qemu provides us with a way to query capabilities *per machinetype*, there's realistically nothing libvirt can do about this problem that isn't going to be wrong for somebody. (Hi Eduardo!! :-) TL;DR - I've actually come to the opinion that the only right thing for libvirt to do here is "nothing" (unless you want to write code to try and interpret the qemu error message after the fact.... Ewwwww, never mind, scrap that!)
In general I'm not a fan of silently changing configs behind the users back. IOW, if we have XML for setting disable-modern explicitly, then I would not want to see us silently setting disable-modern ourselves. Leave it upto the app to decide to set it if they really want to continue using virtio-blk instead of virtio-scsi.
src/qemu/qemu_command.c | 12 +++++ .../qemuxml2argv-virtio-lun-legacy.args | 30 +++++++++++++ .../qemuxml2argv-virtio-lun-legacy.xml | 52 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 3 ++ 4 files changed, 97 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-lun-legacy.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-lun-legacy.xml
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 051a0bc..03dc20e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2075,6 +2075,18 @@ qemuBuildDriveDevStr(const virDomainDef *def, virBufferAddLit(&opt, "virtio-blk-pci"); if (disk->iothread) virBufferAsprintf(&opt, ",iothread=iothread%u", disk->iothread); + + /* + * SCSI command passthrough was deprecated in virtio 1.0, + * see https://bugzilla.redhat.com/show_bug.cgi?id=1365823 + */ + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_BLK_SCSI) && + disk->device == VIR_DOMAIN_DISK_DEVICE_LUN && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_PCI_DISABLE_LEGACY)) { + VIR_WARN("lun type devices are deprecated for virtio-blk-pci devices, " + "consider using disks on a virtio-scsi controller"); Adding warnings like this is fairly pointless since they've invisible to the person who actually needs to see them (the app developer), and annoying to the person who does see them (the host administrator).
To be fair, that's assuming it was an application that silently added "device='lun'" rather than a user explicitly requesting it. Still, even though it was me who originally forced poor Jan to add the warning, the more I think about the details of qemu binary capabilities vs. capabilities of specific versions of machinetypes supported by that qemu binary, the more I realize that it's pointless for libvirt to report this particular warning or try to correct for the problem, because the chances are very high that it will be wrong about it :-/
+ virBufferAddLit(&opt, ",disable-modern=on"); + } } Regards, Daniel
participants (3)
-
Daniel P. Berrange -
Ján Tomko -
Laine Stump