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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list