On Thu, Aug 03, 2017 at 10:07:41AM +0200, Kevin Wolf wrote:
Am 31.07.2017 um 11:54 hat Manos Pitsidianakis geschrieben:+/* Extract ThrottleConfig options. Assumes cfg is initialized and will be + * checked for validity. + */ +static int throttle_extract_options(QemuOpts *opts, ThrottleConfig *cfg, + Error **errp) +{ + if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_TOTAL)) { + cfg->buckets[THROTTLE_BPS_TOTAL].avg = + qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_TOTAL, + 0); + } + if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_READ)) { + cfg->buckets[THROTTLE_BPS_READ].avg = + qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_READ, + 0); + } + if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_WRITE)) { + cfg->buckets[THROTTLE_BPS_WRITE].avg = + qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_WRITE, + 0); + } + if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_IOPS_TOTAL)) { + cfg->buckets[THROTTLE_OPS_TOTAL].avg = + qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX QEMU_OPT_IOPS_TOTAL, + 0); + } + if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_IOPS_READ)) { + cfg->buckets[THROTTLE_OPS_READ].avg = + qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX QEMU_OPT_IOPS_READ, + 0); + } + if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_IOPS_WRITE)) { + cfg->buckets[THROTTLE_OPS_WRITE].avg = + qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX QEMU_OPT_IOPS_WRITE, + 0); + } + if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_TOTAL_MAX)) { + cfg->buckets[THROTTLE_BPS_TOTAL].max = + qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX + QEMU_OPT_BPS_TOTAL_MAX, 0); + } + if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_READ_MAX)) { + cfg->buckets[THROTTLE_BPS_READ].max = + qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX + QEMU_OPT_BPS_READ_MAX, 0); + } + if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_WRITE_MAX)) { + cfg->buckets[THROTTLE_BPS_WRITE].max = + qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX + QEMU_OPT_BPS_WRITE_MAX, 0); + } + if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_IOPS_TOTAL_MAX)) { + cfg->buckets[THROTTLE_OPS_TOTAL].max = + qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX + QEMU_OPT_IOPS_TOTAL_MAX, 0); + } + if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_IOPS_READ_MAX)) { + cfg->buckets[THROTTLE_OPS_READ].max = + qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX + QEMU_OPT_IOPS_READ_MAX, 0); + } + if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_IOPS_WRITE_MAX)) { + cfg->buckets[THROTTLE_OPS_WRITE].max = + qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX + QEMU_OPT_IOPS_WRITE_MAX, 0); + } + if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_TOTAL_MAX_LENGTH)) { + if (qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX + QEMU_OPT_BPS_TOTAL_MAX_LENGTH, 1) > UINT_MAX) { + error_setg(errp, "%s value must be in the range [0, %u]", + THROTTLE_OPT_PREFIX QEMU_OPT_BPS_TOTAL_MAX_LENGTH, + UINT_MAX); + return -1; + } + cfg->buckets[THROTTLE_BPS_TOTAL].burst_length = + qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX + QEMU_OPT_BPS_TOTAL_MAX_LENGTH, 1); + } + if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_READ_MAX_LENGTH)) { + if (qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX + QEMU_OPT_BPS_READ_MAX_LENGTH, 1) > UINT_MAX) { + error_setg(errp, "%s must be in the range [0, %u]", + THROTTLE_OPT_PREFIX QEMU_OPT_BPS_READ_MAX_LENGTH, + UINT_MAX); + return -1; + } + cfg->buckets[THROTTLE_BPS_READ].burst_length = + qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX + QEMU_OPT_BPS_READ_MAX_LENGTH, 1); + } + if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_BPS_WRITE_MAX_LENGTH)) { + if (qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX + QEMU_OPT_BPS_WRITE_MAX_LENGTH, 1) > UINT_MAX) { + error_setg(errp, "%s must be in the range [0, %u]", + THROTTLE_OPT_PREFIX QEMU_OPT_BPS_WRITE_MAX_LENGTH, + UINT_MAX); + return -1; + } + cfg->buckets[THROTTLE_BPS_WRITE].burst_length = + qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX + QEMU_OPT_BPS_WRITE_MAX_LENGTH, 1); + } + if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_IOPS_TOTAL_MAX_LENGTH)) { + if (qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX + QEMU_OPT_IOPS_TOTAL_MAX_LENGTH, 1) > UINT_MAX) { + error_setg(errp, "%s must be in the range [0, %u]", + THROTTLE_OPT_PREFIX QEMU_OPT_IOPS_TOTAL_MAX_LENGTH, + UINT_MAX); + return -1; + } + cfg->buckets[THROTTLE_OPS_TOTAL].burst_length = + qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX + QEMU_OPT_IOPS_TOTAL_MAX_LENGTH, 1); + } + if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_IOPS_READ_MAX_LENGTH)) { + if (qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX + QEMU_OPT_IOPS_READ_MAX_LENGTH, 1) > UINT_MAX) { + error_setg(errp, "%s must be in the range [0, %u]", + THROTTLE_OPT_PREFIX QEMU_OPT_IOPS_READ_MAX_LENGTH, + UINT_MAX); + return -1; + } + cfg->buckets[THROTTLE_OPS_READ].burst_length = + qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX + QEMU_OPT_IOPS_READ_MAX_LENGTH, 1); + } + if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_IOPS_WRITE_MAX_LENGTH)) { + if (qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX + QEMU_OPT_IOPS_WRITE_MAX_LENGTH, 1) > UINT_MAX) { + error_setg(errp, "%s must be in the range [0, %u]", + THROTTLE_OPT_PREFIX QEMU_OPT_IOPS_WRITE_MAX_LENGTH, + UINT_MAX); + return -1; + } + cfg->buckets[THROTTLE_OPS_WRITE].burst_length = + qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX + QEMU_OPT_IOPS_WRITE_MAX_LENGTH, 1); + } + if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX QEMU_OPT_IOPS_SIZE)) { + cfg->op_size = + qemu_opt_get_number(opts, THROTTLE_OPT_PREFIX QEMU_OPT_IOPS_SIZE, + 0); + } + return 0; +}This function is very repetitive, but each block is long enough that you have to look closely to review whether the right constants are used everywhere. Maybe this could become a bit more readable with a macro or two?
How about this?
# #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);
# IF_OPT_SET(cfg->buckets[THROTTLE_BPS_READ].max , QEMU_OPT_BPS_READ_MAX);
# IF_OPT_SET(cfg->buckets[THROTTLE_BPS_WRITE].max,
QEMU_OPT_BPS_WRITE_MAX);
# IF_OPT_SET(cfg->buckets[THROTTLE_OPS_TOTAL].max,
QEMU_OPT_IOPS_TOTAL_MAX);
# IF_OPT_SET(cfg->buckets[THROTTLE_OPS_READ].max, QEMU_OPT_IOPS_READ_MAX);
# IF_OPT_SET(cfg->buckets[THROTTLE_OPS_WRITE].max,
QEMU_OPT_IOPS_WRITE_MAX);
# IF_OPT_SET(cfg->op_size, QEMU_OPT_IOPS_SIZE);
# #define IF_OPT_UINT_SET(rvalue,opt_name) \
# if (qemu_opt_get(opts, THROTTLE_OPT_PREFIX opt_name)) { \
# if (qemu_opt_get_number(opts, \
# THROTTLE_OPT_PREFIX opt_name, 1) > UINT_MAX) { \
# error_setg(errp, "%s value must be in the range [0, %u]", \
# THROTTLE_OPT_PREFIX opt_name, UINT_MAX); \
# return -1; \
# } \
# rvalue = qemu_opt_get_number(opts, opt_name, 1); \
# }
# IF_OPT_UINT_SET(cfg->buckets[THROTTLE_BPS_TOTAL].burst_length,
# QEMU_OPT_BPS_TOTAL_MAX_LENGTH);
# IF_OPT_UINT_SET(cfg->buckets[THROTTLE_BPS_READ].burst_length,
# QEMU_OPT_BPS_READ_MAX_LENGTH);
# IF_OPT_UINT_SET(cfg->buckets[THROTTLE_BPS_WRITE].burst_length,
# QEMU_OPT_BPS_WRITE_MAX_LENGTH);
# IF_OPT_UINT_SET(cfg->buckets[THROTTLE_OPS_TOTAL].burst_length,
# QEMU_OPT_IOPS_TOTAL_MAX_LENGTH);
# IF_OPT_UINT_SET(cfg->buckets[THROTTLE_OPS_READ].burst_length,
# QEMU_OPT_IOPS_READ_MAX_LENGTH);
# IF_OPT_UINT_SET(cfg->buckets[THROTTLE_OPS_WRITE].burst_length,
# QEMU_OPT_IOPS_WRITE_MAX_LENGTH);
+static int throttle_configure_tgm(BlockDriverState *bs, + ThrottleGroupMember *tgm, + QDict *options, Error **errp) +{ + int ret; + ThrottleConfig cfg; + const char *group_name = NULL; + Error *local_err = NULL; + QemuOpts *opts = qemu_opts_create(&throttle_opts, NULL, 0, &local_err); + if (local_err) { + error_propagate(errp, local_err); + return -EINVAL; + }As Stefan said, qemu_opts_create() can't fail if you pass NULL for id, so you can just use &error_abort.+ + qemu_opts_absorb_qdict(opts, options, &local_err); + if (local_err) { + error_propagate(errp, local_err); + goto err; + } + + /* If no name is specified, 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));The documentation of throttle_group_register_tgm() suggests that you have to pass a string here, but group_name can be NULL if the option wasn't given. Probably just means that the comment for that function needs to be updated.+ /* 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); + + ret = 0; + goto fin; + +err: + ret = -EINVAL; +fin: + qemu_opts_del(opts); + return ret; +} + +static int throttle_open(BlockDriverState *bs, QDict *options, + int flags, Error **errp) +{ + ThrottleGroupMember *tgm = bs->opaque; + + bs->file = bdrv_open_child(NULL, options, "file", + bs, &child_file, false, errp);Indentation is off.+ if (!bs->file) { + return -EINVAL; + } + + return throttle_configure_tgm(bs, tgm, options, errp); +} + +static void throttle_close(BlockDriverState *bs) +{ + ThrottleGroupMember *tgm = bs->opaque; + throttle_group_unregister_tgm(tgm); +} + + +static int64_t throttle_getlength(BlockDriverState *bs) +{ + return bdrv_getlength(bs->file->bs); +} + + +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); +}I think we want to set BlockDriver.supported_write_flags so that passing down flags is actually of any use.
Can you explain what you mean? Do you mean doing this in throttle_open():
bs->supported_write_flags = bs->file->bs->supported_write_flags; bs->supported_zero_flags = bs->file->bs->supported_zero_flags;
+ +static int coroutine_fn throttle_co_pwrite_zeroes(BlockDriverState *bs, + int64_t offset, int bytes, BdrvRequestFlags flags) +{ + ThrottleGroupMember *tgm = bs->opaque; + throttle_group_co_io_limits_intercept(tgm, bytes, true); + + return bdrv_co_pwrite_zeroes(bs->file, offset, bytes, flags); +}The same is true for BlockDriver.supported_zero_flags.+static int coroutine_fn throttle_co_pdiscard(BlockDriverState *bs, + int64_t offset, int bytes) +{ + ThrottleGroupMember *tgm = bs->opaque; + throttle_group_co_io_limits_intercept(tgm, bytes, true); + + return bdrv_co_pdiscard(bs->file->bs, offset, bytes); +} + +static int throttle_co_flush(BlockDriverState *bs) +{ + return bdrv_co_flush(bs->file->bs); +} + +static void throttle_detach_aio_context(BlockDriverState *bs) +{ + ThrottleGroupMember *tgm = bs->opaque; + throttle_group_detach_aio_context(tgm); +} + +static void throttle_attach_aio_context(BlockDriverState *bs, + AioContext *new_context)Indentation is off here...+{ + ThrottleGroupMember *tgm = bs->opaque; + throttle_group_attach_aio_context(tgm, new_context); +} + +static int throttle_reopen_prepare(BDRVReopenState *reopen_state, + BlockReopenQueue *queue, Error **errp)...and here.+{ + 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); +} + +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; +} + +static void throttle_reopen_abort(BDRVReopenState *state) +{ + ThrottleGroupMember *tgm = state->opaque; + + throttle_group_unregister_tgm(tgm); + g_free(state->opaque); + state->opaque = NULL; +} + +static bool throttle_recurse_is_first_non_filter(BlockDriverState *bs, + BlockDriverState *candidate) +{ + return bdrv_recurse_is_first_non_filter(bs->file->bs, candidate); +} + +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 = throttle_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, +};What about .bdrv_co_get_block_status?
We should use the default implementation here that you merged into your 2.11 tree, thanks for noticing and for all the comments.
+static void bdrv_throttle_init(void) +{ + bdrv_register(&bdrv_throttle); +} + +block_init(bdrv_throttle_init); diff --git a/include/qemu/throttle-options.h b/include/qemu/throttle-options.h index 182b7896e1..3528a8f4a2 100644 --- a/include/qemu/throttle-options.h +++ b/include/qemu/throttle-options.h @@ -29,6 +29,7 @@ #define QEMU_OPT_BPS_WRITE_MAX "bps-write-max" #define QEMU_OPT_BPS_WRITE_MAX_LENGTH "bps-write-max-length" #define QEMU_OPT_IOPS_SIZE "iops-size" +#define QEMU_OPT_THROTTLE_GROUP_NAME "throttle-group" #define THROTTLE_OPT_PREFIX "throttling." #define THROTTLE_OPTS \Kevin
signature.asc
Description: PGP signature
