| Message ID | 20220503145529.37070-3-hreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block/file: Show extent size in qemu-img info | expand |
Am 03.05.2022 um 16:55 hat Hanna Reitz geschrieben: > The ImageInfo object currently only contains (optional) format-specific > image information. However, perhaps the protocol node can provide some > additional information, so add a new field presenting it. > > Signed-off-by: Hanna Reitz <hreitz@redhat.com> > --- > qapi/block-core.json | 6 +++++- > block/qapi.c | 19 +++++++++++++++++++ > 2 files changed, 24 insertions(+), 1 deletion(-) > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index beeb91952a..e7d6c2e0cc 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -236,6 +236,9 @@ > # @format-specific: structure supplying additional format-specific > # information (since 1.7) > # > +# @protocol-specific: structure supplying additional protocol-specific > +# information (since 7.1) > +# > # Since: 1.3 > # > ## > @@ -246,7 +249,8 @@ > '*backing-filename': 'str', '*full-backing-filename': 'str', > '*backing-filename-format': 'str', '*snapshots': ['SnapshotInfo'], > '*backing-image': 'ImageInfo', > - '*format-specific': 'ImageInfoSpecific' } } > + '*format-specific': 'ImageInfoSpecific', > + '*protocol-specific': 'ImageInfoSpecific' } } I'm not a fan of this one. It solves the problem for exactly one special case (even if admittedly a common one) and leaves everything else as it is. It is unclear what it produces in configurations that aren't the simple one format node on top of one protocol node layout. I would rather interpret 'format-specific' as 'driver-specific' and make the ImageInfo for any child node accessible. With rbd we already interpret it like a generic driver thing that is not just for formats that because it implements .bdrv_get_specific_info even though we didn't have a 'protocol-specific' yet. Making other nodes has precedence, too. 'backing-image' is even in the context of this hunk. VMDK exposes its extents the same way. So maybe what we really want is a 'children' list with the ImageInfo of every child node. And then qemu-img could go through all children and print headings like "Driver specific information for file (#block123)". Then filters like blkdebug could add their information and it would be printed, too. > ## > # @ImageCheck: > diff --git a/block/qapi.c b/block/qapi.c > index 51202b470a..293983cf82 100644 > --- a/block/qapi.c > +++ b/block/qapi.c > @@ -262,6 +262,7 @@ void bdrv_query_image_info(BlockDriverState *bs, > int64_t size; > const char *backing_filename; > BlockDriverInfo bdi; > + BlockDriverState *protocol_bs; > int ret; > Error *err = NULL; > ImageInfo *info; > @@ -303,6 +304,24 @@ void bdrv_query_image_info(BlockDriverState *bs, > } > info->has_format_specific = info->format_specific != NULL; > > + /* Try to look for an unambiguous protocol node */ > + protocol_bs = bs; > + while (protocol_bs && !QLIST_EMPTY(&protocol_bs->children)) { > + protocol_bs = bdrv_primary_bs(protocol_bs); > + } If bs is already a leaf node, this duplicates the information, which looks weird: $ build/qemu-img info -f file ~/tmp/test.raw image: /home/kwolf/tmp/test.raw file format: file virtual size: 10 GiB (10737418240 bytes) disk size: 7.63 GiB Format specific information: extent size: 1048576 Protocol specific information: extent size: 1048576 > > + if (protocol_bs) { > + /* Assert that this is a protocol node */ > + assert(QLIST_EMPTY(&protocol_bs->children)); > + > + info->protocol_specific = bdrv_get_specific_info(protocol_bs, &err); > + if (err) { > + error_propagate(errp, err); > + qapi_free_ImageInfo(info); > + goto out; > + } > + info->has_protocol_specific = info->protocol_specific != NULL; > + } > + > backing_filename = bs->backing_file; > if (backing_filename[0] != '\0') { > char *backing_filename2; Kevin
On 04.05.22 10:36, Kevin Wolf wrote: > Am 03.05.2022 um 16:55 hat Hanna Reitz geschrieben: >> The ImageInfo object currently only contains (optional) format-specific >> image information. However, perhaps the protocol node can provide some >> additional information, so add a new field presenting it. >> >> Signed-off-by: Hanna Reitz <hreitz@redhat.com> >> --- >> qapi/block-core.json | 6 +++++- >> block/qapi.c | 19 +++++++++++++++++++ >> 2 files changed, 24 insertions(+), 1 deletion(-) >> >> diff --git a/qapi/block-core.json b/qapi/block-core.json >> index beeb91952a..e7d6c2e0cc 100644 >> --- a/qapi/block-core.json >> +++ b/qapi/block-core.json >> @@ -236,6 +236,9 @@ >> # @format-specific: structure supplying additional format-specific >> # information (since 1.7) >> # >> +# @protocol-specific: structure supplying additional protocol-specific >> +# information (since 7.1) >> +# >> # Since: 1.3 >> # >> ## >> @@ -246,7 +249,8 @@ >> '*backing-filename': 'str', '*full-backing-filename': 'str', >> '*backing-filename-format': 'str', '*snapshots': ['SnapshotInfo'], >> '*backing-image': 'ImageInfo', >> - '*format-specific': 'ImageInfoSpecific' } } >> + '*format-specific': 'ImageInfoSpecific', >> + '*protocol-specific': 'ImageInfoSpecific' } } > I'm not a fan of this one. It solves the problem for exactly one special > case (even if admittedly a common one) and leaves everything else as it > is. It is unclear what it produces in configurations that aren't the > simple one format node on top of one protocol node layout. I don’t disagree, but I do wonder how often this structure is used outside of `qemu-img info`, where filter nodes and more complex configurations are very rare. I understand wanting to support complex block graph configurations everywhere, I’m just wondering whether there is actually much of a use for that here. > I would rather interpret 'format-specific' as 'driver-specific' and make > the ImageInfo for any child node accessible. Again, I don’t disagree, but I have reservations about that. I don’t think this is a trivial approach to take. First, that will be kind of bad for VMDK files, which already have all of their extent children in their driver-specific info, so we’d duplicate that info. Second, same for the backing child, generally. Do we want to exclude specifically the backing child from that list of ImageInfos for all children, because we already have an entry for it in ImageInfo itself? That wouldn’t make much sense. Deprecate backing-child? Works for the future, weird in the meantime. Third, the implementation would not be trivial. bdrv_query_image_info() specifically says to return "flat" image information, i.e. not to query the backing child information. Currently, its callers fill that blank some way or another, with `qemu-img info` creating a list of files (i.e. the backing chain) instead of using that backing-image field. I actually have no idea how we should bring that together. Should bdrv_query_image_info() also not collect that ImageInfo list of all children, and then collect_image_info_list() will put those into its list, too, making it recursive? Then we have the problem of describing nodes in this graph, and as written below I wouldn’t be happy to use auto-generated node names for this. Or should bdrv_query_image_info() collect all those children, and then collect_image_info_list() will just drop the backing child from them, so that it still gets a flat backing chain list, but the other children will be nested, allowing users to identify which nodes those are based on nesting? (And nesting would require adding indentation support to bdrv_image_info_dump(), and bdrv_snapshot_dump().) Fourth, precisely for the common case of not having filters or other more complex configurations, the additional info provided by the protocol node’s ImageInfo is limited. Most of it just duplicates information from the format node, the really interesting bit is just the ImageInfoSpecific, so for `qemu-img info` it’ll mostly just clutter the output. Many fields are also named on the assumption that this information is about a format node ("file format", "virtual size"), and so I personally find it confusing to see those things in the information about a protocol node when using `qemu-img info`. > With rbd we already interpret it like a generic driver thing that is not > just for formats that because it implements .bdrv_get_specific_info even > though we didn't have a 'protocol-specific' yet. On one hand, that’s the same thing I’m doing in this series. On the other, I think the rbd implementation as a whole has not been well thought out, because it must have faced exactly the same problem that I’m trying to solve in this patch here, but obviously it hasn’t been addressed yet. (Instead, it probably relied on users calling `qemu-img info -f rbd`, which is just cheating. I mean, I could do that, too, and just drop anything but patch 4.) > Making other nodes has precedence, too. 'backing-image' is even in the > context of this hunk. VMDK exposes its extents the same way. Both of which now make the solution to include the list of all children’s ImageInfos just more complicated, yes. O:) (I know that me saying that simply means that these were probably bad solutions then, and that maybe we should’ve had a list of all children’s ImageInfos from the start. Which means dancing around the issue even more won’t make it better, I know. O:) I’m just trying to say that simply adding this list isn’t an ideal solution now, under the current circumstances; but I’m not saying there is any ideal solution.) > So maybe > what we really want is a 'children' list with the ImageInfo of every > child node. And then qemu-img could go through all children and print > headings like "Driver specific information for file (#block123)". I would very much rather drop auto-generated node names, and instead just print the child name and rely on indentation. I have an example below. > Then > filters like blkdebug could add their information and it would be > printed, too. Is this really something that would ever be useful in practice? I understand your concern (and share it to a degree), but I feel like allowing for this ImageInfo struct to represent and encompass a complex block graph comes at the detriment of readability and understandability of `qemu-img info` output for plain images. For example, this is how I’d imagine the output for a raw image: image: test.raw file format: raw virtual size: 10 GiB (10737418240 bytes) disk size: 1 MiB child 'file': image: test.raw file format: file virtual size: 10 GiB (10737418240 bytes) disk size: 1 MiB Driver specific information: extent size: 1048576 Personally, I like that less than what this series’s v1 produces. I understand it represents the modular nature of the block graph, but that’s generally not something I want to see when I run `qemu-img info` on a plain image (which is 98 % of the use I have for `qemu-img info`). >> ## >> # @ImageCheck: >> diff --git a/block/qapi.c b/block/qapi.c >> index 51202b470a..293983cf82 100644 >> --- a/block/qapi.c >> +++ b/block/qapi.c >> @@ -262,6 +262,7 @@ void bdrv_query_image_info(BlockDriverState *bs, >> int64_t size; >> const char *backing_filename; >> BlockDriverInfo bdi; >> + BlockDriverState *protocol_bs; >> int ret; >> Error *err = NULL; >> ImageInfo *info; >> @@ -303,6 +304,24 @@ void bdrv_query_image_info(BlockDriverState *bs, >> } >> info->has_format_specific = info->format_specific != NULL; >> >> + /* Try to look for an unambiguous protocol node */ >> + protocol_bs = bs; >> + while (protocol_bs && !QLIST_EMPTY(&protocol_bs->children)) { >> + protocol_bs = bdrv_primary_bs(protocol_bs); >> + } > If bs is already a leaf node, this duplicates the information, which > looks weird: > > $ build/qemu-img info -f file ~/tmp/test.raw > image: /home/kwolf/tmp/test.raw > file format: file > virtual size: 10 GiB (10737418240 bytes) > disk size: 7.63 GiB > Format specific information: > extent size: 1048576 > Protocol specific information: > extent size: 1048576 I mean, that isn’t wrong, but also fixable if need be. >> + if (protocol_bs) { >> + /* Assert that this is a protocol node */ >> + assert(QLIST_EMPTY(&protocol_bs->children)); >> + >> + info->protocol_specific = bdrv_get_specific_info(protocol_bs, &err); >> + if (err) { >> + error_propagate(errp, err); >> + qapi_free_ImageInfo(info); >> + goto out; >> + } >> + info->has_protocol_specific = info->protocol_specific != NULL; >> + } >> + >> backing_filename = bs->backing_file; >> if (backing_filename[0] != '\0') { >> char *backing_filename2; > Kevin >
diff --git a/qapi/block-core.json b/qapi/block-core.json index beeb91952a..e7d6c2e0cc 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -236,6 +236,9 @@ # @format-specific: structure supplying additional format-specific # information (since 1.7) # +# @protocol-specific: structure supplying additional protocol-specific +# information (since 7.1) +# # Since: 1.3 # ## @@ -246,7 +249,8 @@ '*backing-filename': 'str', '*full-backing-filename': 'str', '*backing-filename-format': 'str', '*snapshots': ['SnapshotInfo'], '*backing-image': 'ImageInfo', - '*format-specific': 'ImageInfoSpecific' } } + '*format-specific': 'ImageInfoSpecific', + '*protocol-specific': 'ImageInfoSpecific' } } ## # @ImageCheck: diff --git a/block/qapi.c b/block/qapi.c index 51202b470a..293983cf82 100644 --- a/block/qapi.c +++ b/block/qapi.c @@ -262,6 +262,7 @@ void bdrv_query_image_info(BlockDriverState *bs, int64_t size; const char *backing_filename; BlockDriverInfo bdi; + BlockDriverState *protocol_bs; int ret; Error *err = NULL; ImageInfo *info; @@ -303,6 +304,24 @@ void bdrv_query_image_info(BlockDriverState *bs, } info->has_format_specific = info->format_specific != NULL; + /* Try to look for an unambiguous protocol node */ + protocol_bs = bs; + while (protocol_bs && !QLIST_EMPTY(&protocol_bs->children)) { + protocol_bs = bdrv_primary_bs(protocol_bs); + } + if (protocol_bs) { + /* Assert that this is a protocol node */ + assert(QLIST_EMPTY(&protocol_bs->children)); + + info->protocol_specific = bdrv_get_specific_info(protocol_bs, &err); + if (err) { + error_propagate(errp, err); + qapi_free_ImageInfo(info); + goto out; + } + info->has_protocol_specific = info->protocol_specific != NULL; + } + backing_filename = bs->backing_file; if (backing_filename[0] != '\0') { char *backing_filename2;
The ImageInfo object currently only contains (optional) format-specific image information. However, perhaps the protocol node can provide some additional information, so add a new field presenting it. Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- qapi/block-core.json | 6 +++++- block/qapi.c | 19 +++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-)