Re: [Qemu-devel] [PATCH v3 07/14] blockdev: Add list of monitor-owned BlockBackends

[Kevin Wolf]

Am 24.02.2016 um 10:28 hat Markus Armbruster geschrieben:
> Max Reitz <address@hidden> writes:
> 
> > On 23.02.2016 10:48, Markus Armbruster wrote:
> >> Max Reitz <address@hidden> writes:
> >> 
> >>> On 22.02.2016 09:24, Markus Armbruster wrote:
> >>>> Max Reitz <address@hidden> writes:
> >>>>
> >>>>> On 17.02.2016 17:20, Kevin Wolf wrote:
> >>>>>> Am 17.02.2016 um 16:41 hat Max Reitz geschrieben:
> >>>>>>> On 17.02.2016 11:53, Kevin Wolf wrote:
> >>>>>>>> Am 16.02.2016 um 19:08 hat Max Reitz geschrieben:
> >>>>>>>>> The monitor does hold references to some BlockBackends so it should 
> >>>>>>>>> have
> >>>>>>>>
> >>>>>>>> s/does hold/holds/?
> >>>>>>>
> >>>>>>> It was intentional, so I'd keep it unless you drop the question mark.
> >>>>>>
> >>>>>> For me it seems to imply something like "contrary to your 
> >>>>>> expectations",
> >>>>>> but maybe that's just my non-native English needing a fix.
> >>>>>>
> >>>>>> I don't find it surprising anyway that the monitor holds BB references.
> >>>>
> >>>> Me neither.
> >>>>
> >>>>> The contrast I tried to point out is that while we do have these
> >>>>> references in theory, and they are reflected by a refcount, too, we do
> >>>>> not actually have these references because the monitor does not yet have
> >>>>> a list of the BBs it owns.
> >>>>
> >>>> Oh yes, it has.  It's just outsources their actual storage to
> >>>> block-backend.c, but that's detail.
> >>>
> >>> In my opinion it's not a reference made by the monitor, then, especially
> >>> because it's done through magic. With this interpretation,
> >>> block-backend.c considers every BB to be monitor-owned (until
> >>> blk_hide...() is called).
> >> 
> >> block-backend.c holds a reference from blk_backends.  By *why* does it
> >> do that?  Let's go through the uses of blk_backends.
> >> 
> >> 0. blk_backends maintenance: blk_new(), blk_delete(),
> >>    blk_hide_on_behalf_of_hmp_drive_del()
> >> 
> >> 1. To permit lookup by name, with blk_by_name().
> >> 
> >>    In my view, block-backend.c holds this reference in trust for those
> >>    parts of QEMU that reference by name rather than by pointer, most
> >>    prominently the monitor.
> >> 
> >>    I can't see the point of backing up the reference by name with a
> >>    reference by pointer in the monitor, like your patch seems to do.
> >>    What's the difference between the two?  Can one ever exist without
> >>    the other?  Why in the monitor, and not in any other part looking up
> >>    by name?
> >> 
> >> 2. To iterate over all named ones, with blk_next().
> >> 
> >>    Since this can only return named BBs, the reference held in trust for
> >>    named lookup suffices.
> >> 
> >> 3. To permit lookup by DriveInfo, with blk_by_legacy_dinfo()
> >> 
> >>    Since this must only return named BBs, the reference held in trust
> >>    for named lookup suffices.
> >> 
> >> 4. To do something so unimportant that it's not worth explaining, with
> >>    blk_remove_bs().
> >> 
> >>    I made a point of giving every single external function a carefully
> >>    worded contract, to hopefully shame future contributors into
> >>    following suit.  Didn't work.
> >
> > Side note: I consider it very important and there was no other way to do
> > it before this series, because there is no list of all block backends.
> >
> >>> Also, if blk_backends is supposed to be the list of monitor-owned BBs,
> >>> then it's a really bad name and I put all the blame on that.
> >> 
> >> Naming is hard.  Feel free to propose better comments and/or better
> >> names.
> >
> > I did. It's "monitor_block_backends".
> >
> >>>>> So it's not an "emphasize the verb because it may be contrary to your
> >>>>> expectations", but an "emphasize it because it is contrary to what the
> >>>>> current code does" (which is not actually referencing these BBs).
> >>>>
> >>>> I disagree :)
> >>>
> >>> Then I'd say "It's contrary to my interpretation of what the current
> >>> code wants to do." Now it's my personal opinion! ;-)
> >>>
> >>> I wouldn't mind removing the "does" from the commit message (obviously),
> >>> but that is not the only thing there which conflicts with your opinion.
> >>> It clearly states that blk_backends is not the list of monitor-owned
> >>> BlockBackends, whereas you are saying that it very much is.
> >>>
> >>> So...? Rephrase it entirely? State that blk_backends is a bad name and
> >>> this commit is basically duplicating blk_backends as
> >>> monitor_block_backends, which is the correct name, and that a follow-up
> >>> commit will make blk_backends contain what it name implies it does? Or
> >>> just call the commit "Remove magic", because it adds explicit functions
> >>> for saying that a BB is monitor-owned or that it is not?
> >> 
> >> Can you explain the *actual* difference between blk_backends and
> >> monitor_block_backends?  "Actual" in the sense of difference in contents
> >> over time.
> >
> > First difference: The name. That's enough reason for me.
> >
> > Second difference: blk_new() adds any BB to blk_backends automatically.
> > It doesn't do so for monitor_block_backends.
> >
> > Third difference: Often the monitor doesn't take care of signalling that
> > it's releasing its reference, only sometimes (where "sometimes" means
> > every time blk_hide...() is called). blk_delete() will instead
> > automatically remove it from blk_backends, because it's assuming that
> > the last reference had been held by the monitor.
> 
> The reference held in trust for lookup by name exists as long as lookup
> by name is permitted.
> 
> Therefore, it goes away when the BB dies or when it loses its name.
> 
> The only way for a BB to lose its name is (was?) the messed up HMP
> drive_del: it wants to kill the BB right away, but can't, so it needs to
> hide it instead until it can.
> 
> New functionality may lead to a more complex live cycle where BBs may
> acquire and lose their name in new ways.
> 
> Note that I carefully avoid calling the reference the monitor's
> reference.  Because you made me realize it's not the monitor's alone!
> Lookup by name has more customers than just the monitor.

Legitimate ones or bugs to fix?

> > The second and third difference are what I referred to as "magic". You
> > could also call them "convenience", if you prefer, but in any case as we
> > can see by the existence blk_hide...(), removing the monitor reference
> > in blk_delete() appears to be wrong. Both should be separate operations,
> > and this is what this patch does.
> 
> I can't see how the existence of blk_hide_on_behalf_of_hmp_drive_del()
> shows that the name going away in blk_delete() is wrong.  It must go
> away there, because the thing it names goes away.

It must go away there _the latest_, but it can (should?) go away earlier.

> Additional operations to add / remove a BBs name may make sense; I'd
> have to see their users.  See "more complex live cycle" above.
> 
> > Assuming that any blk_new() is ultimately done by the monitor, we maybe
> > actually do not need an own monitor_add_blk() function; except that
> > Kevin stated that he does deem it useful when I proposed inlining it
> > (back) into blk_new().
> 
> As Kevin noted, that's not a good assumption.
> 
> > All in all, you have convinced me that the commit message is incorrect.
> > It should state that blk_backends is effectively repurposed to contain
> > the list of all BBs, and that a more explicit monitor_block_backends
> > list will take its place, with explicit operations for the monitor to
> > signal when it takes or releases the reference to a BB.
> 
> A member of monitor_block_backends is always a member of blk_backends.
> Correct?
> 
> Is a member of blk_backends with a name always a member of
> monitor_block_backends?

I would have expected that yes, but apparently you found other places
that use the name, so it might not be as clear as I thought it was.

Kevin