[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH for-2.10] block: Skip implicit nodes in query-bl
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH for-2.10] block: Skip implicit nodes in query-block/blockstats |
Date: |
Wed, 19 Jul 2017 14:09:21 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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: address@hidden
> > Signed-off-by: Kevin Wolf <address@hidden>
> > ---
> > 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
pgpdG1UfBKjzM.pgp
Description: PGP signature