| Message ID | 20230607152627.468786-3-andrey.drobyshev@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qemu-img: map: implement support for compressed clusters | expand |
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. > + } > 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("]"); > }
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("]"); >> } >
On 7/6/23 15:10, Andrey Drobyshev wrote: > 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. you right, that makes sense Den
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' } } ## # @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 (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("]"); }
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(-)