I addressed those comments in v2 https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/5Z3J4... 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