Re: [Qemu-block] [PATCH RFC v3 3/8] block: add throttle block filter driver

[Stefan Hajnoczi]

On Fri, Jun 23, 2017 at 03:46:55PM +0300, Manos Pitsidianakis wrote:
> +static int throttle_configure_tgm(BlockDriverState *bs, ThrottleGroupMember 
> *tgm,
> +                                                    QDict *options, Error 
> **errp)
> +{
> +    int ret = 0;
> +    ThrottleState *ts;
> +    ThrottleTimers *tt;
> +    ThrottleConfig cfg;
> +    QemuOpts *opts = NULL;
> +    const char *group_name = NULL;
> +    Error *local_err = NULL;
> +
> +    opts = qemu_opts_create(&throttle_opts, NULL, 0, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return -EINVAL;
> +    }
> +    qemu_opts_absorb_qdict(opts, options, &local_err);
> +    if (local_err) {
> +        goto err;
> +    }
> +
> +    group_name = qemu_opt_get(opts, QEMU_OPT_THROTTLE_GROUP_NAME);
> +    if (!group_name) {
> +        group_name = bdrv_get_device_or_node_name(bs);
> +        if (!strlen(group_name)) {
> +            error_setg(&local_err,
> +                       "A group name must be specified for this device.");

To make the error message clearer:
s/A group name/A throttle group name/

> +static int throttle_open(BlockDriverState *bs, QDict *options,
> +                            int flags, Error **errp)
> +{
> +    ThrottleGroupMember *tgm = bs->opaque;
> +    Error *local_err = NULL;
> +
> +    bs->open_flags = flags;

Why is this necessary?  The other block drivers don't do it.  It should
be taken care of in block.c.

> +    bs->file = bdrv_open_child(NULL, options, "file",
> +                                    bs, &child_file, false, &local_err);
> +
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return -EINVAL;
> +    }
> +
> +    qdict_flatten(options);
> +    return throttle_configure_tgm(bs, tgm, options, errp);

Who destroys bs->file on error?

> +}
> +
> +static void throttle_close(BlockDriverState *bs)
> +{
> +    ThrottleGroupMember *tgm = bs->opaque;
> +    bdrv_drained_begin(bs);

Why is this necessary?  bdrv_close() already encloses drv->bdrv_close()
in a bdrv_drained_begin/end region.

> +    throttle_group_unregister_tgm(tgm);
> +    bdrv_drained_end(bs);
> +    return;

Please omit return at the end of a void function.  They are a
distraction.

> +static void throttle_reopen_commit(BDRVReopenState *state)
> +{
> +    ThrottleGroupMember *tgm = state->bs->opaque;
> +    BlockDriverState *bs = state->bs;
> +
> +    bdrv_drained_begin(bs);

Why is this necessary?  bdrv_reopen_multiple() calls
bdrv_reopen_commit() from within a bdrv_drain_all_begin()/end() region.

> +static BlockDriver bdrv_throttle = {
> +    .format_name                        =   "throttle",
> +    .protocol_name                      =   "throttle",
> +    .instance_size                      =   sizeof(ThrottleGroupMember),
> +
> +    .bdrv_file_open                     =   throttle_open,
> +    .bdrv_close                         =   throttle_close,
> +    .bdrv_co_flush                      =   throttle_co_flush,
> +
> +    .bdrv_child_perm                    =   bdrv_filter_default_perms,
> +
> +    .bdrv_getlength                     =   throttle_getlength,
> +
> +    .bdrv_co_preadv                     =   throttle_co_preadv,
> +    .bdrv_co_pwritev                    =   throttle_co_pwritev,
> +
> +    .bdrv_co_pwrite_zeroes              =   throttle_co_pwrite_zeroes,
> +    .bdrv_co_pdiscard                   =   throttle_co_pdiscard,
> +
> +    .bdrv_recurse_is_first_non_filter   =   bdrv_recurse_is_first_non_filter,
> +
> +    .bdrv_attach_aio_context            =   throttle_attach_aio_context,
> +    .bdrv_detach_aio_context            =   throttle_detach_aio_context,
> +
> +    .bdrv_reopen_prepare                =   throttle_reopen_prepare,
> +    .bdrv_reopen_commit                 =   throttle_reopen_commit,
> +    .bdrv_reopen_abort                  =   throttle_reopen_abort,
> +
> +    .is_filter                          =   true,
> +};

Missing:
bdrv_co_get_block_status()
bdrv_truncate()
bdrv_get_info()
bdrv_probe_blocksizes()
bdrv_probe_geometry()
bdrv_media_changed()
bdrv_eject()
bdrv_lock_medium()
bdrv_co_ioctl()

See block/raw-format.c.

I think most of these could be modified in block.c or block/io.c to
automatically call bs->file's function if drv doesn't implement them.
This way all block drivers would transparently pass them through by
default and block/raw-format.c code could be eliminated.

signature.asc
Description: PGP signature