[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 3/3] block: remove legacy I/O throttling
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH 3/3] block: remove legacy I/O throttling |
Date: |
Wed, 2 Aug 2017 15:36:07 +0100 |
User-agent: |
Mutt/1.8.3 (2017-05-23) |
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