| Message ID | 1493735386-39622-3-git-send-email-pradeep.jagadeesh@huawei.com |
|---|---|
| State | New |
| Headers | show |
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> > --- > +++ b/fsdev/qemu-fsdev-throttle.c > @@ -29,6 +29,102 @@ static void fsdev_throttle_write_timer_cb(void *opaque) > qemu_co_enter_next(&fst->throttled_reqs[true]); > } > > +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? > + > +void fsdev_get_io_throttle(FsThrottle *fst, IOThrottle **fs9pcfg, > + char *fsdevice, Error **errp) > +{ > + > + ThrottleConfig cfg = fst->cfg; > + IOThrottle *fscfg = g_malloc0(sizeof(*fscfg)); > + > + fscfg->has_id = true; > + fscfg->id = g_strdup(fsdevice); > + fscfg->bps = cfg.buckets[THROTTLE_BPS_TOTAL].avg; > + fscfg->bps_rd = cfg.buckets[THROTTLE_BPS_READ].avg; > + fscfg->bps_wr = cfg.buckets[THROTTLE_BPS_WRITE].avg; Shouldn't you be setting has_bps, has_bps_rd, has_bps_wr, and so on, to true? > +++ b/fsdev/qemu-fsdev-throttle.h > @@ -19,7 +19,14 @@ > #include "qemu/main-loop.h" > #include "qemu/coroutine.h" > #include "qapi/error.h" > +#include "qapi/qmp/qerror.h" > #include "qemu/throttle.h" > +#include "qapi/qmp/types.h" > +#include "qapi-visit.h" > +#include "qapi/qobject-output-visitor.h" > +#include "qapi/util.h" > +#include "qmp-commands.h" > +#include "qemu/throttle-options.h" > > typedef struct FsThrottle { > ThrottleState ts; > @@ -36,4 +43,10 @@ void coroutine_fn fsdev_co_throttle_request(FsThrottle *, bool , > struct iovec *, int); > > void fsdev_throttle_cleanup(FsThrottle *); > + > +void fsdev_set_io_throttle(IOThrottle *, FsThrottle *, Error **); Even though it's not necessary per C, we tend to spell parameter names in function declarations as it aids legibility. (Seeing 'Error **' is weird compared to the usual 'Error **errp') > @@ -1570,6 +1571,80 @@ void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict) > hmp_handle_error(mon, &err); > } > > +#ifdef CONFIG_VIRTFS > + > +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? > +++ b/qapi/fsdev.json > @@ -0,0 +1,84 @@ > +# -*- Mode: Python -*- > + > +## > +# == QAPI fsdev definitions > +## > + > +# QAPI common definitions > +{ 'include': 'iothrottle.json' } > + > +## > +# @fsdev-set-io-throttle: > +# > +# Change I/O limits for a 9p/fsdev device. > +# > +# I/O limits can be enabled by setting throttle value to non-zero number. > +# > +# I/O limits can be disabled by setting all throttle values to 0. > +# > +# Returns: Nothing on success > +# If @device is not a valid fsdev device, DeviceNotFound > +# > +# Since: 2.10 > +# > +# Example: > +# > +# -> { "execute": "fsdev-set-io-throttle", > +# "arguments": { "id": "id0-1-0", > +# "bps": 1000000, > +# "bps_rd": 0, > +# "bps_wr": 0, > +# "iops": 0, > +# "iops_rd": 0, > +# "iops_wr": 0, > +# "bps_max": 8000000, > +# "bps_rd_max": 0, > +# "bps_wr_max": 0, > +# "iops_max": 0, > +# "iops_rd_max": 0, > +# "iops_wr_max": 0, > +# "bps_max_length": 60, > +# "iops_size": 0 } } > +# <- { "returns": {} } > +## > +{ 'command': 'fsdev-set-io-throttle', 'boxed': true, > + 'data': 'IOThrottle' } This part looks okay. > +## > +# @query-fsdev-io-throttle: > +# > +# Returns: a list of @IOThrottle describing io throttle values of each fsdev device > +# > +# Since: 2.10 > +# > +# Example: > +# > +# -> { "Execute": "query-fsdev-io-throttle" } > +# <- { "returns" : [ > +# { > +# "id": "id0-hd0", > +# "bps":1000000, > +# "bps_rd":0, > +# "bps_wr":0, > +# "iops":1000000, > +# "iops_rd":0, > +# "iops_wr":0, > +# "bps_max": 8000000, > +# "bps_rd_max": 0, > +# "bps_wr_max": 0, > +# "iops_max": 0, > +# "iops_rd_max": 0, > +# "iops_wr_max": 0, > +# "bps_max_length": 0, > +# "bps_rd_max_length": 0, > +# "bps_wr_max_length": 0, > +# "iops_max_length": 0, > +# "iops_rd_max_length": 0, > +# "iops_wr_max_length": 0, > +# "iops_size": 0 > +# } > +# ] > +# } > +# > +## > +{ 'command': 'query-fsdev-io-throttle', 'returns': [ 'IOThrottle' ] } > + > diff --git a/qapi/iothrottle.json b/qapi/iothrottle.json > index 124ab40..698d4bc 100644 > --- a/qapi/iothrottle.json > +++ b/qapi/iothrottle.json > @@ -3,6 +3,7 @@ > ## > # == QAPI IOThrottle definitions > ## > +## > # @IOThrottle: This looks like a spurious change > # > # A set of parameters describing iothrottle >
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> >> --- > >> +++ b/fsdev/qemu-fsdev-throttle.c >> @@ -29,6 +29,102 @@ static void fsdev_throttle_write_timer_cb(void *opaque) >> qemu_co_enter_next(&fst->throttled_reqs[true]); >> } >> >> +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? > > >> + >> +void fsdev_get_io_throttle(FsThrottle *fst, IOThrottle **fs9pcfg, >> + char *fsdevice, Error **errp) >> +{ >> + >> + ThrottleConfig cfg = fst->cfg; >> + IOThrottle *fscfg = g_malloc0(sizeof(*fscfg)); >> + >> + fscfg->has_id = true; >> + fscfg->id = g_strdup(fsdevice); >> + fscfg->bps = cfg.buckets[THROTTLE_BPS_TOTAL].avg; >> + fscfg->bps_rd = cfg.buckets[THROTTLE_BPS_READ].avg; >> + fscfg->bps_wr = cfg.buckets[THROTTLE_BPS_WRITE].avg; > > Shouldn't you be setting has_bps, has_bps_rd, has_bps_wr, and so on, to > true? Yes, I need to set, but its only for max values, I mean has_bps_max..etc > > >> +++ b/fsdev/qemu-fsdev-throttle.h >> @@ -19,7 +19,14 @@ >> #include "qemu/main-loop.h" >> #include "qemu/coroutine.h" >> #include "qapi/error.h" >> +#include "qapi/qmp/qerror.h" >> #include "qemu/throttle.h" >> +#include "qapi/qmp/types.h" >> +#include "qapi-visit.h" >> +#include "qapi/qobject-output-visitor.h" >> +#include "qapi/util.h" >> +#include "qmp-commands.h" >> +#include "qemu/throttle-options.h" >> >> typedef struct FsThrottle { >> ThrottleState ts; >> @@ -36,4 +43,10 @@ void coroutine_fn fsdev_co_throttle_request(FsThrottle *, bool , >> struct iovec *, int); >> >> void fsdev_throttle_cleanup(FsThrottle *); >> + >> +void fsdev_set_io_throttle(IOThrottle *, FsThrottle *, Error **); > > Even though it's not necessary per C, we tend to spell parameter names > in function declarations as it aids legibility. (Seeing 'Error **' is > weird compared to the usual 'Error **errp') OK, I will fix it. > > >> @@ -1570,6 +1571,80 @@ void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict) >> hmp_handle_error(mon, &err); >> } >> >> +#ifdef CONFIG_VIRTFS >> + >> +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. > > >> +++ b/qapi/fsdev.json >> @@ -0,0 +1,84 @@ >> +# -*- Mode: Python -*- >> + >> +## >> +# == QAPI fsdev definitions >> +## >> + >> +# QAPI common definitions >> +{ 'include': 'iothrottle.json' } >> + >> +## >> +# @fsdev-set-io-throttle: >> +# >> +# Change I/O limits for a 9p/fsdev device. >> +# >> +# I/O limits can be enabled by setting throttle value to non-zero number. >> +# >> +# I/O limits can be disabled by setting all throttle values to 0. >> +# >> +# Returns: Nothing on success >> +# If @device is not a valid fsdev device, DeviceNotFound >> +# >> +# Since: 2.10 >> +# >> +# Example: >> +# >> +# -> { "execute": "fsdev-set-io-throttle", >> +# "arguments": { "id": "id0-1-0", >> +# "bps": 1000000, >> +# "bps_rd": 0, >> +# "bps_wr": 0, >> +# "iops": 0, >> +# "iops_rd": 0, >> +# "iops_wr": 0, >> +# "bps_max": 8000000, >> +# "bps_rd_max": 0, >> +# "bps_wr_max": 0, >> +# "iops_max": 0, >> +# "iops_rd_max": 0, >> +# "iops_wr_max": 0, >> +# "bps_max_length": 60, >> +# "iops_size": 0 } } >> +# <- { "returns": {} } >> +## >> +{ 'command': 'fsdev-set-io-throttle', 'boxed': true, >> + 'data': 'IOThrottle' } > > This part looks okay. > >> +## >> +# @query-fsdev-io-throttle: >> +# >> +# Returns: a list of @IOThrottle describing io throttle values of each fsdev device >> +# >> +# Since: 2.10 >> +# >> +# Example: >> +# >> +# -> { "Execute": "query-fsdev-io-throttle" } >> +# <- { "returns" : [ >> +# { >> +# "id": "id0-hd0", >> +# "bps":1000000, >> +# "bps_rd":0, >> +# "bps_wr":0, >> +# "iops":1000000, >> +# "iops_rd":0, >> +# "iops_wr":0, >> +# "bps_max": 8000000, >> +# "bps_rd_max": 0, >> +# "bps_wr_max": 0, >> +# "iops_max": 0, >> +# "iops_rd_max": 0, >> +# "iops_wr_max": 0, >> +# "bps_max_length": 0, >> +# "bps_rd_max_length": 0, >> +# "bps_wr_max_length": 0, >> +# "iops_max_length": 0, >> +# "iops_rd_max_length": 0, >> +# "iops_wr_max_length": 0, >> +# "iops_size": 0 >> +# } >> +# ] >> +# } >> +# >> +## >> +{ 'command': 'query-fsdev-io-throttle', 'returns': [ 'IOThrottle' ] } >> + >> diff --git a/qapi/iothrottle.json b/qapi/iothrottle.json >> index 124ab40..698d4bc 100644 >> --- a/qapi/iothrottle.json >> +++ 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? I will remove it. Its not really required. > >> # >> # A set of parameters describing iothrottle >> >
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). >>> +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.
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 >
On 05/04/2017 10:12 AM, Pradeep Jagadeesh wrote: >>>>> + 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? Yes, any optional parameter (one named '*foo' in the .json file) has a counterpart has_foo boolean, which should be set to true when the parameter is in use (and thus making it obvious when it is set to false that it is not in use). >> 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. If the parser chokes without it, then that implies patch 1 is broken, and the fix should be squashed there for full bisectability. Either way, it looks like a spurious change when it is occurring in patch 3.
On 5/4/2017 5:19 PM, Eric Blake wrote: > On 05/04/2017 10:12 AM, Pradeep Jagadeesh wrote: > >>>>>> + 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? > > Yes, any optional parameter (one named '*foo' in the .json file) has a > counterpart has_foo boolean, which should be set to true when the > parameter is in use (and thus making it obvious when it is set to false > that it is not in use). OK, but here the id is only with * (Optional), others are not. Still need them to set? > >>> 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. > > If the parser chokes without it, then that implies patch 1 is broken, > and the fix should be squashed there for full bisectability. Either > way, it looks like a spurious change when it is occurring in patch 3. > Yes, I reordered the patches and fixed this issue. -Pradeep
diff --git a/Makefile b/Makefile index 31d41a7..7b928d9 100644 --- a/Makefile +++ b/Makefile @@ -414,6 +414,9 @@ qapi-modules = $(SRC_PATH)/qapi-schema.json $(SRC_PATH)/qapi/common.json \ $(SRC_PATH)/qapi/event.json $(SRC_PATH)/qapi/introspect.json \ $(SRC_PATH)/qapi/crypto.json $(SRC_PATH)/qapi/rocker.json \ $(SRC_PATH)/qapi/trace.json +ifdef CONFIG_VIRTFS +qapi-modules += $(SRC_PATH)/qapi/fsdev.json +endif qapi-types.c qapi-types.h :\ $(qapi-modules) $(SRC_PATH)/scripts/qapi-types.py $(qapi-py) diff --git a/Makefile.objs b/Makefile.objs index 6167e7b..3d01f54 100644 --- a/Makefile.objs +++ b/Makefile.objs @@ -79,9 +79,9 @@ common-obj-$(CONFIG_FDT) += device_tree.o ###################################################################### # qapi -common-obj-y += qmp-marshal.o +common-obj-y += qmp-marshal.o qmp.o common-obj-y += qmp-introspect.o -common-obj-y += qmp.o hmp.o +common-obj-y += hmp.o endif ####################################################################### diff --git a/fsdev/qemu-fsdev-dummy.c b/fsdev/qemu-fsdev-dummy.c index 6dc0fbc..0e23636 100644 --- a/fsdev/qemu-fsdev-dummy.c +++ b/fsdev/qemu-fsdev-dummy.c @@ -14,8 +14,19 @@ #include "qemu-fsdev.h" #include "qemu/config-file.h" #include "qemu/module.h" +#include "qmp-commands.h" int qemu_fsdev_add(QemuOpts *opts) { return 0; } + +void qmp_fsdev_set_io_throttle(IOThrottle *arg, Error **errp) +{ + return; +} + +IOThrottleList *qmp_query_fsdev_io_throttle(Error **errp) +{ + abort(); +} diff --git a/fsdev/qemu-fsdev-throttle.c b/fsdev/qemu-fsdev-throttle.c index 7ae4e86..43dbcd8 100644 --- a/fsdev/qemu-fsdev-throttle.c +++ b/fsdev/qemu-fsdev-throttle.c @@ -29,6 +29,102 @@ static void fsdev_throttle_write_timer_cb(void *opaque) qemu_co_enter_next(&fst->throttled_reqs[true]); } +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; + } + if (arg->has_bps_rd_max) { + cfg.buckets[THROTTLE_BPS_READ].max = arg->bps_rd_max; + } + if (arg->has_bps_wr_max) { + cfg.buckets[THROTTLE_BPS_WRITE].max = arg->bps_wr_max; + } + if (arg->has_iops_max) { + cfg.buckets[THROTTLE_OPS_TOTAL].max = arg->iops_max; + } + if (arg->has_iops_rd_max) { + cfg.buckets[THROTTLE_OPS_READ].max = arg->iops_rd_max; + } + if (arg->has_iops_wr_max) { + cfg.buckets[THROTTLE_OPS_WRITE].max = arg->iops_wr_max; + } + + if (arg->has_bps_max_length) { + cfg.buckets[THROTTLE_BPS_TOTAL].burst_length = arg->bps_max_length; + } + if (arg->has_bps_rd_max_length) { + cfg.buckets[THROTTLE_BPS_READ].burst_length = arg->bps_rd_max_length; + } + if (arg->has_bps_wr_max_length) { + cfg.buckets[THROTTLE_BPS_WRITE].burst_length = arg->bps_wr_max_length; + } + if (arg->has_iops_max_length) { + cfg.buckets[THROTTLE_OPS_TOTAL].burst_length = arg->iops_max_length; + } + if (arg->has_iops_rd_max_length) { + cfg.buckets[THROTTLE_OPS_READ].burst_length = arg->iops_rd_max_length; + } + if (arg->has_iops_wr_max_length) { + cfg.buckets[THROTTLE_OPS_WRITE].burst_length = arg->iops_wr_max_length; + } + + if (arg->has_iops_size) { + cfg.op_size = arg->iops_size; + } + + if (throttle_is_valid(&cfg, errp)) { + fst->cfg = cfg; + fsdev_throttle_init(fst); + } +} + +void fsdev_get_io_throttle(FsThrottle *fst, IOThrottle **fs9pcfg, + char *fsdevice, Error **errp) +{ + + ThrottleConfig cfg = fst->cfg; + IOThrottle *fscfg = g_malloc0(sizeof(*fscfg)); + + fscfg->has_id = true; + fscfg->id = g_strdup(fsdevice); + fscfg->bps = cfg.buckets[THROTTLE_BPS_TOTAL].avg; + fscfg->bps_rd = cfg.buckets[THROTTLE_BPS_READ].avg; + fscfg->bps_wr = cfg.buckets[THROTTLE_BPS_WRITE].avg; + + fscfg->iops = cfg.buckets[THROTTLE_OPS_TOTAL].avg; + fscfg->iops_rd = cfg.buckets[THROTTLE_OPS_READ].avg; + fscfg->iops_wr = cfg.buckets[THROTTLE_OPS_WRITE].avg; + + fscfg->bps_max = cfg.buckets[THROTTLE_BPS_TOTAL].max; + fscfg->bps_rd_max = cfg.buckets[THROTTLE_BPS_READ].max; + fscfg->bps_wr_max = cfg.buckets[THROTTLE_BPS_WRITE].max; + fscfg->bps_max = cfg.buckets[THROTTLE_OPS_TOTAL].max; + fscfg->iops_rd_max = cfg.buckets[THROTTLE_OPS_READ].max; + fscfg->iops_wr_max = cfg.buckets[THROTTLE_OPS_WRITE].max; + + fscfg->bps_max_length = cfg.buckets[THROTTLE_BPS_TOTAL].burst_length; + fscfg->bps_rd_max_length = cfg.buckets[THROTTLE_BPS_READ].burst_length; + fscfg->bps_wr_max_length = cfg.buckets[THROTTLE_BPS_WRITE].burst_length; + fscfg->iops_max_length = cfg.buckets[THROTTLE_OPS_TOTAL].burst_length; + fscfg->iops_rd_max_length = cfg.buckets[THROTTLE_OPS_READ].burst_length; + fscfg->iops_wr_max_length = cfg.buckets[THROTTLE_OPS_WRITE].burst_length; + fscfg->iops_size = cfg.op_size; + + *fs9pcfg = fscfg; +} + void fsdev_throttle_parse_opts(QemuOpts *opts, FsThrottle *fst, Error **errp) { throttle_config_init(&fst->cfg); diff --git a/fsdev/qemu-fsdev-throttle.h b/fsdev/qemu-fsdev-throttle.h index e418643..0abba43 100644 --- a/fsdev/qemu-fsdev-throttle.h +++ b/fsdev/qemu-fsdev-throttle.h @@ -19,7 +19,14 @@ #include "qemu/main-loop.h" #include "qemu/coroutine.h" #include "qapi/error.h" +#include "qapi/qmp/qerror.h" #include "qemu/throttle.h" +#include "qapi/qmp/types.h" +#include "qapi-visit.h" +#include "qapi/qobject-output-visitor.h" +#include "qapi/util.h" +#include "qmp-commands.h" +#include "qemu/throttle-options.h" typedef struct FsThrottle { ThrottleState ts; @@ -36,4 +43,10 @@ void coroutine_fn fsdev_co_throttle_request(FsThrottle *, bool , struct iovec *, int); void fsdev_throttle_cleanup(FsThrottle *); + +void fsdev_set_io_throttle(IOThrottle *, FsThrottle *, Error **); + +void fsdev_get_io_throttle(FsThrottle *, IOThrottle **, char *, Error **); + + #endif /* _FSDEV_THROTTLE_H */ diff --git a/fsdev/qemu-fsdev.c b/fsdev/qemu-fsdev.c index 266e442..a99e299 100644 --- a/fsdev/qemu-fsdev.c +++ b/fsdev/qemu-fsdev.c @@ -16,6 +16,7 @@ #include "qemu-common.h" #include "qemu/config-file.h" #include "qemu/error-report.h" +#include "qmp-commands.h" static QTAILQ_HEAD(FsDriverEntry_head, FsDriverListEntry) fsdriver_entries = QTAILQ_HEAD_INITIALIZER(fsdriver_entries); @@ -98,3 +99,39 @@ FsDriverEntry *get_fsdev_fsentry(char *id) } return NULL; } + +void qmp_fsdev_set_io_throttle(IOThrottle *arg, Error **errp) +{ + + FsDriverEntry *fse; + + fse = get_fsdev_fsentry(arg->has_id ? arg->id : NULL); + if (!fse) { + return; + } + + fsdev_set_io_throttle(arg, &fse->fst, errp); +} + +IOThrottleList *qmp_query_fsdev_io_throttle(Error **errp) +{ + IOThrottleList *head = NULL, **p_next = &head; + struct FsDriverListEntry *fsle; + Error *local_err = NULL; + + QTAILQ_FOREACH(fsle, &fsdriver_entries, next) { + IOThrottleList *fscfg = g_malloc0(sizeof(*fscfg)); + fsdev_get_io_throttle(&fsle->fse.fst, &fscfg->value, + fsle->fse.fsdev_id, &local_err); + if (local_err) { + error_propagate(errp, local_err); + g_free(fscfg); + qapi_free_IOThrottleList(head); + return NULL; + } + + *p_next = fscfg; + p_next = &fscfg->next; + } + return head; +} diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx index a53f105..8c1c33e 100644 --- a/hmp-commands-info.hx +++ b/hmp-commands-info.hx @@ -84,6 +84,25 @@ STEXI Show block device statistics. ETEXI +#if defined(CONFIG_VIRTFS) + + { + .name = "query-fsdev-iothrottle", + .args_type = "", + .params = "", + .help = "show fsdev device throttle information", + .cmd = hmp_fsdev_get_io_throttle, + }, + +#endif + +STEXI +@item info fsdev throttle +@findex fsdevthrottleinfo +Show fsdev device throttleinfo. + +ETEXI + { .name = "block-jobs", .args_type = "", diff --git a/hmp-commands.hx b/hmp-commands.hx index 0aca984..7699475 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -1660,6 +1660,25 @@ STEXI Change I/O throttle limits for a block drive to @var{bps} @var{bps_rd} @var{bps_wr} @var{iops} @var{iops_rd} @var{iops_wr} ETEXI +#if defined(CONFIG_VIRTFS) + + { + .name = "fsdev-set-io-throttle", + .args_type = "device:B,bps:l,bps_rd:l,bps_wr:l,iops:l,iops_rd:l,iops_wr:l", + .params = "device bps bps_rd bps_wr iops iops_rd iops_wr", + .help = "change I/O throttle limits for a block drive", + .cmd = hmp_fsdev_set_io_throttle, + }, + +#endif + +STEXI +@item fsdev_set_io_throttle @var{device} @var{bps} @var{bps_rd} @var{bps_wr} @var{iops} @var{iops_rd} @var{iops_wr} +@findex fsdev_set_io_throttle +Change I/O throttle limits for a block drive to @var{bps} @var{bps_rd} @var{bps_wr} @var{iops} @var{iops_rd} @var{iops_wr} + +ETEXI + { .name = "set_password", .args_type = "protocol:s,password:s,connected:s?", diff --git a/hmp.c b/hmp.c index ab407d6..9c02e55 100644 --- a/hmp.c +++ b/hmp.c @@ -38,6 +38,7 @@ #include "qemu/cutils.h" #include "qemu/error-report.h" #include "hw/intc/intc.h" +#include "fsdev/qemu-fsdev-throttle.h" #ifdef CONFIG_SPICE #include <spice/enums.h> @@ -1570,6 +1571,80 @@ void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict) hmp_handle_error(mon, &err); } +#ifdef CONFIG_VIRTFS + +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"), + }; + + qmp_fsdev_set_io_throttle(&throttle, &err); + hmp_handle_error(mon, &err); +} + +static void print_fsdev_throttle_config(Monitor *mon, IOThrottle *fscfg, + Error *err) +{ + if (fscfg->bps || fscfg->bps_rd || fscfg->bps_wr || + fscfg->iops || fscfg->iops_rd || fscfg->iops_wr) + { + monitor_printf(mon, "%s", fscfg->id); + monitor_printf(mon, " I/O throttling:" + " bps=%" PRId64 + " bps_rd=%" PRId64 " bps_wr=%" PRId64 + " bps_max=%" PRId64 + " bps_rd_max=%" PRId64 + " bps_wr_max=%" PRId64 + " iops=%" PRId64 " iops_rd=%" PRId64 + " iops_wr=%" PRId64 + " iops_max=%" PRId64 + " iops_rd_max=%" PRId64 + " iops_wr_max=%" PRId64 + " iops_size=%" PRId64, + fscfg->bps, + fscfg->bps_rd, + fscfg->bps_wr, + fscfg->bps_max, + fscfg->bps_rd_max, + fscfg->bps_wr_max, + fscfg->iops, + fscfg->iops_rd, + fscfg->iops_wr, + fscfg->iops_max, + fscfg->iops_rd_max, + fscfg->iops_wr_max, + fscfg->iops_size); + } + hmp_handle_error(mon, &err); +} + +void hmp_fsdev_get_io_throttle(Monitor *mon, const QDict *qdict) +{ + Error *err = NULL; + IOThrottleList *fs9p_list, *info; + fs9p_list = qmp_query_fsdev_io_throttle(&err); + + for (info = fs9p_list; info; info = info->next) { + if (info != fs9p_list) { + monitor_printf(mon, "\n"); + } + print_fsdev_throttle_config(mon, info->value, err); + qapi_free_IOThrottle(info->value); + } + qapi_free_IOThrottleList(fs9p_list); +} + +#endif + void hmp_block_stream(Monitor *mon, const QDict *qdict) { Error *error = NULL; diff --git a/hmp.h b/hmp.h index 799fd37..acce4c1 100644 --- a/hmp.h +++ b/hmp.h @@ -78,6 +78,11 @@ void hmp_expire_password(Monitor *mon, const QDict *qdict); void hmp_eject(Monitor *mon, const QDict *qdict); void hmp_change(Monitor *mon, const QDict *qdict); void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict); +#ifdef CONFIG_VIRTFS +void hmp_fsdev_set_io_throttle(Monitor *mon, const QDict *qdict); +void hmp_fsdev_get_io_throttle(Monitor *mon, const QDict *qdict); +#endif + void hmp_block_stream(Monitor *mon, const QDict *qdict); void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict); void hmp_block_job_cancel(Monitor *mon, const QDict *qdict); diff --git a/qapi-schema.json b/qapi-schema.json index 01b087f..6aa1446 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -81,6 +81,9 @@ # QAPI block definitions { 'include': 'qapi/block.json' } +# QAPI fsdev definitions +{ 'include': 'qapi/fsdev.json' } + # QAPI event definitions { 'include': 'qapi/event.json' } diff --git a/qapi/fsdev.json b/qapi/fsdev.json new file mode 100644 index 0000000..eff1efe --- /dev/null +++ b/qapi/fsdev.json @@ -0,0 +1,84 @@ +# -*- Mode: Python -*- + +## +# == QAPI fsdev definitions +## + +# QAPI common definitions +{ 'include': 'iothrottle.json' } + +## +# @fsdev-set-io-throttle: +# +# Change I/O limits for a 9p/fsdev device. +# +# I/O limits can be enabled by setting throttle value to non-zero number. +# +# I/O limits can be disabled by setting all throttle values to 0. +# +# Returns: Nothing on success +# If @device is not a valid fsdev device, DeviceNotFound +# +# Since: 2.10 +# +# Example: +# +# -> { "execute": "fsdev-set-io-throttle", +# "arguments": { "id": "id0-1-0", +# "bps": 1000000, +# "bps_rd": 0, +# "bps_wr": 0, +# "iops": 0, +# "iops_rd": 0, +# "iops_wr": 0, +# "bps_max": 8000000, +# "bps_rd_max": 0, +# "bps_wr_max": 0, +# "iops_max": 0, +# "iops_rd_max": 0, +# "iops_wr_max": 0, +# "bps_max_length": 60, +# "iops_size": 0 } } +# <- { "returns": {} } +## +{ 'command': 'fsdev-set-io-throttle', 'boxed': true, + 'data': 'IOThrottle' } +## +# @query-fsdev-io-throttle: +# +# Returns: a list of @IOThrottle describing io throttle values of each fsdev device +# +# Since: 2.10 +# +# Example: +# +# -> { "Execute": "query-fsdev-io-throttle" } +# <- { "returns" : [ +# { +# "id": "id0-hd0", +# "bps":1000000, +# "bps_rd":0, +# "bps_wr":0, +# "iops":1000000, +# "iops_rd":0, +# "iops_wr":0, +# "bps_max": 8000000, +# "bps_rd_max": 0, +# "bps_wr_max": 0, +# "iops_max": 0, +# "iops_rd_max": 0, +# "iops_wr_max": 0, +# "bps_max_length": 0, +# "bps_rd_max_length": 0, +# "bps_wr_max_length": 0, +# "iops_max_length": 0, +# "iops_rd_max_length": 0, +# "iops_wr_max_length": 0, +# "iops_size": 0 +# } +# ] +# } +# +## +{ 'command': 'query-fsdev-io-throttle', 'returns': [ 'IOThrottle' ] } + diff --git a/qapi/iothrottle.json b/qapi/iothrottle.json index 124ab40..698d4bc 100644 --- a/qapi/iothrottle.json +++ b/qapi/iothrottle.json @@ -3,6 +3,7 @@ ## # == QAPI IOThrottle definitions ## +## # @IOThrottle: # # A set of parameters describing iothrottle
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> --- Makefile | 3 ++ Makefile.objs | 4 +- fsdev/qemu-fsdev-dummy.c | 11 ++++++ fsdev/qemu-fsdev-throttle.c | 96 +++++++++++++++++++++++++++++++++++++++++++++ fsdev/qemu-fsdev-throttle.h | 13 ++++++ fsdev/qemu-fsdev.c | 37 +++++++++++++++++ hmp-commands-info.hx | 19 +++++++++ hmp-commands.hx | 19 +++++++++ hmp.c | 75 +++++++++++++++++++++++++++++++++++ hmp.h | 5 +++ qapi-schema.json | 3 ++ qapi/fsdev.json | 84 +++++++++++++++++++++++++++++++++++++++ qapi/iothrottle.json | 1 + 13 files changed, 368 insertions(+), 2 deletions(-) create mode 100644 qapi/fsdev.json