Re: [Qemu-devel] [PATCH v5 10/13] blockdev: Keep track of monitor-owned BDS

[Max Reitz <mreitz@redhat.com>]

On 2015-03-20 at 04:04, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> On 03/03/2015 01:13 PM, Max Reitz wrote:
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> ---
>>>   block.c                                |  2 ++
>>>   blockdev.c                             | 18 ++++++++++++++++++
>>>   include/block/block_int.h              |  4 ++++
>>>   stubs/Makefile.objs                    |  1 +
>>>   stubs/blockdev-close-all-bdrv-states.c |  5 +++++
>>>   5 files changed, 30 insertions(+)
>>>   create mode 100644 stubs/blockdev-close-all-bdrv-states.c
>> Again, might be nice for the commit message to document why adding this
>> is useful, but doesn't affect the code.
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
> Might be nice?  This absolutely needs an explanation, in the code!
>
> Why do we need a separate list of "monitor-owned BDS"?

I thought it self-evident, but you're right, of course: The concept 
being that the monitor can hold references to BDS ("owning them", see 
below), of course it should keep track of those references (which I 
thought didn't need an explanation, because if you hold a reference, 
well, you better actually hold one).

> What makes a BDS "monitor-owned"?

Being created explicitly.

BDS created implicitly because a BB was created (-drive, blockdev-add 
with @id; in these cases, the BBs are monitor-owned) or that are not the 
root of a BDS tree (thus created implicitly in that tree) are not 
monitor-owned. All BDS created explicitly (blockdev-add without @id) are 
monitor-owned.

> What are the invariants governing relations among bdrv_states,
> graph_bdrv_states (what a horrible name) and blk_backends?

bdrv_states is removed in the follow-up series. I think it's legacy 
cruft which we needed at a time where we did not have BlockBackends: It 
tracks all the BDSs with a BB, basically (a BDS is inserted there in 
bdrv_new_root() which is only called from blk_new_with_bs()). Therefore, 
we don't need it, as we have a list of all the BBs already.

graph_bdrv_states tracks all BDSs with a node name. We need that because 
we want to find BDSs by node name.

blk_backends, which supersedes bdrv_states, basically, will, after the 
follow-up, contain every single BlockBackend there is. It will be 
different from monitor_block_backends (follow-up...) in that the latter 
only contains the monitor-owned BBs.

Why we need that difference is a question for the follow-up. However, I 
can answer it here, too: We do have some operations that should actually 
be run over all BlockBackends, like blk_name_taken(), blk_drain_all(), 
blk_flush_all(), and so on. However, all the BBs that are not 
monitor-owned should not be visible to the user (the monitor!), so for 
operations like blk_by_name() or qmp_query_blockstats(), only the 
monitor-owned BBs should be considered.

What makes a BB monitor-owned? It's monitor-owned if it has been created 
using -drive or blockdev-add. Are there any other BBs right now? Except 
for qemu-{img,io,nbd}, there is only xen_disk.c, so practically, no.

What makes a BB monitor-unowned? Deleting its reference through a 
monitor command while there are still other users (in theory that would 
be NBD, block jobs, etc., although I don't know whether that actually 
works that way right now). So it is probably possible to have 
non-monitor-owned BBs, but those should not be visible to the user. 
However, we need to consider them inside of the block layer whenever we 
want to do something for actually all the BBs.

Side note: As far as I know, we currently only have drive_del to drop 
the monitor reference to a BB. What happens there before the follow-up 
is the BB is made anonymous and removed from blk_backends 
(blk_hide_on_behalf_of_hmp_drive_del()) and the root BDS from 
bdrv_states (it's really redundant), so before that series, blk_backends 
is basically the list of monitor-owned BBs (which I don't think is 
right, it should contain all BBs, and we need a separate list for the 
monitor-owned ones).

I hope I was able to explain myself.

Max