| Message ID | 20180823154655.40188-2-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | block nodes graph visualization | expand |
On 23.08.18 17:46, Vladimir Sementsov-Ogievskiy wrote: > Add a new command, returning block nodes (and their users) graph. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > qapi/block-core.json | 91 +++++++++++++++++++++++ > include/block/block.h | 1 + > include/sysemu/block-backend.h | 2 + > block.c | 129 +++++++++++++++++++++++++++++++++ > block/block-backend.c | 5 ++ > blockdev.c | 5 ++ > 6 files changed, 233 insertions(+) The design looks better (that is to say, good) to me, so I mostly have technical remarks. (Only a bit of bike-shedding this time.) > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 4c7a37afdc..34cdc595d7 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -1629,6 +1629,97 @@ > ## > { 'command': 'query-named-block-nodes', 'returns': [ 'BlockDeviceInfo' ] } > > +## > +# @BlockGraphNodeType: > +# > +# Since: 3.1 > +## > +{ 'enum': 'BlockGraphNodeType', > + 'data': [ 'blk', 'job', 'bds' ] } I wouldn't use abbreviations here, but the full names. At least they should be described. (Though with x-debug-, it doesn't matter too much.) > + > +## > +# @BlockGraphNode: > +# I think a description on at least what the name means for each type would be useful; and that @id is generated just for this request and not some significant value in the block layer. > +# Since: 3.1 > +## > +{ 'struct': 'BlockGraphNode', > + 'data': { 'id': 'uint64', 'type': 'BlockGraphNodeType', 'name': 'str' } } > + > +## > +# @BlockPermission: > +# > +# Enum of base block permissions. > +# > +# @consistent-read: A user that has the "permission" of consistent reads is > +# guaranteed that their view of the contents of the block > +# device is complete and self-consistent, representing the > +# contents of a disk at a specific point. > +# For most block devices (including their backing files) this > +# is true, but the property cannot be maintained in a few > +# situations like for intermediate nodes of a commit block > +# job. > +# > +# @write: This permission is required to change the visible disk contents. > +# > +# @write-unchanged: This permission (which is weaker than BLK_PERM_WRITE) is > +# both enough and required for writes to the block node when > +# the caller promises that the visible disk content doesn't > +# change. > +# As the BLK_PERM_WRITE permission is strictly stronger, > +# either is sufficient to perform an unchanging write. > +# > +# @resize: This permission is required to change the size of a block node. > +# > +# @graph-mod: This permission is required to change the node that this > +# BdrvChild points to. > +# > +# Since: 3.1 > +## > + { 'enum': 'BlockPermission', > + 'data': [ 'consistent-read', 'write', 'write-unchanged', 'resize', > + 'graph-mod' ] } > +## > +# @BlockGraphEdge: > +# > +# Block Graph edge description for x-debug-query-block-graph. > +# > +# @parent: parent id > +# > +# @child: child id > +# > +# @name: name of the relation (examples are 'file' and 'backing') > +# > +# @perm: granted permissions for the parent operating on the child > +# > +# @shared-perm: permissions that can still be granted to other users of the > +# child while it is still attached this parent s/attached this/attached to this/ > +# > +# Since: 3.1 > +## > +{ 'struct': 'BlockGraphEdge', > + 'data': { 'parent': 'uint64', 'child': 'uint64', > + 'name': 'str', 'perm': [ 'BlockPermission' ], > + 'shared-perm': [ 'BlockPermission' ] } } > + > +## > +# @BlockGraph: > +# > +# Block Graph - list of nodes and list of edges. > +# > +# Since: 3.1 > +## > +{ 'struct': 'BlockGraph', > + 'data': { 'nodes': ['BlockGraphNode'], 'edges': ['BlockGraphEdge'] } } > + > +## > +# @x-debug-query-block-graph: > +# > +# Get the block graph. > +# > +# Since: 3.1 > +## > +{ 'command': 'x-debug-query-block-graph', 'returns': 'BlockGraph' } > + > ## > # @drive-mirror: > # [...] > diff --git a/block.c b/block.c > index 6161dbe3eb..588f5a2648 100644 > --- a/block.c > +++ b/block.c > @@ -4003,6 +4003,135 @@ BlockDeviceInfoList *bdrv_named_nodes_list(Error **errp) > return list; > } > > +#define QAPI_LIST_ADD(list, element) do { \ > + typeof(list) _tmp = g_new(typeof(*(list)), 1); \ > + _tmp->value = (element); \ > + _tmp->next = (list); \ > + list = _tmp; \ Interesting, how yoo put (list) as an rvalue in parentheses, but don't do so when it's an lvalue. :-) I can follow your line of thinking, but it should probably always be in parentheses. > +} while (0) > + > +typedef struct BlockGraphConstructor { > + BlockGraph *graph; > + GHashTable *hash; Maybe... "graph_nodes"? "hash" seems awfully unspecific. > +} BlockGraphConstructor; > + > +static BlockGraphConstructor *graph_new(void) I think this is a very broad name. Maybe prefix all of the function names with debug_? (Or dbg_, or dbg_blk, or something) > +{ > + BlockGraphConstructor *gr = g_new(BlockGraphConstructor, 1); > + > + gr->graph = g_new0(BlockGraph, 1); > + gr->hash = g_hash_table_new(NULL, NULL); > + > + return gr; > +} > + > +static BlockGraph *graph_finalize(BlockGraphConstructor *gr) > +{ > + g_hash_table_destroy(gr->hash); > + > + return gr->graph; gr is leaked here. > +} > + > +static uint64_t graph_node_num(BlockGraphConstructor *gr, void *node) > +{ > + uint64_t ret = (uint64_t)g_hash_table_lookup(gr->hash, node); I'd prefer a cast to uintptr_t. Otherwise the compiler may warn that you cast a pointer to an integer of different size (with -m32). Storing it in a uint64_t (with an implicit cast then) is OK, though. But maybe you do want to store it in a uintptr_t. The only reason not to is because it's a uint64_t in the QAPI schema, but I think it'd be a bit cleaner to work with uintptr_t internally and then emit it as uint64_t (because that's definitely safe). It's just a bit more honest because this way it's clear that with -m32, we cannot even represent IDs larger than 32 bit (which doesn't matter). > + > + if (ret > 0) { Just for style I'd prefer != over >. That makes it more clear that this is a NULL check and not a check for errors (represented as negative integers, even though @ret is unsigned). > + return ret; > + } > + > + ret = g_hash_table_size(gr->hash) + 1; Maybe add a comment that you add 1 because 0 (NULL) is reserved for non-entries? (Yes, it's clear, or I wouldn't have figured it out, but I'd still find it nice.) > + g_hash_table_insert(gr->hash, node, (void *)ret); This definitely needs a uintptr_t cast or you'll get a warning for 32 bit pointers. > + > + return ret; > +} > + > +static void graph_add_node(BlockGraphConstructor *gr, void *node, > + BlockGraphNodeType type, const char *name) > +{ > + BlockGraphNode *n; > + > + n = g_new0(BlockGraphNode, 1); > + > + n->id = graph_node_num(gr, node); > + n->type = type; > + n->name = g_strdup(name); > + > + QAPI_LIST_ADD(gr->graph->nodes, n); > +} > + > +static void graph_add_edge(BlockGraphConstructor *gr, void *parent, > + const BdrvChild *child) > +{ > + typedef struct { > + unsigned int flag; > + BlockPermission num; > + } PermissionMap; > + > + static PermissionMap permissions[] = { You can add a const if you like. > + { BLK_PERM_CONSISTENT_READ, BLOCK_PERMISSION_CONSISTENT_READ }, > + { BLK_PERM_WRITE, BLOCK_PERMISSION_WRITE }, > + { BLK_PERM_WRITE_UNCHANGED, BLOCK_PERMISSION_WRITE_UNCHANGED }, > + { BLK_PERM_RESIZE, BLOCK_PERMISSION_RESIZE }, > + { BLK_PERM_GRAPH_MOD, BLOCK_PERMISSION_GRAPH_MOD }, > + { 0, 0 } > + }; You could add a static assertion that "1 << (sizeof(permissions) / sizeof(permissions[0]) - 1) == BLK_PERM_ALL + 1" to ensure that whenever we add a permission, we don't forget to update this list. But then again, I don't think we're going to add a permission any time soon... > + PermissionMap *p; > + BlockGraphEdge *edge; > + > + edge = g_new0(BlockGraphEdge, 1); > + > + edge->parent = graph_node_num(gr, parent); > + edge->child = graph_node_num(gr, child->bs); > + edge->name = g_strdup(child->name); > + > + for (p = permissions; p->flag; p++) { > + if (p->flag & child->perm) { > + QAPI_LIST_ADD(edge->perm, p->num); > + } > + if (p->flag & child->shared_perm) { > + QAPI_LIST_ADD(edge->shared_perm, p->num); > + } > + } > + > + QAPI_LIST_ADD(gr->graph->edges, edge); > +} > + > + > +BlockGraph *bdrv_get_block_graph(Error **errp) > +{ > + BlockBackend *blk; > + BlockJob *job; > + BlockDriverState *bs; > + BdrvChild *child; > + BlockGraphConstructor *gr = graph_new(); > + > + for (blk = blk_all_next(NULL); blk; blk = blk_all_next(blk)) { > + graph_add_node(gr, blk, BLOCK_GRAPH_NODE_TYPE_BLK, blk_name(blk)); Hm, I think blk_name() is empty for most backends. blk_root_get_parent_desc() then falls back to blk_get_attached_dev_id(). I think we should do the same here. Max > + if (blk_root(blk)) { > + graph_add_edge(gr, blk, blk_root(blk)); > + } > + } > + > + for (job = block_job_next(NULL); job; job = block_job_next(job)) { > + GSList *el; > + > + graph_add_node(gr, job, BLOCK_GRAPH_NODE_TYPE_JOB, job->job.id); > + for (el = job->nodes; el; el = el->next) { > + graph_add_edge(gr, job, (BdrvChild *)el->data); > + } > + } > + > + QTAILQ_FOREACH(bs, &graph_bdrv_states, node_list) { > + graph_add_node(gr, bs, BLOCK_GRAPH_NODE_TYPE_BDS, bs->node_name); > + QLIST_FOREACH(child, &bs->children, next) { > + graph_add_edge(gr, bs, child); > + } > + } > + > + return graph_finalize(gr); > +} > + > BlockDriverState *bdrv_lookup_bs(const char *device, > const char *node_name, > Error **errp)
On 9/28/18 11:31 AM, Max Reitz wrote: > On 23.08.18 17:46, Vladimir Sementsov-Ogievskiy wrote: >> Add a new command, returning block nodes (and their users) graph. >> Drive-by comment without actually reading the patch yet: >> +++ b/block.c >> @@ -4003,6 +4003,135 @@ BlockDeviceInfoList *bdrv_named_nodes_list(Error **errp) >> return list; >> } >> >> +#define QAPI_LIST_ADD(list, element) do { \ >> + typeof(list) _tmp = g_new(typeof(*(list)), 1); \ >> + _tmp->value = (element); \ >> + _tmp->next = (list); \ >> + list = _tmp; \ > > Interesting, how yoo put (list) as an rvalue in parentheses, but don't > do so when it's an lvalue. :-) > > I can follow your line of thinking, but it should probably always be in > parentheses. This macro seems rather handy. How much else in the code base would benefit if we promote this into something officially provided by qapi code (probably in qlist.h) rather than making multiple callsites repeat the boilerplate?
28.09.2018 19:31, Max Reitz wrote: > On 23.08.18 17:46, Vladimir Sementsov-Ogievskiy wrote: >> Add a new command, returning block nodes (and their users) graph. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> qapi/block-core.json | 91 +++++++++++++++++++++++ >> include/block/block.h | 1 + >> include/sysemu/block-backend.h | 2 + >> block.c | 129 +++++++++++++++++++++++++++++++++ >> block/block-backend.c | 5 ++ >> blockdev.c | 5 ++ >> 6 files changed, 233 insertions(+) > The design looks better (that is to say, good) to me, so I mostly have > technical remarks. (Only a bit of bike-shedding this time.) > >> diff --git a/qapi/block-core.json b/qapi/block-core.json >> index 4c7a37afdc..34cdc595d7 100644 >> --- a/qapi/block-core.json >> +++ b/qapi/block-core.json >> @@ -1629,6 +1629,97 @@ >> ## >> { 'command': 'query-named-block-nodes', 'returns': [ 'BlockDeviceInfo' ] } >> >> +## >> +# @BlockGraphNodeType: >> +# >> +# Since: 3.1 >> +## >> +{ 'enum': 'BlockGraphNodeType', >> + 'data': [ 'blk', 'job', 'bds' ] } > I wouldn't use abbreviations here, but the full names. At least they > should be described. Hmm do you have a suggestion? Do you mean something like block-backend block-job block-driver-state ? > > (Though with x-debug-, it doesn't matter too much.) > >> + >> +## >> +# @BlockGraphNode: >> +# > I think a description on at least what the name means for each type > would be useful; and that @id is generated just for this request and not > some significant value in the block layer. let me compose here, before sending next version: @id: Block graph node identifier. This @id is generated only for x-debug-query-block-graph and don't relate to any other identifiers in Qemu. @type: Type of graph node. Can be one of block-backend, block-job or block-driver-state. @name: Human readable name of the node. Corresponds to node-name for block-driver-state nodes, and not guaranteed to be unique in the whole graph (with block-jobs and block-backends) > >> +# Since: 3.1 >> +## >> +{ 'struct': 'BlockGraphNode', >> + 'data': { 'id': 'uint64', 'type': 'BlockGraphNodeType', 'name': 'str' } } >> + >> +## >> +# @BlockPermission: >> +# >> +# Enum of base block permissions. >> +# >> +# @consistent-read: A user that has the "permission" of consistent reads is >> +# guaranteed that their view of the contents of the block >> +# device is complete and self-consistent, representing the >> +# contents of a disk at a specific point. >> +# For most block devices (including their backing files) this >> +# is true, but the property cannot be maintained in a few >> +# situations like for intermediate nodes of a commit block >> +# job. >> +# >> +# @write: This permission is required to change the visible disk contents. >> +# >> +# @write-unchanged: This permission (which is weaker than BLK_PERM_WRITE) is >> +# both enough and required for writes to the block node when >> +# the caller promises that the visible disk content doesn't >> +# change. >> +# As the BLK_PERM_WRITE permission is strictly stronger, >> +# either is sufficient to perform an unchanging write. >> +# >> +# @resize: This permission is required to change the size of a block node. >> +# >> +# @graph-mod: This permission is required to change the node that this >> +# BdrvChild points to. >> +# >> +# Since: 3.1 >> +## >> + { 'enum': 'BlockPermission', >> + 'data': [ 'consistent-read', 'write', 'write-unchanged', 'resize', >> + 'graph-mod' ] } >> +## >> +# @BlockGraphEdge: >> +# >> +# Block Graph edge description for x-debug-query-block-graph. >> +# >> +# @parent: parent id >> +# >> +# @child: child id >> +# >> +# @name: name of the relation (examples are 'file' and 'backing') >> +# >> +# @perm: granted permissions for the parent operating on the child >> +# >> +# @shared-perm: permissions that can still be granted to other users of the >> +# child while it is still attached this parent > s/attached this/attached to this/ > >> +# >> +# Since: 3.1 >> +## >> +{ 'struct': 'BlockGraphEdge', >> + 'data': { 'parent': 'uint64', 'child': 'uint64', >> + 'name': 'str', 'perm': [ 'BlockPermission' ], >> + 'shared-perm': [ 'BlockPermission' ] } } >> + >> +## >> +# @BlockGraph: >> +# >> +# Block Graph - list of nodes and list of edges. >> +# >> +# Since: 3.1 >> +## >> +{ 'struct': 'BlockGraph', >> + 'data': { 'nodes': ['BlockGraphNode'], 'edges': ['BlockGraphEdge'] } } >> + >> +## >> +# @x-debug-query-block-graph: >> +# >> +# Get the block graph. >> +# >> +# Since: 3.1 >> +## >> +{ 'command': 'x-debug-query-block-graph', 'returns': 'BlockGraph' } >> + >> ## >> # @drive-mirror: >> # > [...] > >> diff --git a/block.c b/block.c >> index 6161dbe3eb..588f5a2648 100644 >> --- a/block.c >> +++ b/block.c >> @@ -4003,6 +4003,135 @@ BlockDeviceInfoList *bdrv_named_nodes_list(Error **errp) >> return list; >> } >> >> +#define QAPI_LIST_ADD(list, element) do { \ >> + typeof(list) _tmp = g_new(typeof(*(list)), 1); \ >> + _tmp->value = (element); \ >> + _tmp->next = (list); \ >> + list = _tmp; \ > Interesting, how yoo put (list) as an rvalue in parentheses, but don't > do so when it's an lvalue. :-) > > I can follow your line of thinking, but it should probably always be in > parentheses. agree. > >> +} while (0) >> + >> +typedef struct BlockGraphConstructor { >> + BlockGraph *graph; >> + GHashTable *hash; > Maybe... "graph_nodes"? "hash" seems awfully unspecific. ok > >> +} BlockGraphConstructor; >> + >> +static BlockGraphConstructor *graph_new(void) > I think this is a very broad name. Maybe prefix all of the function > names with debug_? (Or dbg_, or dbg_blk, or something) ok > >> +{ >> + BlockGraphConstructor *gr = g_new(BlockGraphConstructor, 1); >> + >> + gr->graph = g_new0(BlockGraph, 1); >> + gr->hash = g_hash_table_new(NULL, NULL); >> + >> + return gr; >> +} >> + >> +static BlockGraph *graph_finalize(BlockGraphConstructor *gr) >> +{ >> + g_hash_table_destroy(gr->hash); >> + >> + return gr->graph; > gr is leaked here. good catch, will fix. > >> +} >> + >> +static uint64_t graph_node_num(BlockGraphConstructor *gr, void *node) >> +{ >> + uint64_t ret = (uint64_t)g_hash_table_lookup(gr->hash, node); > I'd prefer a cast to uintptr_t. Otherwise the compiler may warn that > you cast a pointer to an integer of different size (with -m32). > > Storing it in a uint64_t (with an implicit cast then) is OK, though. > But maybe you do want to store it in a uintptr_t. The only reason not > to is because it's a uint64_t in the QAPI schema, but I think it'd be a > bit cleaner to work with uintptr_t internally and then emit it as > uint64_t (because that's definitely safe). It's just a bit more honest > because this way it's clear that with -m32, we cannot even represent IDs > larger than 32 bit (which doesn't matter). ok. then, why not use just "void *" ? > >> + >> + if (ret > 0) { > Just for style I'd prefer != over >. That makes it more clear that this > is a NULL check and not a check for errors (represented as negative > integers, even though @ret is unsigned). hm and it will be more clear with pointer type... > >> + return ret; >> + } >> + >> + ret = g_hash_table_size(gr->hash) + 1; > Maybe add a comment that you add 1 because 0 (NULL) is reserved for > non-entries? (Yes, it's clear, or I wouldn't have figured it out, but > I'd still find it nice.) hmm, I don't remember why is it reserved, looks like it doesn't matter.. But it may be more native to count graph nodes from 1, not from 0. > >> + g_hash_table_insert(gr->hash, node, (void *)ret); > This definitely needs a uintptr_t cast or you'll get a warning for 32 > bit pointers. > >> + >> + return ret; >> +} >> + >> +static void graph_add_node(BlockGraphConstructor *gr, void *node, >> + BlockGraphNodeType type, const char *name) >> +{ >> + BlockGraphNode *n; >> + >> + n = g_new0(BlockGraphNode, 1); >> + >> + n->id = graph_node_num(gr, node); >> + n->type = type; >> + n->name = g_strdup(name); >> + >> + QAPI_LIST_ADD(gr->graph->nodes, n); >> +} >> + >> +static void graph_add_edge(BlockGraphConstructor *gr, void *parent, >> + const BdrvChild *child) >> +{ >> + typedef struct { >> + unsigned int flag; >> + BlockPermission num; >> + } PermissionMap; >> + >> + static PermissionMap permissions[] = { > You can add a const if you like. yes > >> + { BLK_PERM_CONSISTENT_READ, BLOCK_PERMISSION_CONSISTENT_READ }, >> + { BLK_PERM_WRITE, BLOCK_PERMISSION_WRITE }, >> + { BLK_PERM_WRITE_UNCHANGED, BLOCK_PERMISSION_WRITE_UNCHANGED }, >> + { BLK_PERM_RESIZE, BLOCK_PERMISSION_RESIZE }, >> + { BLK_PERM_GRAPH_MOD, BLOCK_PERMISSION_GRAPH_MOD }, >> + { 0, 0 } >> + }; > You could add a static assertion that "1 << (sizeof(permissions) / > sizeof(permissions[0]) - 1) == BLK_PERM_ALL + 1" to ensure that whenever > we add a permission, we don't forget to update this list. But then > again, I don't think we're going to add a permission any time soon... ok. hmm, we have cheating bdrv_mirror_top_child_perm(), which definitely shows that permission scheme is not complete.. :) > >> + PermissionMap *p; >> + BlockGraphEdge *edge; >> + >> + edge = g_new0(BlockGraphEdge, 1); >> + >> + edge->parent = graph_node_num(gr, parent); >> + edge->child = graph_node_num(gr, child->bs); >> + edge->name = g_strdup(child->name); >> + >> + for (p = permissions; p->flag; p++) { >> + if (p->flag & child->perm) { >> + QAPI_LIST_ADD(edge->perm, p->num); >> + } >> + if (p->flag & child->shared_perm) { >> + QAPI_LIST_ADD(edge->shared_perm, p->num); >> + } >> + } >> + >> + QAPI_LIST_ADD(gr->graph->edges, edge); >> +} >> + >> + >> +BlockGraph *bdrv_get_block_graph(Error **errp) >> +{ >> + BlockBackend *blk; >> + BlockJob *job; >> + BlockDriverState *bs; >> + BdrvChild *child; >> + BlockGraphConstructor *gr = graph_new(); >> + >> + for (blk = blk_all_next(NULL); blk; blk = blk_all_next(blk)) { >> + graph_add_node(gr, blk, BLOCK_GRAPH_NODE_TYPE_BLK, blk_name(blk)); > Hm, I think blk_name() is empty for most backends. > blk_root_get_parent_desc() then falls back to blk_get_attached_dev_id(). > I think we should do the same here. ok, good thing. However backup target blk will remain unnamed anyway. > > Max > >> + if (blk_root(blk)) { >> + graph_add_edge(gr, blk, blk_root(blk)); >> + } >> + } >> + >> + for (job = block_job_next(NULL); job; job = block_job_next(job)) { >> + GSList *el; >> + >> + graph_add_node(gr, job, BLOCK_GRAPH_NODE_TYPE_JOB, job->job.id); >> + for (el = job->nodes; el; el = el->next) { >> + graph_add_edge(gr, job, (BdrvChild *)el->data); >> + } >> + } >> + >> + QTAILQ_FOREACH(bs, &graph_bdrv_states, node_list) { >> + graph_add_node(gr, bs, BLOCK_GRAPH_NODE_TYPE_BDS, bs->node_name); >> + QLIST_FOREACH(child, &bs->children, next) { >> + graph_add_edge(gr, bs, child); >> + } >> + } >> + >> + return graph_finalize(gr); >> +} >> + >> BlockDriverState *bdrv_lookup_bs(const char *device, >> const char *node_name, >> Error **errp)
On 02.10.18 15:01, Vladimir Sementsov-Ogievskiy wrote: > 28.09.2018 19:31, Max Reitz wrote: >> On 23.08.18 17:46, Vladimir Sementsov-Ogievskiy wrote: >>> Add a new command, returning block nodes (and their users) graph. >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>> --- >>> qapi/block-core.json | 91 +++++++++++++++++++++++ >>> include/block/block.h | 1 + >>> include/sysemu/block-backend.h | 2 + >>> block.c | 129 +++++++++++++++++++++++++++++++++ >>> block/block-backend.c | 5 ++ >>> blockdev.c | 5 ++ >>> 6 files changed, 233 insertions(+) >> The design looks better (that is to say, good) to me, so I mostly have >> technical remarks. (Only a bit of bike-shedding this time.) >> >>> diff --git a/qapi/block-core.json b/qapi/block-core.json >>> index 4c7a37afdc..34cdc595d7 100644 >>> --- a/qapi/block-core.json >>> +++ b/qapi/block-core.json >>> @@ -1629,6 +1629,97 @@ >>> ## >>> { 'command': 'query-named-block-nodes', 'returns': [ >>> 'BlockDeviceInfo' ] } >>> +## >>> +# @BlockGraphNodeType: >>> +# >>> +# Since: 3.1 >>> +## >>> +{ 'enum': 'BlockGraphNodeType', >>> + 'data': [ 'blk', 'job', 'bds' ] } >> I wouldn't use abbreviations here, but the full names. At least they >> should be described. > > Hmm do you have a suggestion? Do you mean something like > block-backend > block-job > block-driver-state > ? That's what I meant, yes; though I'm not sure I was right. "block-backend" sounds good; "job" is up to you since it isn't necessarily an abbreviation. About BDSs... I guess I'll leave it up to you, too. block-driver-state doesn't tell people more than just "bds", so it isn't much more useful. Usually I'd call it "node" because that's what we like to call it in docs. Unfortunately, that won't quite work here, because in this case, all of these things are nodes. "block-node" would be the best I can come up with. But I'm not sure whether it's any better than "bds", so I'll just leave it up to you. Documentation would still be nice, but I don't even know how you'd describe @bds. "BlockDriverState"? Technically it'd be "Node of the block graph", but, er, well. That's a bad description for an element of the "BlockGraphNodeType" enum. So what I'm saying is "block-backend" would be nice, the rest is up to you. >> (Though with x-debug-, it doesn't matter too much.) >> >>> + >>> +## >>> +# @BlockGraphNode: >>> +# >> I think a description on at least what the name means for each type >> would be useful; and that @id is generated just for this request and not >> some significant value in the block layer. > > let me compose here, before sending next version: > > @id: Block graph node identifier. This @id is generated only for > x-debug-query-block-graph and don't relate to any other identifiers in s/don't/does not/ > Qemu. > @type: Type of graph node. Can be one of block-backend, block-job or > block-driver-state. > @name: Human readable name of the node. Corresponds to node-name for > block-driver-state nodes, and not guaranteed to be unique in the whole I'd prefer s/, and/; is/ (or s/, and/. Is/) > graph (with block-jobs and block-backends) Sounds good to me overall. [...] >>> +} >>> + >>> +static uint64_t graph_node_num(BlockGraphConstructor *gr, void *node) >>> +{ >>> + uint64_t ret = (uint64_t)g_hash_table_lookup(gr->hash, node); >> I'd prefer a cast to uintptr_t. Otherwise the compiler may warn that >> you cast a pointer to an integer of different size (with -m32). >> >> Storing it in a uint64_t (with an implicit cast then) is OK, though. >> But maybe you do want to store it in a uintptr_t. The only reason not >> to is because it's a uint64_t in the QAPI schema, but I think it'd be a >> bit cleaner to work with uintptr_t internally and then emit it as >> uint64_t (because that's definitely safe). It's just a bit more honest >> because this way it's clear that with -m32, we cannot even represent IDs >> larger than 32 bit (which doesn't matter). > > ok. then, why not use just "void *" ? Because you want to store integer IDs and not pointers. >>> + >>> + if (ret > 0) { >> Just for style I'd prefer != over >. That makes it more clear that this >> is a NULL check and not a check for errors (represented as negative >> integers, even though @ret is unsigned). > > hm and it will be more clear with pointer type... > >> >>> + return ret; >>> + } >>> + >>> + ret = g_hash_table_size(gr->hash) + 1; >> Maybe add a comment that you add 1 because 0 (NULL) is reserved for >> non-entries? (Yes, it's clear, or I wouldn't have figured it out, but >> I'd still find it nice.) > > hmm, I don't remember why is it reserved, looks like it doesn't matter.. > But it may be more native to count graph nodes from 1, not from 0. Because you only have a single function for adding entries to and querying entries from gr->hash. So it needs to distinguish whether a node already has an ID or not. If it does not, g_hash_table_lookup() will return NULL, so you cannot add entries with index 0, because they will look like the entry does not exist yet. (So the first call to graph_node_num() will return 0, and the second call for the same node will overwrite that value with something else. Presumably you'll then have a BlockBackend with ID 0 but not edge using the ID 0.) You can use ID 0 if instead of checking whether "ret != 0" you'd use g_hash_table_contains() or g_hash_table_lookup_extended() instead. Max
05.10.2018 22:34, Max Reitz wrote: > On 02.10.18 15:01, Vladimir Sementsov-Ogievskiy wrote: >> 28.09.2018 19:31, Max Reitz wrote: >>> On 23.08.18 17:46, Vladimir Sementsov-Ogievskiy wrote: >>>> Add a new command, returning block nodes (and their users) graph. >>>> >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>>> --- >>>> qapi/block-core.json | 91 +++++++++++++++++++++++ >>>> include/block/block.h | 1 + >>>> include/sysemu/block-backend.h | 2 + >>>> block.c | 129 +++++++++++++++++++++++++++++++++ >>>> block/block-backend.c | 5 ++ >>>> blockdev.c | 5 ++ >>>> 6 files changed, 233 insertions(+) >>> The design looks better (that is to say, good) to me, so I mostly have >>> technical remarks. (Only a bit of bike-shedding this time.) >>> >>>> diff --git a/qapi/block-core.json b/qapi/block-core.json >>>> index 4c7a37afdc..34cdc595d7 100644 >>>> --- a/qapi/block-core.json >>>> +++ b/qapi/block-core.json >>>> @@ -1629,6 +1629,97 @@ >>>> ## >>>> { 'command': 'query-named-block-nodes', 'returns': [ >>>> 'BlockDeviceInfo' ] } >>>> +## >>>> +# @BlockGraphNodeType: >>>> +# >>>> +# Since: 3.1 >>>> +## >>>> +{ 'enum': 'BlockGraphNodeType', >>>> + 'data': [ 'blk', 'job', 'bds' ] } >>> I wouldn't use abbreviations here, but the full names. At least they >>> should be described. >> Hmm do you have a suggestion? Do you mean something like >> block-backend >> block-job >> block-driver-state >> ? > That's what I meant, yes; though I'm not sure I was right. > > "block-backend" sounds good; "job" is up to you since it isn't > necessarily an abbreviation. > > About BDSs... I guess I'll leave it up to you, too. block-driver-state > doesn't tell people more than just "bds", so it isn't much more useful. > Usually I'd call it "node" because that's what we like to call it in > docs. Unfortunately, that won't quite work here, because in this case, > all of these things are nodes. > > "block-node" would be the best I can come up with. But I'm not sure > whether it's any better than "bds", so I'll just leave it up to you. > > Documentation would still be nice, but I don't even know how you'd > describe @bds. "BlockDriverState"? Technically it'd be "Node of the > block graph", but, er, well. That's a bad description for an element of > the "BlockGraphNodeType" enum. > > So what I'm saying is "block-backend" would be nice, the rest is up to you. > >>> (Though with x-debug-, it doesn't matter too much.) >>> >>>> + >>>> +## >>>> +# @BlockGraphNode: >>>> +# >>> I think a description on at least what the name means for each type >>> would be useful; and that @id is generated just for this request and not >>> some significant value in the block layer. >> let me compose here, before sending next version: >> >> @id: Block graph node identifier. This @id is generated only for >> x-debug-query-block-graph and don't relate to any other identifiers in > s/don't/does not/ > >> Qemu. >> @type: Type of graph node. Can be one of block-backend, block-job or >> block-driver-state. >> @name: Human readable name of the node. Corresponds to node-name for >> block-driver-state nodes, and not guaranteed to be unique in the whole > I'd prefer s/, and/; is/ (or s/, and/. Is/) > >> graph (with block-jobs and block-backends) > Sounds good to me overall. > > [...] > >>>> +} >>>> + >>>> +static uint64_t graph_node_num(BlockGraphConstructor *gr, void *node) >>>> +{ >>>> + uint64_t ret = (uint64_t)g_hash_table_lookup(gr->hash, node); >>> I'd prefer a cast to uintptr_t. Otherwise the compiler may warn that >>> you cast a pointer to an integer of different size (with -m32). >>> >>> Storing it in a uint64_t (with an implicit cast then) is OK, though. >>> But maybe you do want to store it in a uintptr_t. The only reason not >>> to is because it's a uint64_t in the QAPI schema, but I think it'd be a >>> bit cleaner to work with uintptr_t internally and then emit it as >>> uint64_t (because that's definitely safe). It's just a bit more honest >>> because this way it's clear that with -m32, we cannot even represent IDs >>> larger than 32 bit (which doesn't matter). >> ok. then, why not use just "void *" ? > Because you want to store integer IDs and not pointers. Ah, didn't know what is uintptr_t, ashamed) > >>>> + >>>> + if (ret > 0) { >>> Just for style I'd prefer != over >. That makes it more clear that this >>> is a NULL check and not a check for errors (represented as negative >>> integers, even though @ret is unsigned). >> hm and it will be more clear with pointer type... >> >>>> + return ret; >>>> + } >>>> + >>>> + ret = g_hash_table_size(gr->hash) + 1; >>> Maybe add a comment that you add 1 because 0 (NULL) is reserved for >>> non-entries? (Yes, it's clear, or I wouldn't have figured it out, but >>> I'd still find it nice.) >> hmm, I don't remember why is it reserved, looks like it doesn't matter.. >> But it may be more native to count graph nodes from 1, not from 0. > Because you only have a single function for adding entries to and > querying entries from gr->hash. So it needs to distinguish whether a > node already has an ID or not. If it does not, g_hash_table_lookup() > will return NULL, so you cannot add entries with index 0, because they > will look like the entry does not exist yet. (So the first call to > graph_node_num() will return 0, and the second call for the same node > will overwrite that value with something else. Presumably you'll then > have a BlockBackend with ID 0 but not edge using the ID 0.) > > You can use ID 0 if instead of checking whether "ret != 0" you'd use > g_hash_table_contains() or g_hash_table_lookup_extended() instead. Hm, looks correct. It's a bit strange to thank for explanation of my own code, nevertheless thank you! > > Max >
diff --git a/qapi/block-core.json b/qapi/block-core.json index 4c7a37afdc..34cdc595d7 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1629,6 +1629,97 @@ ## { 'command': 'query-named-block-nodes', 'returns': [ 'BlockDeviceInfo' ] } +## +# @BlockGraphNodeType: +# +# Since: 3.1 +## +{ 'enum': 'BlockGraphNodeType', + 'data': [ 'blk', 'job', 'bds' ] } + +## +# @BlockGraphNode: +# +# Since: 3.1 +## +{ 'struct': 'BlockGraphNode', + 'data': { 'id': 'uint64', 'type': 'BlockGraphNodeType', 'name': 'str' } } + +## +# @BlockPermission: +# +# Enum of base block permissions. +# +# @consistent-read: A user that has the "permission" of consistent reads is +# guaranteed that their view of the contents of the block +# device is complete and self-consistent, representing the +# contents of a disk at a specific point. +# For most block devices (including their backing files) this +# is true, but the property cannot be maintained in a few +# situations like for intermediate nodes of a commit block +# job. +# +# @write: This permission is required to change the visible disk contents. +# +# @write-unchanged: This permission (which is weaker than BLK_PERM_WRITE) is +# both enough and required for writes to the block node when +# the caller promises that the visible disk content doesn't +# change. +# As the BLK_PERM_WRITE permission is strictly stronger, +# either is sufficient to perform an unchanging write. +# +# @resize: This permission is required to change the size of a block node. +# +# @graph-mod: This permission is required to change the node that this +# BdrvChild points to. +# +# Since: 3.1 +## + { 'enum': 'BlockPermission', + 'data': [ 'consistent-read', 'write', 'write-unchanged', 'resize', + 'graph-mod' ] } +## +# @BlockGraphEdge: +# +# Block Graph edge description for x-debug-query-block-graph. +# +# @parent: parent id +# +# @child: child id +# +# @name: name of the relation (examples are 'file' and 'backing') +# +# @perm: granted permissions for the parent operating on the child +# +# @shared-perm: permissions that can still be granted to other users of the +# child while it is still attached this parent +# +# Since: 3.1 +## +{ 'struct': 'BlockGraphEdge', + 'data': { 'parent': 'uint64', 'child': 'uint64', + 'name': 'str', 'perm': [ 'BlockPermission' ], + 'shared-perm': [ 'BlockPermission' ] } } + +## +# @BlockGraph: +# +# Block Graph - list of nodes and list of edges. +# +# Since: 3.1 +## +{ 'struct': 'BlockGraph', + 'data': { 'nodes': ['BlockGraphNode'], 'edges': ['BlockGraphEdge'] } } + +## +# @x-debug-query-block-graph: +# +# Get the block graph. +# +# Since: 3.1 +## +{ 'command': 'x-debug-query-block-graph', 'returns': 'BlockGraph' } + ## # @drive-mirror: # diff --git a/include/block/block.h b/include/block/block.h index 4e0871aaf9..6f2ccad040 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -448,6 +448,7 @@ void bdrv_eject(BlockDriverState *bs, bool eject_flag); const char *bdrv_get_format_name(BlockDriverState *bs); BlockDriverState *bdrv_find_node(const char *node_name); BlockDeviceInfoList *bdrv_named_nodes_list(Error **errp); +BlockGraph *bdrv_get_block_graph(Error **errp); BlockDriverState *bdrv_lookup_bs(const char *device, const char *node_name, Error **errp); diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h index 830d873f24..32ca5c1b12 100644 --- a/include/sysemu/block-backend.h +++ b/include/sysemu/block-backend.h @@ -237,4 +237,6 @@ int coroutine_fn blk_co_copy_range(BlockBackend *blk_in, int64_t off_in, int bytes, BdrvRequestFlags read_flags, BdrvRequestFlags write_flags); +const BdrvChild *blk_root(BlockBackend *blk); + #endif diff --git a/block.c b/block.c index 6161dbe3eb..588f5a2648 100644 --- a/block.c +++ b/block.c @@ -4003,6 +4003,135 @@ BlockDeviceInfoList *bdrv_named_nodes_list(Error **errp) return list; } +#define QAPI_LIST_ADD(list, element) do { \ + typeof(list) _tmp = g_new(typeof(*(list)), 1); \ + _tmp->value = (element); \ + _tmp->next = (list); \ + list = _tmp; \ +} while (0) + +typedef struct BlockGraphConstructor { + BlockGraph *graph; + GHashTable *hash; +} BlockGraphConstructor; + +static BlockGraphConstructor *graph_new(void) +{ + BlockGraphConstructor *gr = g_new(BlockGraphConstructor, 1); + + gr->graph = g_new0(BlockGraph, 1); + gr->hash = g_hash_table_new(NULL, NULL); + + return gr; +} + +static BlockGraph *graph_finalize(BlockGraphConstructor *gr) +{ + g_hash_table_destroy(gr->hash); + + return gr->graph; +} + +static uint64_t graph_node_num(BlockGraphConstructor *gr, void *node) +{ + uint64_t ret = (uint64_t)g_hash_table_lookup(gr->hash, node); + + if (ret > 0) { + return ret; + } + + ret = g_hash_table_size(gr->hash) + 1; + g_hash_table_insert(gr->hash, node, (void *)ret); + + return ret; +} + +static void graph_add_node(BlockGraphConstructor *gr, void *node, + BlockGraphNodeType type, const char *name) +{ + BlockGraphNode *n; + + n = g_new0(BlockGraphNode, 1); + + n->id = graph_node_num(gr, node); + n->type = type; + n->name = g_strdup(name); + + QAPI_LIST_ADD(gr->graph->nodes, n); +} + +static void graph_add_edge(BlockGraphConstructor *gr, void *parent, + const BdrvChild *child) +{ + typedef struct { + unsigned int flag; + BlockPermission num; + } PermissionMap; + + static PermissionMap permissions[] = { + { BLK_PERM_CONSISTENT_READ, BLOCK_PERMISSION_CONSISTENT_READ }, + { BLK_PERM_WRITE, BLOCK_PERMISSION_WRITE }, + { BLK_PERM_WRITE_UNCHANGED, BLOCK_PERMISSION_WRITE_UNCHANGED }, + { BLK_PERM_RESIZE, BLOCK_PERMISSION_RESIZE }, + { BLK_PERM_GRAPH_MOD, BLOCK_PERMISSION_GRAPH_MOD }, + { 0, 0 } + }; + PermissionMap *p; + BlockGraphEdge *edge; + + edge = g_new0(BlockGraphEdge, 1); + + edge->parent = graph_node_num(gr, parent); + edge->child = graph_node_num(gr, child->bs); + edge->name = g_strdup(child->name); + + for (p = permissions; p->flag; p++) { + if (p->flag & child->perm) { + QAPI_LIST_ADD(edge->perm, p->num); + } + if (p->flag & child->shared_perm) { + QAPI_LIST_ADD(edge->shared_perm, p->num); + } + } + + QAPI_LIST_ADD(gr->graph->edges, edge); +} + + +BlockGraph *bdrv_get_block_graph(Error **errp) +{ + BlockBackend *blk; + BlockJob *job; + BlockDriverState *bs; + BdrvChild *child; + BlockGraphConstructor *gr = graph_new(); + + for (blk = blk_all_next(NULL); blk; blk = blk_all_next(blk)) { + graph_add_node(gr, blk, BLOCK_GRAPH_NODE_TYPE_BLK, blk_name(blk)); + if (blk_root(blk)) { + graph_add_edge(gr, blk, blk_root(blk)); + } + } + + for (job = block_job_next(NULL); job; job = block_job_next(job)) { + GSList *el; + + graph_add_node(gr, job, BLOCK_GRAPH_NODE_TYPE_JOB, job->job.id); + for (el = job->nodes; el; el = el->next) { + graph_add_edge(gr, job, (BdrvChild *)el->data); + } + } + + QTAILQ_FOREACH(bs, &graph_bdrv_states, node_list) { + graph_add_node(gr, bs, BLOCK_GRAPH_NODE_TYPE_BDS, bs->node_name); + QLIST_FOREACH(child, &bs->children, next) { + graph_add_edge(gr, bs, child); + } + } + + return graph_finalize(gr); +} + BlockDriverState *bdrv_lookup_bs(const char *device, const char *node_name, Error **errp) diff --git a/block/block-backend.c b/block/block-backend.c index fa120630be..f3704fe87d 100644 --- a/block/block-backend.c +++ b/block/block-backend.c @@ -2234,3 +2234,8 @@ int coroutine_fn blk_co_copy_range(BlockBackend *blk_in, int64_t off_in, blk_out->root, off_out, bytes, read_flags, write_flags); } + +const BdrvChild *blk_root(BlockBackend *blk) +{ + return blk->root; +} diff --git a/blockdev.c b/blockdev.c index 72f5347df5..3b98ce7571 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3485,6 +3485,11 @@ BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp) return bdrv_named_nodes_list(errp); } +BlockGraph *qmp_x_debug_query_block_graph(Error **errp) +{ + return bdrv_get_block_graph(errp); +} + BlockJob *do_blockdev_backup(BlockdevBackup *backup, JobTxn *txn, Error **errp) {
Add a new command, returning block nodes (and their users) graph. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- qapi/block-core.json | 91 +++++++++++++++++++++++ include/block/block.h | 1 + include/sysemu/block-backend.h | 2 + block.c | 129 +++++++++++++++++++++++++++++++++ block/block-backend.c | 5 ++ blockdev.c | 5 ++ 6 files changed, 233 insertions(+)