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: 4479 bytes --]

On 24.02.2016 10:28, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> writes:
> 
>> On 23.02.2016 10:48, Markus Armbruster wrote:

[...]

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

And I'd like it to be more explicit than this. A BB should simply never
have a name anymore when it's deleted. The current life cycle has a
short time where a named BB can have a refcount of 0.

While I know that this time span is considered to be "atomic", it still
seems off.

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

Maybe, but I'd personally consider this a service offered by the
monitor, and thus a reference 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.
> 
> 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.

Yes, it must, but I believe it shouldn't have a name at that point at all.

> Additional operations to add / remove a BBs name may make sense; I'd
> have to see their users.  See "more complex live cycle" above.

They do make sense, if for nothing else, then because blk_hide...() is
replaced by a function that actually does something that makes sense in
the general life cycle.

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

Which is a reason why we should have said explicit functions.

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

I sure hope so.

> Is a member of blk_backends with a name always a member of
> monitor_block_backends?

No. Right now, after blk_new() it isn't; but Kevin proposed moving the
name setting to monitor_add_blk(), then it will be.

Apart from that, a BB that's passed to blk_delete() is currently
generally still a member of blk_backends (with a name). As said above, I
don't think it should be, and consequently it will not be a member of
monitor_block_backends.

So I guess you could say that I believe that being a member of
blk_backends hand having a name before this patch should be equivalent
to being a member of monitor_block_backends after this patch. It isn't,
because blk_backends contained some BBs which shouldn't be there, in my
opinion.

Max


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