Re: [Qemu-devel] [PATCH v5 24/42] block: Use child access functions for QAPI queries

[Max Reitz]

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 <address@hidden>
>> ---
>>   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 <address@hidden>
> 
>>       }
>>   
>> -    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,
>>
> 
>

signature.asc
Description: OpenPGP digital signature