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

[Manos Pitsidianakis <el13635@mail.ntua.gr>]

[-- Attachment #1: Type: text/plain, Size: 4407 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]