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

[Stefan Hajnoczi]

On Tue, Sep 29, 2020 at 08:07:38AM -0500, Eric Blake wrote:
> 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.

Great, thanks for pointing this out.

I'll wait for comments from Kevin. These things could be fixed when
merging.

Stefan

signature.asc
Description: PGP signature