[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v3 07/14] blockdev: Add list of mon
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v3 07/14] blockdev: Add list of monitor-owned BlockBackends |
Date: |
Wed, 24 Feb 2016 16:25:21 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 |
On 24.02.2016 10:28, Markus Armbruster wrote:
> Max Reitz <address@hidden> 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
- Re: [Qemu-block] [PATCH v3 07/14] blockdev: Add list of monitor-owned BlockBackends, (continued)
- Re: [Qemu-block] [PATCH v3 07/14] blockdev: Add list of monitor-owned BlockBackends, Kevin Wolf, 2016/02/17
- Re: [Qemu-block] [PATCH v3 07/14] blockdev: Add list of monitor-owned BlockBackends, Max Reitz, 2016/02/20
- Re: [Qemu-block] [PATCH v3 07/14] blockdev: Add list of monitor-owned BlockBackends, Max Reitz, 2016/02/20
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 07/14] blockdev: Add list of monitor-owned BlockBackends, Markus Armbruster, 2016/02/22
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 07/14] blockdev: Add list of monitor-owned BlockBackends, Max Reitz, 2016/02/22
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 07/14] blockdev: Add list of monitor-owned BlockBackends, Markus Armbruster, 2016/02/23
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 07/14] blockdev: Add list of monitor-owned BlockBackends, Max Reitz, 2016/02/23
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 07/14] blockdev: Add list of monitor-owned BlockBackends, Kevin Wolf, 2016/02/23
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 07/14] blockdev: Add list of monitor-owned BlockBackends, Markus Armbruster, 2016/02/24
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 07/14] blockdev: Add list of monitor-owned BlockBackends, Kevin Wolf, 2016/02/24
- Re: [Qemu-block] [Qemu-devel] [PATCH v3 07/14] blockdev: Add list of monitor-owned BlockBackends,
Max Reitz <=
[Qemu-block] [PATCH v3 09/14] block: Move some bdrv_*_all() functions to BB, Max Reitz, 2016/02/16
[Qemu-block] [PATCH v3 10/14] block: Add bdrv_next_monitor_owned(), Max Reitz, 2016/02/16
[Qemu-block] [PATCH v3 12/14] block: Rewrite bdrv_next(), Max Reitz, 2016/02/16
[Qemu-block] [PATCH v3 11/14] block: Add blk_next_root_bs(), Max Reitz, 2016/02/16
[Qemu-block] [PATCH v3 13/14] block: Use bdrv_next() instead of bdrv_states, Max Reitz, 2016/02/16
[Qemu-block] [PATCH v3 14/14] block: Remove bdrv_states list, Max Reitz, 2016/02/16
Re: [Qemu-block] [PATCH v3 00/14] blockdev: Further BlockBackend work, Kevin Wolf, 2016/02/17