On 05/03/2017 10:40 AM, Pradeep Jagadeesh wrote: > On 5/3/2017 12:13 AM, Eric Blake wrote: >> On 05/02/2017 09:29 AM, Pradeep Jagadeesh wrote: >>> This patchset enables qmp interfaces for the fsdev >>> devices. This provides two interfaces one >>> for querying info of all the fsdev devices. The second one >>> to set the IO limits for the required fsdev device. >>> >>> Signed-off-by: Pradeep Jagadeesh <pradeep.jagade...@huawei.com> >>> --- >>
>>> +void fsdev_set_io_throttle(IOThrottle *arg, FsThrottle *fst, Error
>>> **errp)
>>> +{
>>> + ThrottleConfig cfg;
>>> +
>>> + throttle_config_init(&cfg);
>>> + cfg.buckets[THROTTLE_BPS_TOTAL].avg = arg->bps;
>>> + cfg.buckets[THROTTLE_BPS_READ].avg = arg->bps_rd;
>>> + cfg.buckets[THROTTLE_BPS_WRITE].avg = arg->bps_wr;
>>> +
>>> + cfg.buckets[THROTTLE_OPS_TOTAL].avg = arg->iops;
>>> + cfg.buckets[THROTTLE_OPS_READ].avg = arg->iops_rd;
>>> + cfg.buckets[THROTTLE_OPS_WRITE].avg = arg->iops_wr;
>>> +
>>> + if (arg->has_bps_max) {
>>> + cfg.buckets[THROTTLE_BPS_TOTAL].max = arg->bps_max;
>>> + }
>>
>> Should the bulk of this be replaced by a call to a common IOThrottle
>> helper function, rather than open-coded duplication?
> If we can reduce the duplicate code with a helper function its a good
> idea. But I can not think of any ways of doing it. Any suggestions?
In fact, you did exactly that in patch 3/4, which I argue should be
rebased to occur in front of this patch to minimize the churn (in other
words, when you add fsdev_set_io_throttle(), it should directly call the
qmp_set_io_throttle() helper).
>>> +void hmp_fsdev_set_io_throttle(Monitor *mon, const QDict *qdict)
>>> +{
>>> + Error *err = NULL;
>>> + IOThrottle throttle = {
>>> + .has_id = true,
>>> + .id = (char *) qdict_get_str(qdict, "id"),
>>> + .bps = qdict_get_int(qdict, "bps"),
>>> + .bps_rd = qdict_get_int(qdict, "bps_rd"),
>>> + .bps_wr = qdict_get_int(qdict, "bps_wr"),
>>> + .iops = qdict_get_int(qdict, "iops"),
>>> + .iops_rd = qdict_get_int(qdict, "iops_rd"),
>>> + .iops_wr = qdict_get_int(qdict, "iops_wr"),
>>
>> Again, don't you need to be setting .has_bps=true and so on?
> Same as above. I have not set any max burst values here, because I
> wanted to keep it in line with the block device.
> May be there is a room to enable these max values in both in future.
I don't think you were getting the point of my question. Since 'bps' is
an optional member of struct IOThrottle, you have to set 'has_bps' at
the same time to make it obvious that 'bps' should affect things.
I understand that you aren't setting 'bps_max', nor it's counterpart
'has_bps_max', and that's not what I was asking about.
>>> +++ b/qapi/iothrottle.json
>>> @@ -3,6 +3,7 @@
>>> ##
>>> # == QAPI IOThrottle definitions
>>> ##
>>> +##
>>> # @IOThrottle:
>>
>> This looks like a spurious change
> You mean the below sentence is not required?
No, the above addition of a second ## is not required.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
