| Message ID | 1504016587-39779-6-git-send-email-pradeep.jagadeesh@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | fsdev: qmp interface for io throttling | expand |
On Tue 29 Aug 2017 04:23:06 PM CEST, Pradeep Jagadeesh wrote: > +static void print_fsdev_throttle_config(Monitor *mon, IOThrottle *fscfg, > + Error *err) > +{ > + 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 > + "\n", > + 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_info_fsdev_iothrottle(Monitor *mon, const QDict *qdict) > +{ > + Error *err = NULL; > + IOThrottleList *fsdev_list, *info; > + fsdev_list = qmp_query_fsdev_io_throttle(&err); > + > + for (info = fsdev_list; info; info = info->next) { > + print_fsdev_throttle_config(mon, info->value, err); > + } > + qapi_free_IOThrottleList(fsdev_list); > +} I don't think what you're doing with the Error here is right: - You store the error returned by qmp_query_fsdev_io_throttle(). - You report the error for _every_ fsdev in the list. That doesn't make much sense. - Furthermore, if there's an error then fsdev_list should be empty, so you're not reporting anything (and you're leaking the error). - Even if the list was not empty, hmp_handle_error() frees the error after reporting it. Therefore the second item in the list would try to report an error that has already been freed. Berto
On 8/30/2017 11:38 AM, Alberto Garcia wrote: > On Tue 29 Aug 2017 04:23:06 PM CEST, Pradeep Jagadeesh wrote: > >> +static void print_fsdev_throttle_config(Monitor *mon, IOThrottle *fscfg, >> + Error *err) >> +{ >> + 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 >> + "\n", >> + 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_info_fsdev_iothrottle(Monitor *mon, const QDict *qdict) >> +{ >> + Error *err = NULL; >> + IOThrottleList *fsdev_list, *info; >> + fsdev_list = qmp_query_fsdev_io_throttle(&err); >> + >> + for (info = fsdev_list; info; info = info->next) { >> + print_fsdev_throttle_config(mon, info->value, err); >> + } >> + qapi_free_IOThrottleList(fsdev_list); >> +} > > I don't think what you're doing with the Error here is right: > > - You store the error returned by qmp_query_fsdev_io_throttle(). > - You report the error for _every_ fsdev in the list. That doesn't > make much sense. > - Furthermore, if there's an error then fsdev_list should be empty, > so you're not reporting anything (and you're leaking the error). > - Even if the list was not empty, hmp_handle_error() frees the error > after reporting it. Therefore the second item in the list would > try to report an error that has already been freed. You mean something like below? fsdev_list = qmp_query_fsdev_io_throttle(&err); if (err) { error_report_err(err); return; } Regards, Pradeep > > Berto >
On Mon 04 Sep 2017 02:22:40 PM CEST, Pradeep Jagadeesh wrote: >>> +void hmp_info_fsdev_iothrottle(Monitor *mon, const QDict *qdict) >>> +{ >>> + Error *err = NULL; >>> + IOThrottleList *fsdev_list, *info; >>> + fsdev_list = qmp_query_fsdev_io_throttle(&err); >>> + >>> + for (info = fsdev_list; info; info = info->next) { >>> + print_fsdev_throttle_config(mon, info->value, err); >>> + } >>> + qapi_free_IOThrottleList(fsdev_list); >>> +} >> >> I don't think what you're doing with the Error here is right: >> >> - You store the error returned by qmp_query_fsdev_io_throttle(). >> - You report the error for _every_ fsdev in the list. That doesn't >> make much sense. >> - Furthermore, if there's an error then fsdev_list should be empty, >> so you're not reporting anything (and you're leaking the error). >> - Even if the list was not empty, hmp_handle_error() frees the error >> after reporting it. Therefore the second item in the list would >> try to report an error that has already been freed. > You mean something like below? > > fsdev_list = qmp_query_fsdev_io_throttle(&err); > if (err) { > error_report_err(err); > return; > } More or less, but there's hmp_handle_error() already, so you should use it: void hmp_info_fsdev_iothrottle(Monitor *mon, const QDict *qdict) { Error *err = NULL; IOThrottleList *fsdev_list, *info; fsdev_list = qmp_query_fsdev_io_throttle(&err); for (info = fsdev_list; info; info = info->next) { print_fsdev_throttle_config(mon, info->value); } qapi_free_IOThrottleList(fsdev_list); hmp_handle_error(mon, &err); } Anyway I just checked that fsdev_get_io_throttle() can never return an Error, so why don't you remove the Error parameter from there altogether? You still need it in qmp_query_fsdev_io_throttle() because that's part of the API, and hmp_info_fsdev_iothrottle() should have the code to handle it like the one I just pasted, but fsdev_get_io_throttle() does not need it, or does it? Berto
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx index d9df238..07ed338 100644 --- a/hmp-commands-info.hx +++ b/hmp-commands-info.hx @@ -84,6 +84,24 @@ STEXI Show block device statistics. ETEXI +#if defined(CONFIG_VIRTFS) + + { + .name = "fsdev_iothrottle", + .args_type = "", + .params = "", + .help = "show fsdev iothrottle information", + .cmd = hmp_info_fsdev_iothrottle, + }, + +#endif + +STEXI +@item info fsdev_iothrottle +@findex fsdev_iothrottle +Show fsdev device throttle info. +ETEXI + { .name = "block-jobs", .args_type = "", diff --git a/hmp-commands.hx b/hmp-commands.hx index 1941e19..aef9f79 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -1680,6 +1680,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 fs devices", + .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 fs devices 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 2dbfb80..712c6a3 100644 --- a/hmp.c +++ b/hmp.c @@ -1783,6 +1783,68 @@ 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, "device"), + }; + + hmp_initialize_io_throttle(&throttle, qdict); + qmp_fsdev_set_io_throttle(&throttle, &err); + hmp_handle_error(mon, &err); +} + +static void print_fsdev_throttle_config(Monitor *mon, IOThrottle *fscfg, + Error *err) +{ + 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 + "\n", + 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_info_fsdev_iothrottle(Monitor *mon, const QDict *qdict) +{ + Error *err = NULL; + IOThrottleList *fsdev_list, *info; + fsdev_list = qmp_query_fsdev_io_throttle(&err); + + for (info = fsdev_list; info; info = info->next) { + print_fsdev_throttle_config(mon, info->value, err); + } + qapi_free_IOThrottleList(fsdev_list); +} + +#endif + void hmp_block_stream(Monitor *mon, const QDict *qdict) { Error *error = NULL; diff --git a/hmp.h b/hmp.h index 1ff4552..d700d7d 100644 --- a/hmp.h +++ b/hmp.h @@ -81,6 +81,10 @@ void hmp_set_password(Monitor *mon, const QDict *qdict); 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); +#ifdef CONFIG_VIRTFS +void hmp_fsdev_set_io_throttle(Monitor *mon, const QDict *qdict); +void hmp_info_fsdev_iothrottle(Monitor *mon, const QDict *qdict); +#endif void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict); void hmp_block_stream(Monitor *mon, const QDict *qdict); void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict);
This patch introduces hmp interfaces for the fsdev devices. Signed-off-by: Pradeep Jagadeesh <pradeep.jagadeesh@huawei.com> --- hmp-commands-info.hx | 18 +++++++++++++++ hmp-commands.hx | 19 ++++++++++++++++ hmp.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++ hmp.h | 4 ++++ 4 files changed, 103 insertions(+)