| Message ID | 20200925134229.246169-5-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block/export: add BlockExportOptions->iothread member | expand |
Am 25.09.2020 um 15:42 hat Stefan Hajnoczi geschrieben: > Make it possible to specify the iothread where the export will run. > > 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 | 4 ++++ > block/export/export.c | 26 +++++++++++++++++++++++++- > 2 files changed, 29 insertions(+), 1 deletion(-) > > diff --git a/qapi/block-export.json b/qapi/block-export.json > index 87ac5117cd..eba6f6eae9 100644 > --- a/qapi/block-export.json > +++ b/qapi/block-export.json > @@ -219,11 +219,15 @@ > # 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 the main loop thread. (since: 5.2) NBD exports currently switch automatically to a different AioContext if another user (usually a guest device using the same node) tries to change the AioContext. I believe this is also the most useful mode in the context of the system emulator. I can see the need for an iothread option in qemu-storage-daemon where usually nobody else will move the node into a different AioContext. But we need to define the semantics more precisely and specify what happens if another user wants to change the AioContext later. Currently, the NBD export will allow this and just switch the AioContext - after this patch, ignoring what the user set explicitly with this new option. I see two options to handle this more consistently: 1. If @iothread is set, move the block node into the requested AioContext, and if that fails, block-export-add fails. Other users of the node will be denied to change the AioContext while the export is active. If @iothread is not given, it behaves like today: Use whatever AioContext the node is currently in and switch whenever another user requests it. 2. Add a bool option @fixed-iothread that determines whether other users can change the AioContext while the export is active. Giving an @iothread and fixed-iothread == true means that we'll enforce the given AioContext during the whole lifetime of the export. With fixed-iothread == false it means that we try to move the block node to the requested iothread if possible (but we won't fail if it isn't possible) and will follow any other user switching the AioContext of the node. Not giving @iothread means that we start with the current AioContext of the node, and @fixed-iothread then means the same as before. Does this make sense to you? Kevin
On Fri, Sep 25, 2020 at 05:01:42PM +0200, Kevin Wolf wrote: > Am 25.09.2020 um 15:42 hat Stefan Hajnoczi geschrieben: > > Make it possible to specify the iothread where the export will run. > > > > 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 | 4 ++++ > > block/export/export.c | 26 +++++++++++++++++++++++++- > > 2 files changed, 29 insertions(+), 1 deletion(-) > > > > diff --git a/qapi/block-export.json b/qapi/block-export.json > > index 87ac5117cd..eba6f6eae9 100644 > > --- a/qapi/block-export.json > > +++ b/qapi/block-export.json > > @@ -219,11 +219,15 @@ > > # 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 the main loop thread. (since: 5.2) > > NBD exports currently switch automatically to a different AioContext if > another user (usually a guest device using the same node) tries to > change the AioContext. I believe this is also the most useful mode in > the context of the system emulator. > > I can see the need for an iothread option in qemu-storage-daemon where > usually nobody else will move the node into a different AioContext. > > But we need to define the semantics more precisely and specify what > happens if another user wants to change the AioContext later. Currently, > the NBD export will allow this and just switch the AioContext - after > this patch, ignoring what the user set explicitly with this new option. > > I see two options to handle this more consistently: > > 1. If @iothread is set, move the block node into the requested > AioContext, and if that fails, block-export-add fails. Other users of > the node will be denied to change the AioContext while the export is > active. > > If @iothread is not given, it behaves like today: Use whatever > AioContext the node is currently in and switch whenever another user > requests it. > > 2. Add a bool option @fixed-iothread that determines whether other users > can change the AioContext while the export is active. > > Giving an @iothread and fixed-iothread == true means that we'll > enforce the given AioContext during the whole lifetime of the export. > With fixed-iothread == false it means that we try to move the block > node to the requested iothread if possible (but we won't fail if it > isn't possible) and will follow any other user switching the > AioContext of the node. > > Not giving @iothread means that we start with the current AioContext > of the node, and @fixed-iothread then means the same as before. > > Does this make sense to you? Thanks for raising this. #2 is more flexible. I suspect most users will be happy with fixed-iothread = false by default (i.e. things keep working even if the iothread is changed). I can adjust this patch to follow those semantics. Stefan
diff --git a/qapi/block-export.json b/qapi/block-export.json index 87ac5117cd..eba6f6eae9 100644 --- a/qapi/block-export.json +++ b/qapi/block-export.json @@ -219,11 +219,15 @@ # 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 the main loop thread. (since: 5.2) +# # Since: 4.2 ## { 'union': 'BlockExportOptions', 'base': { 'type': 'BlockExportType', 'id': 'str', + '*iothread': 'str', 'node-name': 'str', '*writable': 'bool', '*writethrough': 'bool' }, diff --git a/block/export/export.c b/block/export/export.c index 550897e236..0fb3d76ee3 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" @@ -66,7 +67,7 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp) const BlockExportDriver *drv; BlockExport *exp = NULL; BlockDriverState *bs; - BlockBackend *blk; + BlockBackend *blk = NULL; AioContext *ctx; uint64_t perm; int ret; @@ -102,6 +103,29 @@ 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) { + goto fail; + } + + aio_context_release(ctx); + aio_context_acquire(new_ctx); + + ctx = new_ctx; + } + /* * Block exports are used for non-shared storage migration. Make sure * that BDRV_O_INACTIVE is cleared and the image is ready for write
Make it possible to specify the iothread where the export will run. 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 | 4 ++++ block/export/export.c | 26 +++++++++++++++++++++++++- 2 files changed, 29 insertions(+), 1 deletion(-)