Bug 1349898

Started working through the basics to add support for the new options to libvirt... I tripped across an issue though due to how libvirt currently generates the qemu command line.  I get the following error(s) if all I do is add support to have a {bps|iops}_*_length parameter on the command line:

... -drive file=/home/vm-images/f18,format=raw,if=none,id=drive-virtio-disk0,bps=5000,iops=6000,bps_max=10000,iops_max=11000,bps_max_length=3,iops_max_length=5: Block format 'raw' does not support the option 'iops_max_length'

...  -drive file=/home/vm-images/f18,format=raw,if=none,id=drive-virtio-disk0,bps_rd=5000,bps_wr=5500,iops_rd=3500,iops_wr=4000,bps_rd_max=6000,bps_wr_max=6500,iops_rd_max=7000,iops_wr_max=7500,iops_size=2000,bps_rd_max_length=3: Block format 'raw' does not support the option 'bps_rd_max_length'

The guest I have uses a top of tree build:

/home/qemu/x86_64-softmmu/qemu-system-x86_64 --version
QEMU emulator version 2.7.50 (v2.7.0-441-gf5edc82), Copyright (c) 2003-2016 Fabrice Bellard and the QEMU Project developers

I know when I added support for luks on the command line I had to prefix the (new) password-secret argument with "file." because libvirt uses a 'legacy' syntax of "-drive file=..." instead of a newer syntax of "-drive driver=%s,filename=..."

Similarly, I tried adding "throttling.{bps|iops}-*", "file.{bps|iops}-*", and "file.throttling.{bps|iops}-*" - none worked.

Alternatively, I note that blockdev.c/drive_new() seems to have some sort of alias for the command options. If I modify that to include the _length values and rebuild, then I can successfully start my domain with the _length parameters.  

+        { "iops_max_length",       "throttling.iops-total-max-length" },
+        { "iops_rd_max_length",    "throttling.iops-read-max-length" },
+        { "iops_wr_max_length",    "throttling.iops-write-max-length" },
+
+        { "bps_max_length",        "throttling.bps-total-max-length" },
+        { "bps_rd_max_length",     "throttling.bps-read-max-length" },
+        { "bps_wr_max_length",     "throttling.bps-write-max-length" },
+

So, does this mean libvirt is going to be "forced" to use the newer "-drive driver=%s,file=%s..." syntax? or is there some "syntax" that could work without this kind of change?

(In reply to John Ferlan from comment #2)
> Started working through the basics to add support for the new options to
> libvirt... I tripped across an issue though due to how libvirt currently
> generates the qemu command line.  I get the following error(s) if all I do
> is add support to have a {bps|iops}_*_length parameter on the command line:
> 
> ... -drive
> file=/home/vm-images/f18,format=raw,if=none,id=drive-virtio-disk0,bps=5000,
> iops=6000,bps_max=10000,iops_max=11000,bps_max_length=3,iops_max_length=5:
> Block format 'raw' does not support the option 'iops_max_length'
> 
> ...  -drive
> file=/home/vm-images/f18,format=raw,if=none,id=drive-virtio-disk0,
> bps_rd=5000,bps_wr=5500,iops_rd=3500,iops_wr=4000,bps_rd_max=6000,
> bps_wr_max=6500,iops_rd_max=7000,iops_wr_max=7500,iops_size=2000,
> bps_rd_max_length=3: Block format 'raw' does not support the option
> 'bps_rd_max_length'
> 
> The guest I have uses a top of tree build:
> 
> /home/qemu/x86_64-softmmu/qemu-system-x86_64 --version
> QEMU emulator version 2.7.50 (v2.7.0-441-gf5edc82), Copyright (c) 2003-2016
> Fabrice Bellard and the QEMU Project developers
> 
> I know when I added support for luks on the command line I had to prefix the
> (new) password-secret argument with "file." because libvirt uses a 'legacy'
> syntax of "-drive file=..." instead of a newer syntax of "-drive
> driver=%s,filename=..."
> 
> Similarly, I tried adding "throttling.{bps|iops}-*", "file.{bps|iops}-*",
> and "file.throttling.{bps|iops}-*" - none worked.
> 
> Alternatively, I note that blockdev.c/drive_new() seems to have some sort of
> alias for the command options. If I modify that to include the _length
> values and rebuild, then I can successfully start my domain with the _length
> parameters.  
> 
> +        { "iops_max_length",       "throttling.iops-total-max-length" },
> +        { "iops_rd_max_length",    "throttling.iops-read-max-length" },
> +        { "iops_wr_max_length",    "throttling.iops-write-max-length" },
> +
> +        { "bps_max_length",        "throttling.bps-total-max-length" },
> +        { "bps_rd_max_length",     "throttling.bps-read-max-length" },
> +        { "bps_wr_max_length",     "throttling.bps-write-max-length" },
> +
> 
> So, does this mean libvirt is going to be "forced" to use the newer "-drive
> driver=%s,file=%s..." syntax? or is there some "syntax" that could work
> without this kind of change?

There are no shortcuts for bps_max_length and iops_max_length.  There are no plans to add shortcuts for advanced options AFAIK.  The long form options should be used:

  qemu-system-x86_64 -enable-kvm -m 1024 -cpu host \
      -drive if=virtio,file=test.img,format=raw,\
             throttling.bps-total=N,\
             throttling.bps-total-max=M,\
             throttling.bps-total-max-length=L

Ahh.. OK...  Turns out it wasn't happy to handle intermixed alias and long hand. If I convert all the existing short hand to longer form and then add the -length parameters, things work.

I've posted patches upstream:

http://www.redhat.com/archives/libvir-list/2016-September/msg01090.html

A v2 was posted today:

http://www.redhat.com/archives/libvir-list/2016-October/msg00272.html

Some patches from v1 already pushed - v2 includes some things found during
debugging of setting the values from virsh (I forgot about virsh in v1).

This series is finally pushed:

commit 13022ce430da9d6ce4dc9e9117d6be519e7afc4a
Author: John Ferlan <jferlan>
Date:   Sun Oct 2 08:24:31 2016 -0400

    virsh: Add _length parameters to virsh output


$ git describe 13022ce430da9d6ce4dc9e9117d6be519e7afc4a
v2.3.0-202-g13022ce

Verified and PASSED

versions:
libvirt-3.2.0-6.el7.x86_64

steps:
1. check blkdeviotune output when getting info
## virsh blkdeviotune r vdb
...
total_bytes_sec_max_length: 0
read_bytes_sec_max_length: 0
write_bytes_sec_max_length: 0
total_iops_sec_max_length: 0
read_iops_sec_max_length: 0
write_iops_sec_max_length: 0


2. check blkdeviotune taking effect when setting info
## virsh blkdeviotune r vdb --total-bytes-sec 111 --total-bytes-sec-max 33333 --total-bytes-sec-max-length 555
## virsh dumpxml r | grep length
        <total_bytes_sec_max_length>555</total_bytes_sec_max_length>
-- restart vm --
## virsh blkdeviotune r vdb --read-bytes-sec 111 --read-bytes-sec-max 33333 --read-bytes-sec-max-length 555
## virsh dumpxml r | grep length
        <read_bytes_sec_max_length>555</read_bytes_sec_max_length>
-- restart vm --
## virsh blkdeviotune r vdb --write-bytes-sec 111 --write-bytes-sec-max 33333 --write-bytes-sec-max-length 555
## virsh dumpxml r | grep length
        <write_bytes_sec_max_length>555</write_bytes_sec_max_length>
-- restart vm --
## virsh blkdeviotune r vdb --total-iops-sec 111 --total-iops-sec-max 33333 --total-iops-sec-max-length 555
## virsh dumpxml r | grep length
        <total_iops_sec_max_length>555</total_iops_sec_max_length>
-- restart vm --
## virsh blkdeviotune r vdb --read-iops-sec 111 --read-iops-sec-max 33333 --read-iops-sec-max-length 555
## virsh dumpxml r | grep length
        <read_iops_sec_max_length>555</read_iops_sec_max_length>
-- restart vm --
## virsh blkdeviotune r vdb --write-iops-sec 111 --write-iops-sec-max 33333 --write-iops-sec-max-length 555
## virsh dumpxml r | grep length
        <write_iops_sec_max_length>555</write_iops_sec_max_length>

3. check setting with --config option and check vm's qemu cmd with existing settings
## virsh blkdeviotune r vdb --total-bytes-sec 111 --total-bytes-sec-max 33333 --total-bytes-sec-max-length 555 --config
## virsh dumpxml r | grep length
<=== nothing, as expected
## virsh dumpxml r --inactive| grep length
        <total_bytes_sec_max_length>555</total_bytes_sec_max_length>
<=== show up in inactive vm xml, as expected

## virsh destroy r
Domain r destroyed

## virsh start r
virsh dumDomain r started

## virsh dumpxml r | grep length
        <total_bytes_sec_max_length>555</total_bytes_sec_max_length>


## ps -ef | grep qemu | grep 555
qemu      6685     1 43 14:59 ?        00:00:16 /usr/libexec/qemu-kvm -name guest=r,...-drive file=/var/lib/libvirt/images/luks.disk,key-secret=virtio-disk1-luks-secret0,format=luks,if=none,id=drive-virtio-disk1,throttling.bps-total=111,throttling.bps-total-max=33333,throttling.bps-total-max-length=555 ...

... and this works for other parameters

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHEA-2017:1846

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHEA-2017:1846