| Message ID | 20230706163047.128999-3-andrey.drobyshev@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qemu-img: map: implement support for compressed clusters | expand |
On 7/6/23 18:30, 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..b263d2cd30 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. Optional, > +# only valid for the formats whith 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("]"); > } Reviewed-by: Denis V. Lunev <den@openvz.org>
On 06.07.23 18:30, 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(-) Patch 3 must be merged into this patch. Every test must pass on every commit so we don’t break bisecting. > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 5dd5f7e4b0..b263d2cd30 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. Optional, > +# only valid for the formats whith support compression This is missing information for when this field was introduced (i.e. a “(since 8.2)”). I also wonder why this field is optional. We have compression information even for formats that don’t support compression, specifically, nothing is compressed. I would just make this field mandatory and print it always. (A technical reason to do so is that this patch uses block_driver_can_compress() to figure out whether there is compression support; but that function only tells whether the driver can write compressed data. Even if it cannot do that, the format may still support compression, and the driver may be able to read compressed data, just not write it.) Hanna > +# > # @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 @@
On 8/25/23 17:14, Hanna Czenczek wrote: > On 06.07.23 18:30, 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(-) > > Patch 3 must be merged into this patch. Every test must pass on every > commit so we don’t break bisecting. Agreed, should've figured that myself. > >> diff --git a/qapi/block-core.json b/qapi/block-core.json >> index 5dd5f7e4b0..b263d2cd30 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. Optional, >> +# only valid for the formats whith support compression > > This is missing information for when this field was introduced (i.e. a > “(since 8.2)”). Noted. > > I also wonder why this field is optional. We have compression > information even for formats that don’t support compression, > specifically, nothing is compressed. I would just make this field > mandatory and print it always. (A technical reason to do so is that > this patch uses block_driver_can_compress() to figure out whether there > is compression support; but that function only tells whether the driver > can write compressed data. Even if it cannot do that, the format may > still support compression, and the driver may be able to read compressed > data, just not write it.) > I figured that for the formats which surely can't support compression, such as "raw", this information would simply be redundant. AFAICT right now the only drivers which can read compressed data are exactly the ones which can write it: vmdk, qcow and qcow2. But if we assume that this might change, and that we'd better show the field value no matter what (as we do with "zero" field) -- I'm OK with it.
On 29.08.23 08:44, Andrey Drobyshev wrote: > On 8/25/23 17:14, Hanna Czenczek wrote: >> On 06.07.23 18:30, 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(-) >> Patch 3 must be merged into this patch. Every test must pass on every >> commit so we don’t break bisecting. > Agreed, should've figured that myself. > >>> diff --git a/qapi/block-core.json b/qapi/block-core.json >>> index 5dd5f7e4b0..b263d2cd30 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. Optional, >>> +# only valid for the formats whith support compression >> This is missing information for when this field was introduced (i.e. a >> “(since 8.2)”). > Noted. > >> I also wonder why this field is optional. We have compression >> information even for formats that don’t support compression, >> specifically, nothing is compressed. I would just make this field >> mandatory and print it always. (A technical reason to do so is that >> this patch uses block_driver_can_compress() to figure out whether there >> is compression support; but that function only tells whether the driver >> can write compressed data. Even if it cannot do that, the format may >> still support compression, and the driver may be able to read compressed >> data, just not write it.) >> > I figured that for the formats which surely can't support compression, > such as "raw", this information would simply be redundant. AFAICT right > now the only drivers which can read compressed data are exactly the ones > which can write it: vmdk, qcow and qcow2. But if we assume that this > might change, and that we'd better show the field value no matter what > (as we do with "zero" field) -- I'm OK with it. It is indeed redundant, but as this is intended to be the machine-readable output, I don’t mind the output becoming arguably needlessly verbose.
diff --git a/qapi/block-core.json b/qapi/block-core.json index 5dd5f7e4b0..b263d2cd30 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. Optional, +# only valid for the formats whith 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(-)