Re: [PATCH 2/4] block: Add protocol-specific image info

[Hanna Reitz]

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