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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6073 bytes --]

On 23.02.2016 10:48, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
> 
>> On 22.02.2016 09:24, Markus Armbruster wrote:
>>> Max Reitz <mreitz@redhat.com> 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().

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]