Re: [Qemu-devel] [PATCH v3 2/4] fsdev: QMP interface for throttling

[Eric Blake]

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 <address@hidden>
>>> ---
>>

>>> +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