qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH v3 07/14] blockdev: Add list of monitor-owned BlockBackends
Date: Wed, 24 Feb 2016 10:28:13 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)

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.

> 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.

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?

> However, I still believe this patch itself to be useful.
>
> Max



reply via email to

[Prev in Thread] Current Thread [Next in Thread]