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

On Tue, Apr 09, 2024 at 20:13:08 -0700, wucf(a)linux.ibm.com wrote: > From: Chun Feng Wu <wucf(a)linux.ibm.com&gt; > > 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.