Re: [Qemu-devel] [PATCH for-2.10 v3] block: Skip implicit nodes in query-block/blockstats

[Max Reitz]

On 2017-07-20 15:11, 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 implicit 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>
> ---
> 
> v3:
> - Fixed skipping during the recursion for query-block [Peter]
> - Fixed backing_file_depth
> - More thorough test case for mirror, added one for commit
> 
> v2:
> - Skip implicit nodes not only on the top level, but also during the recursive
>   calls [Peter]
> - Spelling fix in the commit message [Manos]
> 
>  block.c                    | 13 -------------
>  block/commit.c             |  3 +++
>  block/mirror.c             |  3 +++
>  block/qapi.c               | 33 +++++++++++++++++++++++++++------
>  include/block/block.h      |  1 -
>  include/block/block_int.h  |  1 +
>  qapi/block-core.json       |  6 ++++--
>  tests/qemu-iotests/040     | 30 +++++++++++++++++++++++++++++-
>  tests/qemu-iotests/040.out |  4 ++--
>  tests/qemu-iotests/041     | 38 +++++++++++++++++++++++++++++++++++++-
>  tests/qemu-iotests/041.out |  4 ++--
>  11 files changed, 108 insertions(+), 28 deletions(-)

[...]

> diff --git a/block/qapi.c b/block/qapi.c
> index 95b2e2d..d2b18ee 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c

[...]

> @@ -133,13 +133,21 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend 
> *blk,
>              qapi_free_BlockDeviceInfo(info);
>              return NULL;
>          }
> +
>          if (bs0->drv && bs0->backing) {
> +            info->backing_file_depth++;
>              bs0 = bs0->backing->bs;
>              (*p_image_info)->has_backing_image = true;
>              p_image_info = &((*p_image_info)->backing_image);
>          } else {
>              break;
>          }
> +
> +        /* Skip automatically inserted nodes that the user isn't aware of for
> +         * query-block (blk != NULL), but not for query-named-block-nodes */
> +        while (blk && bs0 && bs0->drv && bs0->implicit) {
> +            bs0 = backing_bs(bs0);
> +        }

If the bottom-most backing file is implicit, this will leave bs0 to be
NULL. The surrounding loop does not look like it could cope well with
that. (And even if it could, we would have to reset has_backing_image to
false, wouldn't we?)

Not necessarily an issue here because all of our implicit nodes are
somewhere in between, but still...

(And I just saw you're even asserting this in another place...)

>      }
>  
>      return info;

[...]

> @@ -446,6 +459,14 @@ static BlockStats *bdrv_query_bds_stats(const 
> BlockDriverState *bs,
>          return s;
>      }
>  
> +    /* Skip automatically inserted nodes that the user isn't aware of in
> +     * a BlockBackend-level command. Stay at the exact node for a node-level
> +     * command. */
> +    while (blk_level && bs->drv && bs->implicit) {
> +        bs = backing_bs(bs);
> +        assert(bs);

(...namely here.)

I extremely doubt this assertion will hold in the future, but well, it's
good enough for now.

So, I've always wanted "file" to be the default child for block filters.
This patch very much establishes that "backing" is the default child, right?

So, all in all, I'd rather like to still discuss some things, but (1)
the patch will do what it's supposed to do as-is, and (2) all of what
I'd like to discuss would be about 2.11 anyway. So:

Reviewed-by: Max Reitz <address@hidden>

signature.asc
Description: OpenPGP digital signature