[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 3/3] block: remove legacy I/O throttling
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-block] [PATCH 3/3] block: remove legacy I/O throttling |
Date: |
Wed, 2 Aug 2017 11:07:24 +0100 |
User-agent: |
Mutt/1.8.3 (2017-05-23) |
On Tue, Aug 01, 2017 at 04:49:07PM +0300, Manos Pitsidianakis wrote:
> diff --git a/block.c b/block.c
> index 9ebdba28b0..c6aad25286 100644
> --- a/block.c
> +++ b/block.c
> @@ -1975,6 +1975,7 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState
> *child_bs,
> child = g_new(BdrvChild, 1);
> *child = (BdrvChild) {
> .bs = NULL,
> + .parent_bs = NULL,
> .name = g_strdup(child_name),
> .role = child_role,
> .perm = perm,
> @@ -2009,6 +2010,7 @@ BdrvChild *bdrv_attach_child(BlockDriverState
> *parent_bs,
> if (child == NULL) {
> return NULL;
> }
> + child->parent_bs = parent_bs;
>
> QLIST_INSERT_HEAD(&parent_bs->children, child, next);
> return child;
> @@ -3729,6 +3731,12 @@ const char *bdrv_get_parent_name(const
> BlockDriverState *bs)
> return name;
> }
> }
> + if (c->parent_bs && c->parent_bs->implicit) {
> + name = bdrv_get_parent_name(c->parent_bs);
> + if (name && *name) {
> + return name;
> + }
> + }
> }
>
> return NULL;
This should be a separate patch.
Who updates parent_bs if the parent is changed (e.g.
bdrv_replace_node())?
We already have bs->parents. Why is BdrvChild->parent_bs needed?
> -void blk_io_limits_disable(BlockBackend *blk)
> +void blk_io_limits_disable(BlockBackend *blk, Error **errp)
> {
> - assert(blk->public.throttle_group_member.throttle_state);
> - bdrv_drained_begin(blk_bs(blk));
Is it safe to drop drained_begin? We must ensure that no I/O requests
run during this function.
> - throttle_group_unregister_tgm(&blk->public.throttle_group_member);
> - bdrv_drained_end(blk_bs(blk));
> + BlockDriverState *bs, *throttle_node;
> +
> + throttle_node = blk_get_public(blk)->throttle_node;
Is blk_get_public() still necessary? Perhaps we can do away with the
concept of the public struct now. It doesn't need to be done in this
patch though.
> +
> + assert(throttle_node && throttle_node->refcnt == 1);
Are you sure the throttle_node->refcnt == 1 assertion holds? For
example, does the built-in NBD server have a reference to the throttle
node if nbd-server-add is called after throttling has been enabled?
Since we have the blk->throttle_node pointer we know we're the owner.
Others may be using the node too but we may choose to remove it at any
time.
signature.asc
Description: PGP signature