On 18.06.19 14:06, Vladimir Sementsov-Ogievskiy wrote:
> 13.06.2019 1:09, Max Reitz wrote:
>> query-block and query-named-block-nodes now return any filtered child
>> under "backing", not just bs->backing or COW children. This is so that
>> filters do not interrupt the reported backing chain. This changes the
>> output for iotest 184, as the throttled node now appears as a backing
>> child.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>> block/qapi.c | 35 ++++++++++++++++++++---------------
>> tests/qemu-iotests/184.out | 7 ++++++-
>> 2 files changed, 26 insertions(+), 16 deletions(-)
>>
>> diff --git a/block/qapi.c b/block/qapi.c
>> index 0c13c86f4e..1fd2937abc 100644
>> --- a/block/qapi.c
>> +++ b/block/qapi.c
>> @@ -150,9 +150,13 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
>> return NULL;
>> }
>>
>> - if (bs0->drv && bs0->backing) {
>> + if (bs0->drv && bdrv_filtered_child(bs0)) {
>> + /*
>> + * Put any filtered child here (for backwards compatibility to when
>> + * we put bs0->backing here, which might be any filtered child).
>> + */
>> info->backing_file_depth++;
>> - bs0 = bs0->backing->bs;
>> + bs0 = bdrv_filtered_bs(bs0);
>
>
> so, here we report all filtered filter children as backing ...
>
>> (*p_image_info)->has_backing_image = true;
>> p_image_info = &((*p_image_info)->backing_image);
>> } else {
>> @@ -161,9 +165,8 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
>>
>> /* 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->drv && bs0->implicit) {
>> - bs0 = backing_bs(bs0);
>> - assert(bs0);
>> + if (blk) {
>> + bs0 = bdrv_skip_implicit_filters(bs0);
>> }
>> }
>>
>> @@ -348,9 +351,9 @@ 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);
>> + if (bs) {
>> + /* Skip automatically inserted nodes that the user isn't aware of */
>> + bs = bdrv_skip_implicit_filters(bs);
>> }
>>
>> info->device = g_strdup(blk_name(blk));
>> @@ -507,6 +510,7 @@ static void bdrv_query_blk_stats(BlockDeviceStats *ds, BlockBackend *blk)
>> static BlockStats *bdrv_query_bds_stats(BlockDriverState *bs,
>> bool blk_level)
>> {
>> + BlockDriverState *storage_bs, *cow_bs;
>> BlockStats *s = NULL;
>>
>> s = g_malloc0(sizeof(*s));
>> @@ -519,9 +523,8 @@ static BlockStats *bdrv_query_bds_stats(BlockDriverState *bs,
>> /* 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);
>> + if (blk_level) {
>> + bs = bdrv_skip_implicit_filters(bs);
>> }
>>
>> if (bdrv_get_node_name(bs)[0]) {
>> @@ -531,14 +534,16 @@ static BlockStats *bdrv_query_bds_stats(BlockDriverState *bs,
>>
>> s->stats->wr_highest_offset = stat64_get(&bs->wr_highest_offset);
>>
>> - if (bs->file) {
>> + storage_bs = bdrv_storage_bs(bs);
>> + if (storage_bs) {
>> s->has_parent = true;
>> - s->parent = bdrv_query_bds_stats(bs->file->bs, blk_level);
>> + s->parent = bdrv_query_bds_stats(storage_bs, blk_level);
>
> ... and here not as "backing" but as "parent"
>
> Shouldn't we report filter-child as backing here too, for consistency?
My idea here was that it compatibility is not that important, it’s just
stats, after all. Well, wr_highest_offset is kind of important, but
that doesn’t matter for a backing file.
So I decided to implement here what actually makes sense. But maybe I
was actually wrong about “what makes sense”. It doesn’t make much sense
to interrupt the backing chain here because of a filter... Maybe the
s->backing field should always be the next non-implicit element in the
backing chain? (So that may be duplicated in s->parent and s->backing,
but actually, so what.)
Max
> anyway:
>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>
>> }
>>
>> - if (blk_level && bs->backing) {
>> + cow_bs = bdrv_filtered_cow_bs(bs);
>> + if (blk_level && cow_bs) {
>> s->has_backing = true;
>> - s->backing = bdrv_query_bds_stats(bs->backing->bs, blk_level);
>> + s->backing = bdrv_query_bds_stats(cow_bs, blk_level);
>> }
>>
>> return s;
>> diff --git a/tests/qemu-iotests/184.out b/tests/qemu-iotests/184.out
>> index 3deb3cfb94..1d61f7e224 100644
>> --- a/tests/qemu-iotests/184.out
>> +++ b/tests/qemu-iotests/184.out
>> @@ -27,6 +27,11 @@ Testing:
>> "iops_rd": 0,
>> "detect_zeroes": "off",
>> "image": {
>> + "backing-image": {
>> + "virtual-size": 1073741824,
>> + "filename": "null-co://",
>> + "format": "null-co"
>> + },
>> "virtual-size": 1073741824,
>> "filename": "json:{\"throttle-group\": \"group0\", \"driver\": \"throttle\", \"file\": {\"driver\": \"null-co\"}}",
>> "format": "throttle"
>> @@ -34,7 +39,7 @@ Testing:
>> "iops_wr": 0,
>> "ro": false,
>> "node-name": "throttle0",
>> - "backing_file_depth": 0,
>> + "backing_file_depth": 1,
>> "drv": "throttle",
>> "iops": 0,
>> "bps_wr": 0,
>>
>
>