Re: [PATCH 2/3] qemu-img: map: report compressed data blocks

[Andrey Drobyshev]

On 6/21/23 21:12, Denis V. Lunev wrote:
> On 6/7/23 17:26, Andrey Drobyshev wrote:
>> Right now "qemu-img map" reports compressed blocks as containing data
>> but having no host offset.  This is not very informative.  Instead,
>> let's add another boolean field named "compressed" in case JSON output
>> mode is specified.  This is achieved by utilizing new allocation status
>> flag BDRV_BLOCK_COMPRESSED for bdrv_block_status().
>>
>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>> ---
>>   qapi/block-core.json |  7 +++++--
>>   qemu-img.c           | 16 +++++++++++++---
>>   2 files changed, 18 insertions(+), 5 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 5dd5f7e4b0..bc6653e5d6 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -409,6 +409,9 @@
>>   #
>>   # @zero: whether the virtual blocks read as zeroes
>>   #
>> +# @compressed: true indicates that data is stored compressed (the target
>> +#     format must support compression)
>> +#
>>   # @depth: number of layers (0 = top image, 1 = top image's backing
>>   #     file, ..., n - 1 = bottom image (where n is the number of images
>>   #     in the chain)) before reaching one for which the range is
>> @@ -426,8 +429,8 @@
>>   ##
>>   { 'struct': 'MapEntry',
>>     'data': {'start': 'int', 'length': 'int', 'data': 'bool',
>> -           'zero': 'bool', 'depth': 'int', 'present': 'bool',
>> -           '*offset': 'int', '*filename': 'str' } }
>> +           'zero': 'bool', 'compressed': 'bool', 'depth': 'int',
>> +           'present': 'bool', '*offset': 'int', '*filename': 'str' } }
> after some thoughts I would say that for compatibility reasons it
> would be beneficial to have compressed field optional.
>>     ##
>>   # @BlockdevCacheInfo:
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 27f48051b0..9bb69f58f6 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -3083,7 +3083,7 @@ static int img_info(int argc, char **argv)
>>   }
>>     static int dump_map_entry(OutputFormat output_format, MapEntry *e,
>> -                          MapEntry *next)
>> +                          MapEntry *next, bool can_compress)
>>   {
>>       switch (output_format) {
>>       case OFORMAT_HUMAN:
>> @@ -3112,6 +3112,9 @@ static int dump_map_entry(OutputFormat
>> output_format, MapEntry *e,
>>                  e->present ? "true" : "false",
>>                  e->zero ? "true" : "false",
>>                  e->data ? "true" : "false");
>> +        if (can_compress) {
>> +            printf(", \"compressed\": %s", e->compressed ? "true" :
>> "false");
> If compressed field is optional, then it would be reasonable to skip
> filling this field for non-compressed clusters. In that case we
> will not need 'can_compress' parameter of the call.
> 
> Ha! More importantly. The field (according to the metadata) is
> mandatory while it is reported conditionally, i.e. the field is
> optional in reality. There is a problem in a this or that way.
> 

Yes, I agree that making this field optional makes sense since we do not
include it for formats which don't support compression.

However, I don't think we should entirely omit it for uncompressed
clusters.  If we keep it present that would be more consistent with the
current logic, as with '"zero": false' field which is always included.

>> +        }
>>           if (e->has_offset) {
>>               printf(", \"offset\": %"PRId64"", e->offset);
>>           }
>> @@ -3172,6 +3175,7 @@ static int get_block_status(BlockDriverState
>> *bs, int64_t offset,
>>           .length = bytes,
>>           .data = !!(ret & BDRV_BLOCK_DATA),
>>           .zero = !!(ret & BDRV_BLOCK_ZERO),
>> +        .compressed = !!(ret & BDRV_BLOCK_COMPRESSED),
>>           .offset = map,
>>           .has_offset = has_offset,
>>           .depth = depth,
>> @@ -3189,6 +3193,7 @@ static inline bool entry_mergeable(const
>> MapEntry *curr, const MapEntry *next)
>>       }
>>       if (curr->zero != next->zero ||
>>           curr->data != next->data ||
>> +        curr->compressed != next->compressed ||
>>           curr->depth != next->depth ||
>>           curr->present != next->present ||
>>           !curr->filename != !next->filename ||
>> @@ -3218,6 +3223,7 @@ static int img_map(int argc, char **argv)
>>       bool force_share = false;
>>       int64_t start_offset = 0;
>>       int64_t max_length = -1;
>> +    bool can_compress = false;
>>         fmt = NULL;
>>       output = NULL;
>> @@ -3313,6 +3319,10 @@ static int img_map(int argc, char **argv)
>>           length = MIN(start_offset + max_length, length);
>>       }
>>   +    if (output_format == OFORMAT_JSON) {
>> +        can_compress = block_driver_can_compress(bs->drv);
>> +    }
>> +
>>       curr.start = start_offset;
>>       while (curr.start + curr.length < length) {
>>           int64_t offset = curr.start + curr.length;
>> @@ -3330,7 +3340,7 @@ static int img_map(int argc, char **argv)
>>           }
>>             if (curr.length > 0) {
>> -            ret = dump_map_entry(output_format, &curr, &next);
>> +            ret = dump_map_entry(output_format, &curr, &next,
>> can_compress);
>>               if (ret < 0) {
>>                   goto out;
>>               }
>> @@ -3338,7 +3348,7 @@ static int img_map(int argc, char **argv)
>>           curr = next;
>>       }
>>   -    ret = dump_map_entry(output_format, &curr, NULL);
>> +    ret = dump_map_entry(output_format, &curr, NULL, can_compress);
>>       if (output_format == OFORMAT_JSON) {
>>           puts("]");
>>       }
>