| Message ID | 20200929125516.186715-5-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block/export: add BlockExportOptions->iothread member | expand |
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.
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
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 # +# 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. +# # Since: 4.2 ## { 'union': 'BlockExportOptions', 'base': { 'type': 'BlockExportType', 'id': 'str', + '*fixed-iothread': 'bool', + '*iothread': 'str', 'node-name': 'str', '*writable': 'bool', '*writethrough': 'bool' }, diff --git a/block/export/export.c b/block/export/export.c index 550897e236..a5b6b02703 100644 --- a/block/export/export.c +++ b/block/export/export.c @@ -15,6 +15,7 @@ #include "block/block.h" #include "sysemu/block-backend.h" +#include "sysemu/iothread.h" #include "block/export.h" #include "block/nbd.h" #include "qapi/error.h" @@ -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; const BlockExportDriver *drv; BlockExport *exp = NULL; BlockDriverState *bs; - BlockBackend *blk; + BlockBackend *blk = NULL; AioContext *ctx; uint64_t perm; int ret; @@ -102,6 +104,28 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp) ctx = bdrv_get_aio_context(bs); aio_context_acquire(ctx); + if (export->has_iothread) { + IOThread *iothread; + AioContext *new_ctx; + + iothread = iothread_by_id(export->iothread); + if (!iothread) { + error_setg(errp, "iothread \"%s\" not found", export->iothread); + goto fail; + } + + new_ctx = iothread_get_aio_context(iothread); + + ret = bdrv_try_set_aio_context(bs, new_ctx, errp); + if (ret == 0) { + aio_context_release(ctx); + aio_context_acquire(new_ctx); + ctx = new_ctx; + } else if (fixed_iothread) { + goto fail; + } + } + /* * Block exports are used for non-shared storage migration. Make sure * that BDRV_O_INACTIVE is cleared and the image is ready for write @@ -116,6 +140,11 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp) } blk = blk_new(ctx, perm, BLK_PERM_ALL); + + if (!fixed_iothread) { + blk_set_allow_aio_context_change(blk, true); + } + ret = blk_insert_bs(blk, bs, errp); if (ret < 0) { goto fail; diff --git a/block/export/vhost-user-blk-server.c b/block/export/vhost-user-blk-server.c index 81072a5a46..a1c37548e1 100644 --- a/block/export/vhost-user-blk-server.c +++ b/block/export/vhost-user-blk-server.c @@ -323,13 +323,17 @@ static const VuDevIface vu_blk_iface = { static void blk_aio_attached(AioContext *ctx, void *opaque) { VuBlkExport *vexp = opaque; + + vexp->export.ctx = ctx; vhost_user_server_attach_aio_context(&vexp->vu_server, ctx); } static void blk_aio_detach(void *opaque) { VuBlkExport *vexp = opaque; + vhost_user_server_detach_aio_context(&vexp->vu_server); + vexp->export.ctx = NULL; } static void @@ -384,7 +388,6 @@ static int vu_blk_exp_create(BlockExport *exp, BlockExportOptions *opts, vu_blk_initialize_config(blk_bs(exp->blk), &vexp->blkcfg, logical_block_size); - blk_set_allow_aio_context_change(exp->blk, true); blk_add_aio_context_notifier(exp->blk, blk_aio_attached, blk_aio_detach, vexp); diff --git a/nbd/server.c b/nbd/server.c index f74766add7..8676008319 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -1560,8 +1560,6 @@ static int nbd_export_create(BlockExport *blk_exp, BlockExportOptions *exp_args, return ret; } - blk_set_allow_aio_context_change(blk, true); - QTAILQ_INIT(&exp->clients); exp->name = g_strdup(arg->name); exp->description = g_strdup(arg->description);
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(-)