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:

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:

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:
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", 
+            /*
+             * 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");
+            }

libvir-list mailing list

Reply via email to