[libvirt] [PATCH 00/12] Add length (duration) params for iotune throttling

[John Ferlan]

On 09/23/2016 08:56 AM, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1349898
> 
> Do a little housekeeping and minor adjustments to existing code, then
> add the various "-length" options for the <iotune> code.
> 

I realized while working on code adding another <iotune> attribute that
I neglected to update/test 'virsh blkdeviotune $dom $device'. I had
checked that 'virsh dumpxml $dom' displayed the new values, but brain
cramped on blkdeviotune...

In any case, a simple enough task one would think, right?  Well as it
turns out, no so simple.

First off with these patches applied, I got the following:

# virsh blkdeviotune $dom $disk
error: Unable to get block I/O throttle parameters
error: internal error: nparams too large

#

Uh oh... Long story short, there are now 19 parameters, but only 16 are
allowed as a maximum (from src/remote/remote_protocol.x):

const REMOTE_DOMAIN_BLOCK_IO_TUNE_PARAMETERS_MAX = 16;

One would think "all" that would need to be done is change that 16 to
say 24 (or 32 for more room).

That led to a similar error.... Strange, until I discovered that the
virTypedParamsDeserialize call in remoteDomainGetBlockIoTune used the
wrong constant. So I changed that and fortunately my virsh works now -
displaying what I want.

BUT, again simple enough, right?  Well not so fast.  If I run an "older"
virsh talking to the newer libvirtd, I get:

# virsh blkdeviotune $dom $disk
error: Unable to get block I/O throttle parameters
error: Unable to decode message payload

#

Running that older virsh in a debugger, the failure occurs in
xdr_remote_domain_get_block_io_tune_ret as soon as the 17th element is
returned. I guess I'd expect that considering it's expecting at most 16
elements in it's xdr_array call.

I guess part of me didn't expect this - the virsh client was able to
allocate space for all 19 elements (due to the 0 nparams call being done
first). I can understand the size of the returned data buffer not
changing - that's not good, but if the client has done the right thing,
then shouldn't that max param in the xdr_array call be the size of the
buffer the client expects rather than the constant max?  At least for
the *Get* interfaces?

So before I spent too much more time on digging on this - is this
expected? (hopefully I've explained it well enough).

Does this parameter count have to stay constant forever? It's too bad we
didn't have the future vision that said there'd be more than 16 returned
values.

At this point I'm not sure what could be done from the (old) client
perspective. If getting the old client error is OK or expected, then I
can just go with the 24 or 32 for a BLOCK_IO_TUNE value. But I figured
I'd put this out there and guage feedback.


Tks -

John

FWIW: python client has similar phenomena

> 
> John Ferlan (12):
>   docs: Fix typo in libvirt-domain.h parameter description
>   include: Update description for <iotune> max params
>   tests: Add blkdeviotune-max xml2xmltest
>   qemu: Convert from shorthand to longer throttling names
>   qemu: Adjust how supportMaxOptions is used.
>   include: Add new definitions for duration for bps/iops throttling
>   caps: Add new capability for the bps/iops throttling length
>   qemu: Add length for bps/iops throttling parameters to driver
>   conf: Add a formatting macro for all the blkiotune values
>   conf: Adjust the PARSE_IOTUNE macro
>   conf: Add support for blkiotune "_length" options
>   qemu: Add the length options to the iotune command line
> 
>  docs/formatdomain.html.in                          |  40 +++++-
>  docs/schemas/domaincommon.rng                      |  38 ++++++
>  include/libvirt/libvirt-domain.h                   | 124 ++++++++++++++++--
>  src/conf/domain_conf.c                             | 142 +++++++++++----------
>  src/conf/domain_conf.h                             |   6 +
>  src/qemu/qemu_capabilities.c                       |   2 +
>  src/qemu/qemu_capabilities.h                       |   1 +
>  src/qemu/qemu_command.c                            |  96 ++++++--------
>  src/qemu/qemu_driver.c                             | 106 ++++++++++++++-
>  src/qemu/qemu_monitor.c                            |   7 +-
>  src/qemu/qemu_monitor.h                            |   3 +-
>  src/qemu/qemu_monitor_json.c                       |  72 ++++++-----
>  src/qemu/qemu_monitor_json.h                       |   3 +-
>  .../caps_2.6.0-gicv2.aarch64.xml                   |   1 +
>  .../caps_2.6.0-gicv3.aarch64.xml                   |   1 +
>  tests/qemucapabilitiesdata/caps_2.6.0.ppc64le.xml  |   1 +
>  tests/qemucapabilitiesdata/caps_2.6.0.x86_64.xml   |   1 +
>  tests/qemucapabilitiesdata/caps_2.7.0.x86_64.xml   |   1 +
>  tests/qemumonitorjsontest.c                        |  17 ++-
>  .../qemuxml2argv-blkdeviotune-max-length.args      |  34 +++++
>  .../qemuxml2argv-blkdeviotune-max-length.xml       |  65 ++++++++++
>  .../qemuxml2argv-blkdeviotune-max.args             |  10 +-
>  .../qemuxml2argv-blkdeviotune-max.xml              |  14 +-
>  .../qemuxml2argv-blkdeviotune.args                 |   5 +-
>  tests/qemuxml2argvtest.c                           |   4 +
>  .../qemuxml2xmlout-blkdeviotune-max-length.xml     |   1 +
>  .../qemuxml2xmlout-blkdeviotune-max.xml            |   1 +
>  tests/qemuxml2xmltest.c                            |   2 +
>  28 files changed, 616 insertions(+), 182 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max-length.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune-max-length.xml
>  create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-blkdeviotune-max-length.xml
>  create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-blkdeviotune-max.xml
>