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

[Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com>]

On 5/3/2017 6:32 PM, Eric Blake wrote:
> 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.jagadeesh@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).
>
OK, done.
>
>>>> +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 need some more clarifications here.
I did not understand what do you mean by an optional parameter?
You mean I need to set "has_*" for all the parameters?
> 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.
If I remove that I will get the compilation error.
I think the parser needs that.

-Pradeep
>