[v1] Support throttle block filters

[PATCH RFC 00/12] Support throttle block filters

Posted by wucf@linux.ibm.com 3 weeks, 1 day ago

From: Chun Feng Wu <wucf@linux.ibm.com>

Hi,

I am thinking to leverage "throttle block filter" in QEMU to support more flexible I/O limits(e.g. tiered I/O groups), one sample provided by QEMU doc is:
https://github.com/qemu/qemu/blob/master/docs/throttle.txt
"For example, let's say that we have three different drives and we want to set I/O limits for
each one of them and an additional set of limits for the combined I/O of all three drives."

The implementation idea is to 
- Define throttle groups(limit) in domain
- Define throttle filter to reference throttle group within disk
- Within domain disk, throttle filters references multiple throttle groups to form filter chain to apply multiple limits in QEMU like above sample
- Add new virsh cmds for throttle group management:
    throttlegroupset               Add or update a throttling group.
    throttlegroupdel               Delete a throttling group.
    throttlegroupinfo              Get a throttling group.
    throttlegrouplist              list all domain throttlegroups
- Update "attach-disk" to add one more option "--throttle-groups" to apply throttle filters e.g. "virsh attach-disk $VM_ID ${DISK_PATH}/vm1_disk_2.qcow2 vdd --driver qemu --subdriver qcow2 --targetbus virtio --throttle-groups limit2,limit012"
- I chose above semantics as I felt they're appropriate, if there are better ones please kindly suggest.

This patchset includes:
- Throttle group and throttle filter definition in patch 1
- New QMP processing to update and get throttle group in patch 2
- New API definition and implementation in patch 3
- Hotplug and qemuProcessLaunch flow implemenation in patch 4, 5
- Domain XML schema and doc(formatdomain.rst) change in patch 6
- Tests in patch 7, 8
- Virsh cmd implementation in patch 9
- Other enhencement/verification implementation in patch 10, 11, 12

From QMP perspective, the sample flow works this way: 
- Throttle group creation:

virsh qemu-monitor-command 1 '{"execute":"object-add", "arguments":{"qom-type":"throttle-group","id":"limit0","limits":{"iops-total":200,"iops-read":0,"iops-total-max":200,"iops-total-max-length":1}}}'

virsh qemu-monitor-command 1 '{"execute":"object-add", "arguments":{"qom-type":"throttle-group","id":"limit1","limits":{"iops-total":250,"iops-read":0,"iops-total-max":250,"iops-total-max-length":1}}}'

virsh qemu-monitor-command 1 '{"execute":"object-add", "arguments":{"qom-type":"throttle-group","id":"limit2","limits":{"iops-total":300,"iops-read":0,"iops-total-max":300,"iops-total-max-length":1}}}'

virsh qemu-monitor-command 1 '{"execute":"object-add", "arguments":{"qom-type":"throttle-group","id":"limit012","limits":{"iops-total":400,"iops-read":0,"iops-total-max":400,"iops-total-max-length":1}}}'

- Chain up filters during attaching disk to apply two filters(limit0 and limit012):

virsh qemu-monitor-command 1 '{"execute":"blockdev-add", "arguments":  {"driver":"file","filename":"/virt/disks/vm1_disk_1.qcow2","node-name":"test-3-storage","auto-read-only":true,"discard":"unmap"}}'
  
virsh qemu-monitor-command 1 '{"execute":"blockdev-add", "arguments":{"node-name":"test-3-format","read-only":false,"driver":"qcow2","file":"test-3-storage","backing":null}}'
  
virsh qemu-monitor-command 1 '{"execute":"blockdev-add", "arguments":{"driver":"throttle","node-name":"libvirt-5-filter","throttle-group": "limit0","file":"test-3-format"}}'

virsh qemu-monitor-command 1 '{"execute":"blockdev-add", "arguments": {"driver":"throttle","node-name":"libvirt-6-filter","throttle-group":"limit012","file":"libvirt-5-filter"}}'

virsh qemu-monitor-command 1 '{"execute": "device_add", "arguments": {"driver":"virtio-blk-pci","scsi":false,"bus":"pci.0","addr":"0x5","drive":"libvirt-6-filter","id":"virtio-disk1"}}'


BTW, I also got support from these guys(guyujie@linux.ibm.com, wuyx@linux.ibm.com, xinhaong@linux.ibm.com, commits under their names are included) to help do some enhancement/verification implementation, thanks a lot!

Any comments/suggestions will be appriciated!



Chun Feng Wu (9):
  config: Introduce ThrottleGroup and ThrottleFilter
  qemu: monitor: Add support for ThrottleGroup operations
  remote: New APIs for ThrottleGroup lifecycle management
  qemu: hotplug: Support hot attach block disk along with throttle
    filters
  qemu: command: Support throttle groups and filters during
    qemuProcessLaunch
  schema: Add new domain elements to support multiple throttle filters
  test: Test throttle group lifecycle APIs
  tests: Test qemuMonitorJSONGetThrottleGroup and
    qemuMonitorJSONUpdateThrottleGroup
  virsh: Add support for throttle group operations

Hao Ning Xin (1):
  config: validate: Verify throttle group fields

Yan Xiu Wu (1):
  config: validate: Use "iotune" and "throttlefilters" exclusively for
    specific disk

Yu Jie Gu (1):
  remote: Define remote_domain_set_throttle_group_args and adjust
    throttle structs sequence

 docs/formatdomain.rst                         |  48 ++
 include/libvirt/libvirt-domain.h              |  29 +
 src/conf/domain_conf.c                        | 364 ++++++++++++
 src/conf/domain_conf.h                        |  43 ++
 src/conf/domain_validate.c                    | 115 ++--
 src/conf/schemas/domaincommon.rng             | 164 +++++-
 src/conf/virconftypes.h                       |   4 +
 src/driver-hypervisor.h                       |  22 +
 src/libvirt-domain.c                          | 188 +++++++
 src/libvirt_private.syms                      |   9 +
 src/libvirt_public.syms                       |   7 +
 src/qemu/qemu_block.c                         | 121 ++++
 src/qemu/qemu_block.h                         |  53 ++
 src/qemu/qemu_command.c                       | 148 +++++
 src/qemu/qemu_command.h                       |   9 +
 src/qemu/qemu_domain.c                        |  79 +++
 src/qemu/qemu_domain.h                        |  10 +
 src/qemu/qemu_driver.c                        | 520 +++++++++++++++++
 src/qemu/qemu_hotplug.c                       |  23 +
 src/qemu/qemu_monitor.c                       |  29 +
 src/qemu/qemu_monitor.h                       |  10 +
 src/qemu/qemu_monitor_json.c                  | 135 +++++
 src/qemu/qemu_monitor_json.h                  |  13 +
 src/remote/remote_daemon_dispatch.c           |  60 ++
 src/remote/remote_driver.c                    |  46 ++
 src/remote/remote_protocol.x                  |  50 +-
 src/remote_protocol-structs                   |  30 +
 src/test/test_driver.c                        | 380 +++++++++++++
 tests/qemumonitorjsontest.c                   |  89 +++
 .../qemustatusxml2xmldata/backup-pull-in.xml  |   1 +
 .../blockjob-blockdev-in.xml                  |   1 +
 .../blockjob-mirror-in.xml                    |   1 +
 .../migration-in-params-in.xml                |   1 +
 .../migration-out-nbd-bitmaps-in.xml          |   1 +
 .../migration-out-nbd-out.xml                 |   1 +
 .../migration-out-nbd-tls-out.xml             |   1 +
 .../migration-out-params-in.xml               |   1 +
 tests/qemustatusxml2xmldata/modern-in.xml     |   1 +
 tests/qemustatusxml2xmldata/upgrade-out.xml   |   1 +
 .../qemustatusxml2xmldata/vcpus-multi-in.xml  |   1 +
 tools/virsh-completer-domain.c                |  62 ++
 tools/virsh-completer-domain.h                |  11 +
 tools/virsh-domain.c                          | 530 ++++++++++++++++++
 43 files changed, 3376 insertions(+), 36 deletions(-)

-- 
2.34.1
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

Re: [PATCH RFC 00/12] Support throttle block filters

Posted by Peter Krempa 3 weeks, 1 day ago

On Tue, Apr 09, 2024 at 20:13:08 -0700, wucf@linux.ibm.com wrote:
> From: Chun Feng Wu <wucf@linux.ibm.com>
> 
> Hi,
> 
> I am thinking to leverage "throttle block filter" in QEMU to support more flexible I/O limits(e.g. tiered I/O groups), one sample provided by QEMU doc is:
> https://github.com/qemu/qemu/blob/master/docs/throttle.txt
> "For example, let's say that we have three different drives and we want to set I/O limits for
> each one of them and an additional set of limits for the combined I/O of all three drives."
> 
> The implementation idea is to 
> - Define throttle groups(limit) in domain
> - Define throttle filter to reference throttle group within disk
> - Within domain disk, throttle filters references multiple throttle groups to form filter chain to apply multiple limits in QEMU like above sample
> - Add new virsh cmds for throttle group management:
>     throttlegroupset               Add or update a throttling group.
>     throttlegroupdel               Delete a throttling group.
>     throttlegroupinfo              Get a throttling group.
>     throttlegrouplist              list all domain throttlegroups
> - Update "attach-disk" to add one more option "--throttle-groups" to apply throttle filters e.g. "virsh attach-disk $VM_ID ${DISK_PATH}/vm1_disk_2.qcow2 vdd --driver qemu --subdriver qcow2 --targetbus virtio --throttle-groups limit2,limit012"
> - I chose above semantics as I felt they're appropriate, if there are better ones please kindly suggest.
> 
> This patchset includes:
> - Throttle group and throttle filter definition in patch 1
> - New QMP processing to update and get throttle group in patch 2
> - New API definition and implementation in patch 3
> - Hotplug and qemuProcessLaunch flow implemenation in patch 4, 5
> - Domain XML schema and doc(formatdomain.rst) change in patch 6
> - Tests in patch 7, 8
> - Virsh cmd implementation in patch 9
> - Other enhencement/verification implementation in patch 10, 11, 12

[...]

> Any comments/suggestions will be appriciated!

Hi, I'll be doing a proper review later on, but a couple of observations
for now:

- The series fails 'check-remote_protocol' test after patch 3 but it's
  fixed later on. Note that per contribution guidelines the series must
  compile cleanly after every single patch.

- The coding style is inconsistent even inside one patch in terms of
  function declaration. Please use the:

returntype
functionname(type1 arg1,
             type2 arg2)

style and use two newlines between functions.

- patch 3 implements both the remote driver stuff and directly qemu
  impl, we try to keep those separated

- don't use line comments ( // comment )

- use g_autofree and g_autoptr instead of adding 'cleanup:' sections. We
  are trying to refactor the code to get away from the design pattern

- avoid empty 'cleanup:' labels, return value directly (e.g. patch 8)

- try to use virJSONValueAdd instead of
  virJSONValueLimitsAppendPositiveNumberLong, it can do the same without
  the extra helper by using the P modifier. Also make sure to check the
  return value of the JSON functions (patch 2).

- the virsh implementation adds  a lot of VSH_OT_ALIAS arguments with
  the spelling we tried to fix in the command you've copied it from. Do
  not add the old stuff again.

- preserve spacing between blocks of code which are not related (e.g.
  patch 11)

Rest of the review after I'll have a deeper look.
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

Re: [PATCH RFC 00/12] Support throttle block filters

Posted by Chun Feng Wu 2 weeks, 6 days ago

I addressed those comments in v2 
https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/5Z3J4SSEFWPZGN436HUGP2M6G4NPCWNW/

On 2024/4/10 16:08, Peter Krempa wrote:
> On Tue, Apr 09, 2024 at 20:13:08 -0700, wucf@linux.ibm.com wrote:
>> From: Chun Feng Wu <wucf@linux.ibm.com>
>>
>> Hi,
>>
>> I am thinking to leverage "throttle block filter" in QEMU to support more flexible I/O limits(e.g. tiered I/O groups), one sample provided by QEMU doc is:
>> https://github.com/qemu/qemu/blob/master/docs/throttle.txt
>> "For example, let's say that we have three different drives and we want to set I/O limits for
>> each one of them and an additional set of limits for the combined I/O of all three drives."
>>
>> The implementation idea is to
>> - Define throttle groups(limit) in domain
>> - Define throttle filter to reference throttle group within disk
>> - Within domain disk, throttle filters references multiple throttle groups to form filter chain to apply multiple limits in QEMU like above sample
>> - Add new virsh cmds for throttle group management:
>>      throttlegroupset               Add or update a throttling group.
>>      throttlegroupdel               Delete a throttling group.
>>      throttlegroupinfo              Get a throttling group.
>>      throttlegrouplist              list all domain throttlegroups
>> - Update "attach-disk" to add one more option "--throttle-groups" to apply throttle filters e.g. "virsh attach-disk $VM_ID ${DISK_PATH}/vm1_disk_2.qcow2 vdd --driver qemu --subdriver qcow2 --targetbus virtio --throttle-groups limit2,limit012"
>> - I chose above semantics as I felt they're appropriate, if there are better ones please kindly suggest.
>>
>> This patchset includes:
>> - Throttle group and throttle filter definition in patch 1
>> - New QMP processing to update and get throttle group in patch 2
>> - New API definition and implementation in patch 3
>> - Hotplug and qemuProcessLaunch flow implemenation in patch 4, 5
>> - Domain XML schema and doc(formatdomain.rst) change in patch 6
>> - Tests in patch 7, 8
>> - Virsh cmd implementation in patch 9
>> - Other enhencement/verification implementation in patch 10, 11, 12
> [...]
>
>> Any comments/suggestions will be appriciated!
> Hi, I'll be doing a proper review later on, but a couple of observations
> for now:
>
> - The series fails 'check-remote_protocol' test after patch 3 but it's
>    fixed later on. Note that per contribution guidelines the series must
>    compile cleanly after every single patch.
>
> - The coding style is inconsistent even inside one patch in terms of
>    function declaration. Please use the:
>
> returntype
> functionname(type1 arg1,
>               type2 arg2)
>
> style and use two newlines between functions.
>
> - patch 3 implements both the remote driver stuff and directly qemu
>    impl, we try to keep those separated
>
> - don't use line comments ( // comment )
>
> - use g_autofree and g_autoptr instead of adding 'cleanup:' sections. We
>    are trying to refactor the code to get away from the design pattern
>
> - avoid empty 'cleanup:' labels, return value directly (e.g. patch 8)
>
> - try to use virJSONValueAdd instead of
>    virJSONValueLimitsAppendPositiveNumberLong, it can do the same without
>    the extra helper by using the P modifier. Also make sure to check the
>    return value of the JSON functions (patch 2).
>
> - the virsh implementation adds  a lot of VSH_OT_ALIAS arguments with
>    the spelling we tried to fix in the command you've copied it from. Do
>    not add the old stuff again.
>
> - preserve spacing between blocks of code which are not related (e.g.
>    patch 11)
>
> Rest of the review after I'll have a deeper look.
>
-- 
Thanks and Regards,

Wu
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

Re: [PATCH RFC 00/12] Support throttle block filters

Posted by Chun Feng Wu 3 weeks, 1 day ago

Thanks Peter for your quick response! I will fix the issues you listed

On 2024/4/10 16:08, Peter Krempa wrote:
> On Tue, Apr 09, 2024 at 20:13:08 -0700, wucf@linux.ibm.com wrote:
>> From: Chun Feng Wu <wucf@linux.ibm.com>
>>
>> Hi,
>>
>> I am thinking to leverage "throttle block filter" in QEMU to support more flexible I/O limits(e.g. tiered I/O groups), one sample provided by QEMU doc is:
>> https://github.com/qemu/qemu/blob/master/docs/throttle.txt
>> "For example, let's say that we have three different drives and we want to set I/O limits for
>> each one of them and an additional set of limits for the combined I/O of all three drives."
>>
>> The implementation idea is to
>> - Define throttle groups(limit) in domain
>> - Define throttle filter to reference throttle group within disk
>> - Within domain disk, throttle filters references multiple throttle groups to form filter chain to apply multiple limits in QEMU like above sample
>> - Add new virsh cmds for throttle group management:
>>      throttlegroupset               Add or update a throttling group.
>>      throttlegroupdel               Delete a throttling group.
>>      throttlegroupinfo              Get a throttling group.
>>      throttlegrouplist              list all domain throttlegroups
>> - Update "attach-disk" to add one more option "--throttle-groups" to apply throttle filters e.g. "virsh attach-disk $VM_ID ${DISK_PATH}/vm1_disk_2.qcow2 vdd --driver qemu --subdriver qcow2 --targetbus virtio --throttle-groups limit2,limit012"
>> - I chose above semantics as I felt they're appropriate, if there are better ones please kindly suggest.
>>
>> This patchset includes:
>> - Throttle group and throttle filter definition in patch 1
>> - New QMP processing to update and get throttle group in patch 2
>> - New API definition and implementation in patch 3
>> - Hotplug and qemuProcessLaunch flow implemenation in patch 4, 5
>> - Domain XML schema and doc(formatdomain.rst) change in patch 6
>> - Tests in patch 7, 8
>> - Virsh cmd implementation in patch 9
>> - Other enhencement/verification implementation in patch 10, 11, 12
> [...]
>
>> Any comments/suggestions will be appriciated!
> Hi, I'll be doing a proper review later on, but a couple of observations
> for now:
>
> - The series fails 'check-remote_protocol' test after patch 3 but it's
>    fixed later on. Note that per contribution guidelines the series must
>    compile cleanly after every single patch.
>
> - The coding style is inconsistent even inside one patch in terms of
>    function declaration. Please use the:
>
> returntype
> functionname(type1 arg1,
>               type2 arg2)
>
> style and use two newlines between functions.
>
> - patch 3 implements both the remote driver stuff and directly qemu
>    impl, we try to keep those separated
>
> - don't use line comments ( // comment )
>
> - use g_autofree and g_autoptr instead of adding 'cleanup:' sections. We
>    are trying to refactor the code to get away from the design pattern
>
> - avoid empty 'cleanup:' labels, return value directly (e.g. patch 8)
>
> - try to use virJSONValueAdd instead of
>    virJSONValueLimitsAppendPositiveNumberLong, it can do the same without
>    the extra helper by using the P modifier. Also make sure to check the
>    return value of the JSON functions (patch 2).
>
> - the virsh implementation adds  a lot of VSH_OT_ALIAS arguments with
>    the spelling we tried to fix in the command you've copied it from. Do
>    not add the old stuff again.
>
> - preserve spacing between blocks of code which are not related (e.g.
>    patch 11)
>
> Rest of the review after I'll have a deeper look.
>
-- 
Thanks and Regards,

Wu
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org