Re: [PATCH v2 4/4] block/export: add iothread and fixed-iothread options

[Eric Blake <eblake@redhat.com>]

On 9/29/20 7:55 AM, Stefan Hajnoczi wrote:
> Make it possible to specify the iothread where the export will run. By
> default the block node can be moved to other AioContexts later and the
> export will follow. The fixed-iothread option forces strict behavior
> that prevents changing AioContext while the export is active. See the
> QAPI docs for details.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> Note the x-blockdev-set-iothread QMP command can be used to do the same,
> but not from the command-line. And it requires sending an additional
> command.
> 
> In the long run vhost-user-blk will support per-virtqueue iothread
> mappings. But for now a single iothread makes sense and most other
> transports will just use one iothread anyway.
> ---
>   qapi/block-export.json               | 11 ++++++++++
>   block/export/export.c                | 31 +++++++++++++++++++++++++++-
>   block/export/vhost-user-blk-server.c |  5 ++++-
>   nbd/server.c                         |  2 --
>   4 files changed, 45 insertions(+), 4 deletions(-)
> 
> diff --git a/qapi/block-export.json b/qapi/block-export.json
> index 87ac5117cd..e2cb21f5f1 100644
> --- a/qapi/block-export.json
> +++ b/qapi/block-export.json
> @@ -219,11 +219,22 @@
>   #                export before completion is signalled. (since: 5.2;
>   #                default: false)
>   #
> +# @iothread: The name of the iothread object where the export will run. The
> +#            default is to use the thread currently associated with the #

Stray #

> +#            block node. (since: 5.2)
> +#
> +# @fixed-iothread: True prevents the block node from being moved to another
> +#                  thread while the export is active. If true and @iothread is
> +#                  given, export creation fails if the block node cannot be
> +#                  moved to the iothread. The default is false.
> +#

Missing a '(since 5.2)' tag.  (Hmm, we're inconsistent on whether it is 
'since 5.2' or 'since: 5.2' inside () parentheticals; Markus, is that 
something we should be cleaning up as part of the conversion to rST?)

> @@ -63,10 +64,11 @@ static const BlockExportDriver *blk_exp_find_driver(BlockExportType type)
>   
>   BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
>   {
> +    bool fixed_iothread = export->has_fixed_iothread && export->fixed_iothread;

Technically, our QAPI code guarantees that export->fixed_iothread is 
false if export->has_fixed_iothread is false.  And someday I'd love to 
let QAPI express default values for bools so that we don't need a 
has_FOO field when a default has been expressed.  But neither of those 
points affect this patch; what you have is correct even if it is verbose.

Otherwise looks reasonable.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org