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

[Stefan Hajnoczi]

On Wed, Aug 02, 2017 at 01:34:46PM +0300, Manos Pitsidianakis wrote:
> 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:
> > > @@ -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).

Please see Kevin's reply to my email.

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

That would be a nice cleanup, thanks!

signature.asc
Description: PGP signature