[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [Qemu-devel] [PATCH v5 10/13] blockdev: Keep track of m

From: Max Reitz
Subject: Re: [Qemu-block] [Qemu-devel] [PATCH v5 10/13] blockdev: Keep track of monitor-owned BDS
Date: Fri, 20 Mar 2015 08:52:16 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0

On 2015-03-20 at 04:04, Markus Armbruster wrote:
Eric Blake <address@hidden> writes:

On 03/03/2015 01:13 PM, Max Reitz wrote:
Signed-off-by: Max Reitz <address@hidden>
  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 <address@hidden>
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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]