[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: |
Tue, 23 Feb 2016 14:52:41 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 |
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 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
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-block] [PATCH v3 08/14] blockdev: Remove blk_hide_on_behalf_of_hmp_drive_del(), (continued)
- [Qemu-block] [PATCH v3 07/14] blockdev: Add list of monitor-owned BlockBackends, Max Reitz, 2016/02/16
- 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/17
- 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 <=
- 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, 2016/02/24
[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