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: [email protected] > > Signed-off-by: Kevin Wolf <[email protected]> > > --- > > 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
pgpyWSMB_Kgsx.pgp
Description: PGP signature
