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

[Manos Pitsidianakis]

On Wed, Aug 02, 2017 at 12:33:19PM +0200, Kevin Wolf wrote:
> Am 02.08.2017 um 12:07 hat Stefan Hajnoczi geschrieben:
> > 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?
>
> I haven't look at the whole patch yet, but BdrvChild->parent_bs is a
> thing that intentionally doesn't exist. A node simply has no business
> knowing its parents - which may or may not be BlockDriverStates (the
> obvious example where they aren't BDSes are BlockBackends, but block
> jobs own some BdrvChild objects, too).
>
> Usually the replacement is a BdrvChildRole callback.
>
> Kevin

I think accessing the parent bs is necessary for the reasons I specified 
in my reply to Stefan. Will a callback that returns BdrvChild->opaque 
(parent_bs) for child_file and child_backing be okay?

signature.asc
Description: PGP signature