[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/4] block: Add protocol-specific image info
From: |
Hanna Reitz |
Subject: |
Re: [PATCH 2/4] block: Add protocol-specific image info |
Date: |
Wed, 4 May 2022 13:25:08 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.8.0 |
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
- Re: [PATCH 1/4] block: Improve empty format-specific info dump, (continued)