Re: [Qemu-devel] [PATCH v6 5/6] fsdev: hmp interface for throttling

[Pradeep Jagadeesh]

On 6/30/2017 11:33 AM, Dr. David Alan Gilbert wrote:
> * Pradeep Jagadeesh (address@hidden) wrote:
> > This patch introduces hmp interfaces for the fsdev
> > devices.
> >
> > Signed-off-by: Pradeep Jagadeesh <address@hidden>
> > ---
> >  hmp-commands-info.hx | 18 ++++++++++++++
> >  hmp-commands.hx      | 19 +++++++++++++++
> >  hmp.c                | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  hmp.h                |  4 ++++
> >  4 files changed, 107 insertions(+)
> >
> > diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> > index ae16901..f23b627 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       = "query-fsdev-iothrottle",
>
> This will end up as:
> info query-fsdev-iothrottle
OK, I fix it as "fsdev_iothrottle"
> so the query- is unneeded since it's already info.
>
> > +        .args_type  = "",
> > +        .params     = "",
> > +        .help       = "show fsdev device throttle information",
> > +        .cmd        = hmp_fsdev_get_io_throttle,
> > +    },
> > +
> > +#endif
> > +
> > +STEXI
> > address@hidden info fsdev throttle
>
> I think that's supposed to match the .name - i.e.
>    @item info query-fsdev-iothrottle
>
>  (or whatever it will become)
OK
> > address@hidden fsdevthrottleinfo
>
> again I think that's supposed to match the .name - see the other entries
> in that file.
OK
> > +Show fsdev device throttleinfo.
>
> And we may as well make that text match the .help text.
OK
> > +ETEXI
> > +
> >      {
> >          .name       = "block-jobs",
> >          .args_type  = "",
> > diff --git a/hmp-commands.hx b/hmp-commands.hx
> > index e763606..c60fd7e 100644
> > --- a/hmp-commands.hx
> > +++ b/hmp-commands.hx
> > @@ -1662,6 +1662,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
> > address@hidden fsdev_set_io_throttle @var{device} @var{bps} @var{bps_rd} 
> > @var{bps_wr} @var{iops} @var{iops_rd} @var{iops_wr}
> > address@hidden fsdev_set_io_throttle
>
> Again I think the @item and @findex have to match the .name - so I think
> make the .name use _'s rather than -'s for HMP.
OK
> > +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 220d301..b1c698b 100644
> > --- a/hmp.c
> > +++ b/hmp.c
> > @@ -1776,6 +1776,72 @@ 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;
> > +
> > +    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)
> > +{
> > +    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);
>
> You've not got anything that can generate an error here have you?
No, I think I can avoid the hmp_handle_error() and also passing of err 
variable to this function.
> > +}
> > +
> > +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);
>
> Since print_fsdev_throttle_config has an if() around it's print,
> doesn't that mean you can end up with a few extra \n's ?
You are right, I removed \n and added it in print_fsdev_throttle_config.

-Pradeep
> > +        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 d8b94ce..05e72f2 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_fsdev_get_io_throttle(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);
> > --
> > 1.8.3.1
>
> Dave
>
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK