Re: [Qemu-devel] [PATCH v4 5/7] block: add throttle block filter driver

[Manos Pitsidianakis]

On Thu, Aug 10, 2017 at 03:54:02PM +0200, Alberto Garcia wrote:
> On Wed 09 Aug 2017 12:07:32 PM CEST, Manos Pitsidianakis wrote:
> > +/* Extract ThrottleConfig options. Assumes cfg is initialized and will be
> > + * checked for validity.
> > + *
> > + * Returns -1 and sets errp if a burst_length value is over UINT_MAX.
> > + */
> > +static int throttle_extract_options(QemuOpts *opts, ThrottleConfig *cfg,
> > +                                    Error **errp)
> > +{
> > +#define IF_OPT_SET(rvalue, opt_name) \
> > +    if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX opt_name)) { \
> > +        rvalue = qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX opt_name, 0); }
> > +
> > +    IF_OPT_SET(cfg->buckets[THROTTLE_BPS_TOTAL].avg, QEMU_OPT_BPS_TOTAL);
> > +    IF_OPT_SET(cfg->buckets[THROTTLE_BPS_READ].avg, QEMU_OPT_BPS_READ);
> > +    IF_OPT_SET(cfg->buckets[THROTTLE_BPS_WRITE].avg, QEMU_OPT_BPS_WRITE);
> > +    IF_OPT_SET(cfg->buckets[THROTTLE_OPS_TOTAL].avg, QEMU_OPT_IOPS_TOTAL);
> > +    IF_OPT_SET(cfg->buckets[THROTTLE_OPS_READ].avg, QEMU_OPT_IOPS_READ);
> > +    IF_OPT_SET(cfg->buckets[THROTTLE_OPS_WRITE].avg, QEMU_OPT_IOPS_WRITE);
> > +    IF_OPT_SET(cfg->buckets[THROTTLE_BPS_TOTAL].max, QEMU_OPT_BPS_TOTAL_MAX);
>  [...]
>
> This is all the code that I was saying that we'd save if we don't allow
> setting limits here.
>
> > +static int throttle_configure_tgm(BlockDriverState *bs,
> > +                                  ThrottleGroupMember *tgm,
> > +                                  QDict *options, Error **errp)
> > +{
> > +    int ret;
> > +    ThrottleConfig cfg;
> > +    const char *group_name = NULL;
>
> No need to set it to NULL here.

I know, I do it out of habit!

> > +    Error *local_err = NULL;
> > +    QemuOpts *opts = qemu_opts_create(&throttle_opts, NULL, 0, 
> > &error_abort);
> > +
> > +    qemu_opts_absorb_qdict(opts, options, &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        goto err;
> > +    }
> > +
> > +    /* If group_name is NULL, an anonymous group will be created */
> > +    group_name = qemu_opt_get(opts, QEMU_OPT_THROTTLE_GROUP_NAME);
> > +
> > +    /* Register membership to group with name group_name */
> > +    throttle_group_register_tgm(tgm, group_name, bdrv_get_aio_context(bs));
> > +
> > +    /* Copy previous configuration */
> > +    throttle_group_get_config(tgm, &cfg);
> > +
> > +    /* Change limits if user has specified them */
> > +    if (throttle_extract_options(opts, &cfg, errp) ||
> > +        !throttle_is_valid(&cfg, errp)) {
> > +        throttle_group_unregister_tgm(tgm);
> > +        goto err;
> > +    }
> > +    /* Update group configuration */
> > +    throttle_group_config(tgm, &cfg);
>
> We'd also spare this, and this function would remain much simpler.
>
> > +
> > +    ret = 0;
> > +    goto fin;
> > +
> > +err:
> > +    ret = -EINVAL;
> > +fin:
> > +    qemu_opts_del(opts);
> > +    return ret;
> > +}
>
> If you set ret = -EINVAL before calling goto err you can simplify this
> part as well, but feel free to ignore this suggestion.
>
> > +static int throttle_reopen_prepare(BDRVReopenState *reopen_state,
> > +                                   BlockReopenQueue *queue, Error **errp)
> > +{
> > +    ThrottleGroupMember *tgm = NULL;
> > +
> > +    assert(reopen_state != NULL);
> > +    assert(reopen_state->bs != NULL);
> > +
> > +    reopen_state->opaque = g_new0(ThrottleGroupMember, 1);
> > +    tgm = reopen_state->opaque;
> > +
> > +    return throttle_configure_tgm(reopen_state->bs, tgm, reopen_state->options,
> > +            errp);
> > +}
>
> I would rename 'reopen_state' as 'state' for consistency with the other
> two functions.

The function signatures in block_int.h have reopen_state, so maybe for 
consistency I should change the other two to reopen_state as well, 
instead.

> > +static void throttle_reopen_commit(BDRVReopenState *state)
> > +{
> > +    ThrottleGroupMember *tgm = state->bs->opaque;
> > +
> > +    throttle_group_unregister_tgm(tgm);
> > +    g_free(state->bs->opaque);
> > +    state->bs->opaque = state->opaque;
> > +    state->opaque = NULL;
> > +}
>
> I also find the mixing of state->bs->opaque and tgm a bit confusing.
> Here's a suggestion, but feel free to ignore it:

You're right, though it's only a few lines it might require a second 
read. I will rewrite those more clearly, too.

signature.asc
Description: PGP signature