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

[Markus Armbruster <armbru@redhat.com>]

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.

Second, QOMifying block nodes feels ambitious to me.  Please discuss it
with block maintainers Kevin and Hanna.

> And Markus, could you please take a look at patch 13?
> I will prepare the V6 after your comments.

Done.  It may need an update for changes made here, though.