Am 19.07.2017 um 13:15 hat Peter Krempa geschrieben: > On Wed, Jul 19, 2017 at 10:44:47 +0200, Kevin Wolf wrote: > > Commits 0db832f and 6cdbceb introduced the automatic insertion of filter > > nodes above the top layer of mirror and commit block jobs. The > > assumption made there was that since libvirt doesn't do node-level > > management of the block layer yet, it shouldn't be affected by added > > nodes. > > > > This is true as far as commands issued by libvirt are concerned. It only > > uses BlockBackend names to address nodes, so any operations it performs > > still operate on the root of the tree as intended. > > > > However, the assumption breaks down when you consider query commands, > > which return data for the wrong node now. These commands also return > > information on some child nodes (bs->file and/or bs->backing), which > > libvirt does make use of, and which refer to the wrong nodes, too. > > > > One of the consequences is that oVirt gets wrong information about the > > image size and stops the VM in response as long as a mirror or commit > > job is running: > > > > https://bugzilla.redhat.com/show_bug.cgi?id=1470634 > > > > This patch fixes the problem by hiding the implict nodes created > > automatically by the mirror and commit block jobs in the output of > > query-block and BlockBackend-based query-blockstats as long as the user > > doesn't indicate that they are aware of those nodes by providing a node > > name for them in the QMP command to start the block job. > > > > The node-based commands query-named-block-nodes and query-blockstats > > with query-nodes=true still show all nodes, including implicit ones. > > This ensures that users that are capable of node-level management can > > still access the full information; users that only know BlockBackends > > won't use these commands. > > > > Cc: qemu-sta...@nongnu.org > > Signed-off-by: Kevin Wolf <kw...@redhat.com> > > --- > > block/commit.c | 3 +++ > > block/mirror.c | 3 +++ > > block/qapi.c | 13 ++++++++++++- > > include/block/block_int.h | 1 + > > qapi/block-core.json | 6 ++++-- > > tests/qemu-iotests/041 | 23 +++++++++++++++++++++++ > > tests/qemu-iotests/041.out | 4 ++-- > > 7 files changed, 48 insertions(+), 5 deletions(-) > > Fails to apply to master, thus I didn't do a functional check, but from > reading the code ...
My last pull request was just merged into master, so now it should apply cleanly. > > diff --git a/block/qapi.c b/block/qapi.c > > index 95b2e2d..0ed23b8 100644 > > --- a/block/qapi.c > > +++ b/block/qapi.c > > @@ -324,6 +324,11 @@ static void bdrv_query_info(BlockBackend *blk, > > BlockInfo **p_info, > > BlockDriverState *bs = blk_bs(blk); > > char *qdev; > > > > + /* Skip automatically inserted nodes that the user isn't aware of */ > > + while (bs && bs->drv && bs->implicit) { > > + bs = backing_bs(bs); > > + } > > So bdrv_query_info then calls bdrv_block_device_info, which also > populates the backing chain for that bs. That function does not do this > trick of skipping the implicit nodes. > > From what I've understood from the 'commit' job, it's possible to have > the implicit node inserted even at the @top bs of the commit job, so it > looks to me as if bdrv_block_device_info will need such a tweak too. Ah, yes, I missed the recursive calls. I will add another loop as you suggest. > > + > > info->device = g_strdup(blk_name(blk)); > > info->type = g_strdup("unknown"); > > info->locked = blk_dev_is_medium_locked(blk); > > @@ -518,12 +523,18 @@ BlockStatsList *qmp_query_blockstats(bool > > has_query_nodes, > > } > > } else { > > for (blk = blk_next(NULL); blk; blk = blk_next(blk)) { > > + BlockDriverState *bs = blk_bs(blk); > > BlockStatsList *info = g_malloc0(sizeof(*info)); > > AioContext *ctx = blk_get_aio_context(blk); > > BlockStats *s; > > > > + /* Skip automatically inserted nodes that the user isn't aware > > of */ > > + while (bs && bs->drv && bs->implicit) { > > + bs = backing_bs(bs); > > + } > > + > > aio_context_acquire(ctx); > > - s = bdrv_query_bds_stats(blk_bs(blk), true); > > + s = bdrv_query_bds_stats(bs, true); > > Same here. The recursive call in bdrv_query_bds_stats does not skip the > implicitly added layers IIUC. Indeed. > > s->has_device = true; > > s->device = g_strdup(blk_name(blk)); > > bdrv_query_blk_stats(s->stats, blk); > > Feel free to add a 'Reviewed-by' if I did not understand what's > happening in the commit job properly, or if you add the skipping into > the gathering functions as well. I'll send a v2 for this, it doesn't seem trivial enough to just add Reviewed-by. Kevin
pgpjwEhTmxVFy.pgp
Description: PGP signature