Re: [Qemu-devel] [PATCH 3/3] block: remove legacy I/O throttling

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

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

On Wed, Aug 02, 2017 at 11:07:24AM +0100, Stefan Hajnoczi wrote:
>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?
>

If I haven't misunderstood this, BdrvChild holds only the child part of 
the parent-child relationship and there's no way to access a parent from 
bs->parents. bdrv_replace_node() will thus only replace the child part 
in BdrvChild from the aspect of the parent. In the old child bs's 
perspective, one of the nodes of bs->parents is removed and in the new 
child bs's perspective a new node in bs->parents was inserted. parent_bs 
thus remains immutable.

child->parent_bs is needed in this patch because in jobs if a job-ID is 
not specified the parent name is used, but this fails if the parent is 
an implicit node instead of BlockBackend and causes a regression 
(certain job setups suddenly need an explicit job ID instead of just 
working).

>> -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.

Thanks, I will put it back in.

>> -    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.

I can include a patch to move throttle_node to BlockBackend and remove 
all BlockBackendPublic code, is that okay?

>
>> +
>> +    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.

Hm.. If that's possible I guess we want the removal to be visible to the 
nbd server too. I will use bdrv_replace_node() instead.

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