Re: [PATCH 2/2] qemuDomainSetBlockIoTune: Skip monitor call for empty cdrom

[Han Han <hhan@xxxxxxxxxx>]

On Thu, Jan 7, 2021 at 5:23 PM Peter Krempa <pkrempa@xxxxxxxxxx> wrote:

Similarly to startup of the VM qemu doesn't like setting throttling for
an empty drive. Just skip it since we do the correct thing once new
media is inserted.

Resolves: https://gitlab.com/libvirt/libvirt/-/issues/117
Signed-off-by: Peter Krempa <pkrempa@xxxxxxxxxx>
---
CC: Han Han <hhan@xxxxxxxxxx>

Please test this commit since you have the environment prepared.

 src/qemu/qemu_driver.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 1283e61785..027617deef 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -16301,16 +16301,23 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,

 #undef CHECK_MAX

-        qemuDomainObjEnterMonitor(driver, vm);
-        ret = qemuMonitorSetBlockIoThrottle(priv->mon, drivealias, qdevid,
-                                            &info, supportMaxOptions,
-                                            set_fields & QEMU_BLOCK_IOTUNE_SET_GROUP_NAME,
-                                            supportMaxLengthOptions);
-        if (qemuDomainObjExitMonitor(driver, vm) < 0)
+        /* blockdev-based qemu doesn't want to set the throttling when a cdrom
+         * is empty. Skip the monitor call here since we will set the throttling
+         * once new media is inserted */
+        if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKDEV) ||
+            !virStorageSourceIsEmpty(disk->src)) {
+
+            qemuDomainObjEnterMonitor(driver, vm);
+            ret = qemuMonitorSetBlockIoThrottle(priv->mon, drivealias, qdevid,
+                                                &info, supportMaxOptions,
+                                                set_fields & QEMU_BLOCK_IOTUNE_SET_GROUP_NAME,
+                                                supportMaxLengthOptions);
+            if (qemuDomainObjExitMonitor(driver, vm) < 0)
+                ret = -1;
+            if (ret < 0)
+                goto endjob;
             ret = -1;
-        if (ret < 0)
-            goto endjob;
-        ret = -1;
+        }

         virDomainDiskSetBlockIOTune(disk, &info);

--
2.29.2

Test on libvirt-v6.10.0-335-gc5904929e8 qemu-5.2.0-4.fc34.x86_64:

Start a VM with empty cdrom:

<disk type="file" device="cdrom">
      <driver name="qemu"/>
      <target dev="sda" bus="scsi"/>
      <readonly/>
      <alias name="scsi0-0-0"/>
      <address type="drive" controller="0" bus="0" target="0" unit="0"/>
    </disk>

Set the blkiotune for the cdrom:
➜  ~ virsh blkdeviotune test sda --total-bytes-sec 10000                                  

➜  ~ virsh blkdeviotune test sda --group-name test    

Dumpxml:

<disk type="file" device="cdrom">
      <driver name="qemu"/>
      <target dev="sda" bus="scsi"/>
      <iotune>
        <total_bytes_sec>10000</total_bytes_sec>
        <group_name>test</group_name>
      </iotune>
      <readonly/>
      <alias name="scsi0-0-0"/>
      <address type="drive" controller="0" bus="0" target="0" unit="0"/>
    </disk>

Insert the media:
➜  ~ virsh change-media test sda --insert /var/lib/libvirt/images/boot.iso
Successfully inserted media.

QMP log:

 56.306 > 0x7f8b04043030 {"execute":"blockdev-open-tray","arguments":{"id":"scsi0-0-0","force":false},"id":"libvirt-379"}
 56.310 ! 0x7f8b04043030 {"timestamp": {"seconds": 1610013536, "microseconds": 856358}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "", "tray-open": true, "id": "scsi0-0-0"}}
 56.311 < 0x7f8b04043030 {"return": {}, "id": "libvirt-379"}
 56.311 > 0x7f8b04043030 {"execute":"blockdev-remove-medium","arguments":{"id":"scsi0-0-0"},"id":"libvirt-380"}
 56.314 < 0x7f8b04043030 {"return": {}, "id": "libvirt-380"}
 56.314 > 0x7f8b04043030 {"execute":"blockdev-add","arguments":{"driver":"file","filename":"/var/lib/libvirt/images/boot.iso","node-name":"libvirt-3-storage","auto-read-only":true,"discard":"unmap"},"id":"libvirt-381"}
 56.318 < 0x7f8b04043030 {"return": {}, "id": "libvirt-381"}
 56.319 > 0x7f8b04043030 {"execute":"blockdev-add","arguments":{"node-name":"libvirt-3-format","read-only":true,"driver":"raw","file":"libvirt-3-storage"},"id":"libvirt-382"}
 56.322 < 0x7f8b04043030 {"return": {}, "id": "libvirt-382"}
 56.322 > 0x7f8b04043030 {"execute":"blockdev-insert-medium","arguments":{"id":"scsi0-0-0","node-name":"libvirt-3-format"},"id":"libvirt-383"}
 56.325 < 0x7f8b04043030 {"return": {}, "id": "libvirt-383"}
 56.325 > 0x7f8b04043030 {"execute":"block_set_io_throttle","arguments":{"id":"scsi0-0-0","bps":10000,"bps_rd":0,"bps_wr":0,"iops":0,"iops_rd":0,"iops_wr":0,"bps_max":0,"bps_rd_max":0,"bps_wr_max":0,"iops_max":0,"iops_rd_max":0,"iops_wr_max":0,"iops_size":0,"group":"test"},"id":"libvirt-384"}
 56.331 < 0x7f8b04043030 {"return": {}, "id": "libvirt-384"}
 56.331 > 0x7f8b04043030 {"execute":"blockdev-close-tray","arguments":{"id":"scsi0-0-0"},"id":"libvirt-385"}
 56.333 ! 0x7f8b04043030 {"timestamp": {"seconds": 1610013536, "microseconds": 880060}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "", "tray-open": false, "id": "scsi0-0-0"}}
 56.333 < 0x7f8b04043030 {"return": {}, "id": "libvirt-385"}

So the total_bytes_sec and group_name are set after medium inserted.

Eject the media:

➜  ~ virsh change-media test sda --eject                                  
Successfully ejected media.

Dumpxml:

<disk type="file" device="cdrom">
      <driver name="qemu" type="raw"/>
      <source index="4"/>
      <target dev="sda" bus="scsi"/>
      <iotune>
        <total_bytes_sec>10000</total_bytes_sec>
        <group_name>test</group_name>
      </iotune>
      <readonly/>
      <alias name="scsi0-0-0"/>
      <address type="drive" controller="0" bus="0" target="0" unit="0"/>
    </disk>

Work as expected.

--

Tested-by: Han Han <hhan@xxxxxxxxxx>