Re: [Qemu-block] [PATCH v4 5/7] block: add throttle block filter driver
On Wed, Aug 09, 2017 at 01:07:32PM +0300, Manos Pitsidianakis wrote:
+static int coroutine_fn throttle_co_preadv(BlockDriverState *bs,
+ uint64_t offset, uint64_t bytes,
+ QEMUIOVector *qiov, int flags)
+{
+
+ThrottleGroupMember *tgm = bs->opaque;
+throttle_group_co_io_limits_intercept(tgm, bytes, false);
+
+return bdrv_co_preadv(bs->file, offset, bytes, qiov, flags);
+}
+
+static int coroutine_fn throttle_co_pwritev(BlockDriverState *bs,
+uint64_t offset, uint64_t bytes,
+QEMUIOVector *qiov, int flags)
+{
+ThrottleGroupMember *tgm = bs->opaque;
+throttle_group_co_io_limits_intercept(tgm, bytes, true);
+
+return bdrv_co_preadv(bs->file, offset, bytes, qiov, flags);
^
Tried some write throttling testing, noticed this. If anyone wants to
test this iteration, change this to bdrv_co_pwritev(), I will correct
this in the next version. (let's pretend this never happened!)
signature.asc
Description: PGP signature
Re: [Qemu-block] [PATCH v4 5/7] block: add throttle block filter driver
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(_opts, NULL, 0,
_abort);
+
+qemu_opts_absorb_qdict(opts, options, _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, );
+
+/* Change limits if user has specified them */
+if (throttle_extract_options(opts, , errp) ||
+!throttle_is_valid(, errp)) {
+throttle_group_unregister_tgm(tgm);
+goto err;
+}
+/* Update group configuration */
+throttle_group_config(tgm, );
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
Re: [Qemu-block] [PATCH v4 5/7] block: add throttle block filter driver
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.
> +Error *local_err = NULL;
> +QemuOpts *opts = qemu_opts_create(_opts, NULL, 0, _abort);
> +
> +qemu_opts_absorb_qdict(opts, options, _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, );
> +
> +/* Change limits if user has specified them */
> +if (throttle_extract_options(opts, , errp) ||
> +!throttle_is_valid(, errp)) {
> +throttle_group_unregister_tgm(tgm);
> +goto err;
> +}
> +/* Update group configuration */
> +throttle_group_config(tgm, );
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.
> +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:
ThrottleGroupMember *old_tgm = state->bs->opaque;
ThrottleGroupMember *new_tgm = state->opaque;
throttle_group_unregister_tgm(old_tgm);
g_free(old_tgm);
state->bs->opaque = new_tgm;
state->opaque = NULL;
> +static void throttle_reopen_abort(BDRVReopenState *state)
> +{
> +ThrottleGroupMember *tgm = state->opaque;
> +
> +throttle_group_unregister_tgm(tgm);
> +g_free(state->opaque);
> +state->opaque = NULL;
> +}
Similar problem here (this one is simpler though).
Apart from the general question of whether we're parsing the limits in
this driver, the rest are just comments about the style. Otherwise the
code looks good, thanks!
Berto
