Re: [PATCH V5 04/13] blockdev: Update tracking iothread users with holder name

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 10138 bytes --]

On Mon, Mar 30, 2026 at 11:02:03AM +0200, Markus Armbruster wrote:
> Zhang Chen <zhangckid@gmail.com> writes:
> 
> > On Wed, Mar 18, 2026 at 5:13 PM Stefan Hajnoczi <stefanha@gmail.com> wrote:
> >>
> >> On Wed, Mar 18, 2026 at 2:20 PM Markus Armbruster <armbru@redhat.com> wrote:
> >> >
> >> > Zhang Chen <zhangckid@gmail.com> writes:
> >> >
> >> > > On Thu, Mar 12, 2026 at 5:16 PM Markus Armbruster <armbru@redhat.com> wrote:
> >> > >>
> >> > >> Context...  we're talking about this command:
> >> > >>
> >> > >>     ##
> >> > >>     # @x-blockdev-set-iothread:
> >> > >>     #
> >> > >>     # Move @node and its children into the @iothread.  If @iothread is
> >> > >>     # null then move @node and its children into the main loop.
> >> > >>     #
> >> > >>     # The node must not be attached to a BlockBackend.
> >> > >>     #
> >> > >>     # @node-name: the name of the block driver node
> >> > >>     #
> >> > >>     # @iothread: the name of the IOThread object or null for the main loop
> >> > >>     #
> >> > >>     # @force: true if the node and its children should be moved when a
> >> > >>     #     BlockBackend is already attached
> >> > >>     #
> >> > >>     # Features:
> >> > >>     #
> >> > >>     # @unstable: This command is experimental and intended for test cases
> >> > >>     #     that need control over IOThreads only.
> >> > >>     #
> >> > >>     # Since: 2.12
> >> > >>     #
> >> > >>     # .. qmp-example::
> >> > >>     #    :title: Move a node into an IOThread
> >> > >>     #
> >> > >>     #     -> { "execute": "x-blockdev-set-iothread",
> >> > >>     #          "arguments": { "node-name": "disk1",
> >> > >>     #                         "iothread": "iothread0" } }
> >> > >>     #     <- { "return": {} }
> >> > >>     #
> >> > >>     # .. qmp-example::
> >> > >>     #    :title: Move a node into the main loop
> >> > >>     #
> >> > >>     #     -> { "execute": "x-blockdev-set-iothread",
> >> > >>     #          "arguments": { "node-name": "disk1",
> >> > >>     #                         "iothread": null } }
> >> > >>     #     <- { "return": {} }
> >> > >>     ##
> >> > >>     { 'command': 'x-blockdev-set-iothread',
> >> > >>       'data' : { 'node-name': 'str',
> >> > >>                  'iothread': 'StrOrNull',
> >> > >>                  '*force': 'bool' },
> >> > >>       'features': [ 'unstable' ],
> >> > >>       'allow-preconfig': true }
> >> > >>
> >> > >>
> >> > >> Stefan Hajnoczi <stefanha@redhat.com> writes:
> >> > >>
> >> > >> > On Tue, Mar 10, 2026 at 06:02:54PM +0800, Zhang Chen wrote:
> >> > >> >> On Mon, Mar 9, 2026 at 4:15 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >> > >> >> >
> >> > >> >> > On Thu, Mar 05, 2026 at 10:24:50PM +0800, Zhang Chen wrote:
> >> > >> >> > > Update the usage of "iothread_get_aio_context()".
> >> > >> >> > >
> >> > >> >> > > Signed-off-by: Zhang Chen <zhangckid@gmail.com>
> >> > >> >> > > ---
> >> > >> >> > >  blockdev.c | 9 ++++++++-
> >> > >> >> > >  1 file changed, 8 insertions(+), 1 deletion(-)
> >> > >> >> > >
> >> > >> >> > > diff --git a/blockdev.c b/blockdev.c
> >> > >> >> > > index 6e86c6262f..01ccf64b3f 100644
> >> > >> >> > > --- a/blockdev.c
> >> > >> >> > > +++ b/blockdev.c
> >> > >> >> > > @@ -3683,7 +3683,14 @@ void qmp_x_blockdev_set_iothread(const char *node_name, StrOrNull *iothread,
> >> > >> >> > >              goto out;
> >> > >> >> > >          }
> >> > >> >> > >
> >> > >> >> > > -        new_context = iothread_get_aio_context(obj);
> >> > >> >> > > +        char *path = object_get_canonical_path(OBJECT(bs));
> >> > >> >> >
> >> > >> >> > CCing Kevin and Markus in case they have an opinion on this.
> >> > >> >> >
> >> > >> >> > BlockDriverState is not a QOM Object so using OBJECT(bs) is undefined
> >> > >> >> > behavior and may crash.
> >> > >>
> >> > >> Yes.
> >> > >>
> >> > >> >> > node_name is unique across block driver graph nodes and could be used.
> >> > >> >> > Unfortunately it's not connected to the QOM Object hierarchy.
> >> > >>
> >> > >> Correct.
> >> > >>
> >> > >> >> >                                                               Maybe it's
> >> > >> >> > best to build a holder name that is an invalid QOM path so there can be
> >> > >> >> > no collisions between QOM paths and block driver graph nodes.
> >> > >>
> >> > >> I guess you're talking about the values that go into IOThreadInfo member
> >> > >> holders.  From PATCH 13:
> >> > >>
> >> > >>     # @holders: The parameter is an array of QOM paths indicating how many
> >> > >>     #     active devices are currently associated with this iothread
> >> > >>     #     (e.g. virtio-blk).  In hotplug scenarios, users can
> >> > >>     #     pre-allocate multiple iothread objects to serve as a persistent
> >> > >>     #     thread pool.  When a device is hot-unplugged, the corresponding
> >> > >>     #     IOThread is released but remains available, allowing subsequent
> >> > >>     #     hot-plugged devices to attach to and reuse the existing thread.
> >> > >>     #     Returns empty if no devices are attached.  (since 11.0)
> >> > >>     #
> >> > >>
> >> > >> I further guess you need it to refer to both QOM objects and block
> >> > >> nodes, and you worry about ambiguity.
> >> > >>
> >> > >> Ambiguity indeed exists: a block node name can be a valid QOM path.
> >> > >>
> >> > >> We could restrict QOM paths to absolute paths.  These start with '/'.
> >> > >> If I remember correctly, node names cannot contain '/'.
> >> > >>
> >> > >> Note that canonical paths (returned object_get_canonical_path()) are
> >> > >> absolute.
> >> > >>
> >> > >> We ran into a similar design issue in review of Vladimir's "[PATCH v10
> >> > >> 4/8] qapi: add blockdev-replace command" not too long ago:
> >> > >>
> >> > >>     Subject: Re: [PATCH v10 4/8] qapi: add blockdev-replace command
> >> > >>     Date: Wed, 04 Feb 2026 13:26:35 +0100
> >> > >>     Message-ID: <87wm0sy9s4.fsf@pond.sub.org>
> >> > >>
> >> > >> There, the new command needs to refer to QOM object, block node, or
> >> > >> block export.
> >> > >>
> >> > >> >> >   g_autofree char *holder = g_strdup_printf("BlockDriverState %s", node_name);
> >> > >> >> >
> >> > >> >> > (A cleaner long-term solution would be making BlockDriverStates QOM
> >> > >> >> > Objects so they have a proper path.)
> >> > >>
> >> > >> Yes, but that's a beefy project, isn't it?
> >> > >>
> >> > >> >> If no other comments, it's OK for me. This issue like I mentioned in
> >> > >> >> patch 7 and 9.
> >> > >> >
> >> > >> > A thought about the QAPI interface:
> >> > >> >
> >> > >> > QAPI expresses as much information in the schema as possible, so I think
> >> > >> > the right approach would be a {'union': 'IOThreadHolder',
> >> > >> > 'discriminator': 'type', ...} that supports at least "qom" and
> >> > >> > "block-node". That way there are proper types to encode QOM Object paths
> >> > >> > vs block node-names. Let's avoid having a single string value that takes
> >> > >> > on different meaning depending on the type of holder.
> >> > >>
> >> > >> This shifts the complexity from semantics to syntax.
> >> > >>
> >> > >> Semantics: the member can have multiple meanings, and you have to
> >> > >> examine its value to decide which one applies.  The member's
> >> > >> documentation should specify how to decide.  Say something like "if the
> >> > >> value starts with '/', it's an absolute QOM path, else it's a block node
> >> > >> name".
> >> > >>
> >> > >> Syntax: meaning is syntactically obvious.  For instance, union of QOM
> >> > >> path and block node name.
> >> > >>
> >> > >> Complex semantics tend to require more complex documentation.
> >> > >>
> >> > >> Which choice is better depends on the specific case.  I generally lean
> >> > >> towards syntax.
> >> > >>
> >> > >
> >> > > I agree Markus's suggestion.
> >> > > Compare with standard QOM path:
> >> > > /machine/peripheral/blk0/virtio-backend
> >> > >
> >> > > I will try to implement block nodes path like this:
> >> > > /machine/blockdriverstate/node-name
> >> >
> >> > I gather you'd like to try creating QOM objects for block nodes.
> >> >
> >> > First, this should not go into /machine.  We already have /chardevs and
> >> > /audiodevs, which suggests something like /blockdevs or /block-nodes.
> >>
> >> It may be a lot of work. An alternative I suggested was a holder enum
> >> instead of a string so that QOM paths and block node names can be
> >> separated with no chance of collisions. That approach is more
> >> straightforward to implement and the main drawback I see is that the
> >> enum would become superfluous if block nodes become QOM objects in the
> >> future. That's not terrible.
> >>
> >> Stefan
> >
> > Ping....
> >
> > Hi Markus and Stefan,
> > As Markus's comments, I prefer the /blockdevs or /block-nodes is more simple,
> > And use a holder enum instead of a string will make it more difficult
> > for users to understand.
> > Please let us make a decision.
> 
> I'm not sure what exactly you need from me.
> 
> I assume you need advice on how to represent a reference to a "holder"
> in the QAPI schema, where the holder can either be a QOM object or a
> block node.
> 
> If everthing was a QOM object, we could simply use a QOM path,
> i.e. 'str'.
> 
> Sadly, block nodes are not QOM objects.  Unless you want to solve the
> (non-trivial) problem of making them QOM objects first, you need
> something else.
> 
> My recommendation for something else would be a union:
> 
>     { 'union': 'IoThreadHolder',
>       'base': { 'type': 'IoThreadHolderKind' },
>       'discriminator': 'type',
>       'data': {
>           'block-node': ...,
>           'qom-object': ... } }
> 
> Stefan, do you agree?

Yes.

Stefan

> 
> Hope this helps!
> 
> > Hi Kevin and Hanna,
> > Considering the possibility of blockdevs being qom in the future,  we
> > need your idea here, any comments for the series?
> >
> > Thanks
> > Chen
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]