On 24.02.2016 10:28, Markus Armbruster wrote: > Max Reitz <mre...@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
signature.asc
Description: OpenPGP digital signature
