I addressed those comments in v2
https://lists.libvirt.org/archives/list/devel@xxxxxxxxxxxxxxxxx/thread/5Z3J4SSEFWPZGN436HUGP2M6G4NPCWNW/
On 2024/4/10 16:08, Peter Krempa wrote:
On Tue, Apr 09, 2024 at 20:13:08 -0700, wucf@xxxxxxxxxxxxx wrote:
From: Chun Feng Wu <wucf@xxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
