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

[Kevin Wolf]

Am 23.02.2016 um 14:52 hat Max Reitz geschrieben:
> 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 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.
> 
> 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().

This assumption is wrong. Or more precisely, it may be correct now, but
we want block jobs and NBD servers to create their own BBs in the
future. These will be BBs that are created by blk_new(), but not
referenced at all by the monitor.

Kevin

> 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.
> 
> However, I still believe this patch itself to be useful.
> 
> Max
>

pgpbIZFsd2kde.pgp
Description: PGP signature