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

[Han Han]

On Thu, Jan 7, 2021 at 5:23 PM Peter Krempa <pkre...@redhat.com> 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 <pkre...@redhat.com>
> ---
> CC: Han Han <h...@redhat.com>
>
> 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 <h...@redhat.com>