| Message ID | 20180525163327.23097-5-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2018-05-25 18:33, Kevin Wolf wrote: > This changes the x-blockdev-create QMP command so that it doesn't block > the monitor and the main loop any more, but starts a background job that > performs the image creation. > > The basic job as implemented here is all that is necessary to make image > creation asynchronous and to provide a QMP interface that can be marked > stable, but it still lacks a few features that jobs usually provide: The > job will ignore pause commands and it doesn't publish progress yet (so > both current-progress and total-progress stay at 0). These features can > be added later without breaking compatibility. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > qapi/block-core.json | 14 +++++++---- > qapi/job.json | 4 +++- > block/create.c | 61 ++++++++++++++++++++++++++++++++---------------- > tests/qemu-iotests/group | 14 ++++++----- > 4 files changed, 61 insertions(+), 32 deletions(-) [...] > diff --git a/block/create.c b/block/create.c > index 8bd8a03719..87fdab3b72 100644 > --- a/block/create.c > +++ b/block/create.c > @@ -24,28 +24,49 @@ > > #include "qemu/osdep.h" > #include "block/block_int.h" > +#include "qemu/job.h" > #include "qapi/qapi-commands-block-core.h" > +#include "qapi/qapi-visit-block-core.h" > +#include "qapi/clone-visitor.h" > #include "qapi/error.h" > > -typedef struct BlockdevCreateCo { > +typedef struct BlockdevCreateJob { > + Job common; > BlockDriver *drv; > BlockdevCreateOptions *opts; > int ret; > - Error **errp; > -} BlockdevCreateCo; > + Error *err; > +} BlockdevCreateJob; > > -static void coroutine_fn bdrv_co_create_co_entry(void *opaque) > +static void blockdev_create_complete(Job *job, void *opaque) > { > - BlockdevCreateCo *cco = opaque; > - cco->ret = cco->drv->bdrv_co_create(cco->opts, cco->errp); > + BlockdevCreateJob *s = container_of(job, BlockdevCreateJob, common); > + > + job_completed(job, s->ret, s->err); > } > > -void qmp_x_blockdev_create(BlockdevCreateOptions *options, Error **errp) > +static void coroutine_fn blockdev_create_run(void *opaque) > { > + BlockdevCreateJob *s = opaque; > + > + s->ret = s->drv->bdrv_co_create(s->opts, &s->err); > + > + qapi_free_BlockdevCreateOptions(s->opts); > + job_defer_to_main_loop(&s->common, blockdev_create_complete, NULL); Better be safe than sorry, but probably not strictly necessary considering the job is always run in the main loop anyway (more on that below, though). > +} > + > +static const JobDriver blockdev_create_job_driver = { > + .instance_size = sizeof(BlockdevCreateJob), > + .job_type = JOB_TYPE_CREATE, > + .start = blockdev_create_run, > +}; > + > +void qmp_x_blockdev_create(const char *job_id, BlockdevCreateOptions *options, > + Error **errp) > +{ > + BlockdevCreateJob *s; > const char *fmt = BlockdevDriver_str(options->driver); > BlockDriver *drv = bdrv_find_format(fmt); > - Coroutine *co; > - BlockdevCreateCo cco; > > /* If the driver is in the schema, we know that it exists. But it may not > * be whitelisted. */ > @@ -61,16 +82,16 @@ void qmp_x_blockdev_create(BlockdevCreateOptions *options, Error **errp) Minor note: The comment above this if () block reads: /* Call callback if it exists */ Which is now no longer really what is happening. It just checks whether the block driver supports blockdev-create. > return; > } > > - cco = (BlockdevCreateCo) { > - .drv = drv, > - .opts = options, > - .ret = -EINPROGRESS, > - .errp = errp, > - }; > - > - co = qemu_coroutine_create(bdrv_co_create_co_entry, &cco); > - qemu_coroutine_enter(co); > - while (cco.ret == -EINPROGRESS) { > - aio_poll(qemu_get_aio_context(), true); > + /* Create the block job */ > + s = job_create(job_id, &blockdev_create_job_driver, NULL, > + qemu_get_aio_context(), JOB_DEFAULT | JOB_MANUAL_DISMISS, Hmmm... The old code already used the main AIO context, but is that correct? When you create a qcow2 node on top of a file node, shouldn't you use the AIO context of the file node? I see that figuring out the correct context generically may be difficult to impossible, so OK, maybe we should just run image creation in the main context for now. But then, qcow2 (and other formats) should at least take a lock on their file's context, which they don't seem to do. So I suppose I can give a Reviewed-by: Max Reitz <mreitz@redhat.com> for this patch, but I think some block drivers need some locking. Max > + NULL, NULL, errp); > + if (!s) { > + return; > } > + > + s->drv = drv, > + s->opts = QAPI_CLONE(BlockdevCreateOptions, options), > + > + job_start(&s->common); > }
On Fri, May 25, 2018 at 06:33:17PM +0200, Kevin Wolf wrote: > This changes the x-blockdev-create QMP command so that it doesn't block > the monitor and the main loop any more, but starts a background job that > performs the image creation. > This comes down to "user error", but now that the monitor and main loop are not blocked (which is good), should we try at all to catch/prevent a blockdev-add that is issued before a blockdev-create completion? > The basic job as implemented here is all that is necessary to make image > creation asynchronous and to provide a QMP interface that can be marked > stable, but it still lacks a few features that jobs usually provide: The > job will ignore pause commands and it doesn't publish progress yet (so > both current-progress and total-progress stay at 0). These features can > be added later without breaking compatibility. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > qapi/block-core.json | 14 +++++++---- > qapi/job.json | 4 +++- > block/create.c | 61 ++++++++++++++++++++++++++++++++---------------- > tests/qemu-iotests/group | 14 ++++++----- > 4 files changed, 61 insertions(+), 32 deletions(-) > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 7dfa77a05c..1ed3a82373 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -4013,14 +4013,18 @@ > ## > # @x-blockdev-create: > # > -# Create an image format on a given node. > -# TODO Replace with something asynchronous (block job?) > +# Starts a job to create an image format on a given node. The job is > +# automatically finalized, but a manual job-dismiss is required. > # > -# Since: 2.12 > +# @job-id: Identifier for the newly created job. > +# > +# @options: Options for the image creation. > +# > +# Since: 3.0 > ## > { 'command': 'x-blockdev-create', > - 'data': 'BlockdevCreateOptions', > - 'boxed': true } > + 'data': { 'job-id': 'str', > + 'options': 'BlockdevCreateOptions' } } > > ## > # @blockdev-open-tray: > diff --git a/qapi/job.json b/qapi/job.json > index 970124de76..69c1970a58 100644 > --- a/qapi/job.json > +++ b/qapi/job.json > @@ -17,10 +17,12 @@ > # > # @backup: drive backup job type, see "drive-backup" > # > +# @create: image creation job type, see "x-blockdev-create" (since 3.0) > +# > # Since: 1.7 > ## > { 'enum': 'JobType', > - 'data': ['commit', 'stream', 'mirror', 'backup'] } > + 'data': ['commit', 'stream', 'mirror', 'backup', 'create'] } > > ## > # @JobStatus: > diff --git a/block/create.c b/block/create.c > index 8bd8a03719..87fdab3b72 100644 > --- a/block/create.c > +++ b/block/create.c > @@ -24,28 +24,49 @@ > > #include "qemu/osdep.h" > #include "block/block_int.h" > +#include "qemu/job.h" > #include "qapi/qapi-commands-block-core.h" > +#include "qapi/qapi-visit-block-core.h" > +#include "qapi/clone-visitor.h" > #include "qapi/error.h" > > -typedef struct BlockdevCreateCo { > +typedef struct BlockdevCreateJob { > + Job common; > BlockDriver *drv; > BlockdevCreateOptions *opts; > int ret; > - Error **errp; > -} BlockdevCreateCo; > + Error *err; > +} BlockdevCreateJob; > > -static void coroutine_fn bdrv_co_create_co_entry(void *opaque) > +static void blockdev_create_complete(Job *job, void *opaque) > { > - BlockdevCreateCo *cco = opaque; > - cco->ret = cco->drv->bdrv_co_create(cco->opts, cco->errp); > + BlockdevCreateJob *s = container_of(job, BlockdevCreateJob, common); > + > + job_completed(job, s->ret, s->err); > } > > -void qmp_x_blockdev_create(BlockdevCreateOptions *options, Error **errp) > +static void coroutine_fn blockdev_create_run(void *opaque) > { > + BlockdevCreateJob *s = opaque; > + > + s->ret = s->drv->bdrv_co_create(s->opts, &s->err); > + > + qapi_free_BlockdevCreateOptions(s->opts); > + job_defer_to_main_loop(&s->common, blockdev_create_complete, NULL); > +} > + > +static const JobDriver blockdev_create_job_driver = { > + .instance_size = sizeof(BlockdevCreateJob), > + .job_type = JOB_TYPE_CREATE, > + .start = blockdev_create_run, > +}; > + > +void qmp_x_blockdev_create(const char *job_id, BlockdevCreateOptions *options, > + Error **errp) > +{ > + BlockdevCreateJob *s; > const char *fmt = BlockdevDriver_str(options->driver); > BlockDriver *drv = bdrv_find_format(fmt); > - Coroutine *co; > - BlockdevCreateCo cco; > > /* If the driver is in the schema, we know that it exists. But it may not > * be whitelisted. */ > @@ -61,16 +82,16 @@ void qmp_x_blockdev_create(BlockdevCreateOptions *options, Error **errp) > return; > } > > - cco = (BlockdevCreateCo) { > - .drv = drv, > - .opts = options, > - .ret = -EINPROGRESS, > - .errp = errp, > - }; > - > - co = qemu_coroutine_create(bdrv_co_create_co_entry, &cco); > - qemu_coroutine_enter(co); > - while (cco.ret == -EINPROGRESS) { > - aio_poll(qemu_get_aio_context(), true); > + /* Create the block job */ > + s = job_create(job_id, &blockdev_create_job_driver, NULL, > + qemu_get_aio_context(), JOB_DEFAULT | JOB_MANUAL_DISMISS, > + NULL, NULL, errp); > + if (!s) { > + return; > } > + > + s->drv = drv, > + s->opts = QAPI_CLONE(BlockdevCreateOptions, options), > + > + job_start(&s->common); > } > diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group > index 93f93d71ba..22b0082db3 100644 > --- a/tests/qemu-iotests/group > +++ b/tests/qemu-iotests/group > @@ -204,14 +204,16 @@ > 203 rw auto migration > 204 rw auto quick > 205 rw auto quick > -206 rw auto > -207 rw auto > +# TODO The following commented out tests need to be reworked to work > +# with the x-blockdev-create job > +#206 rw auto > +#207 rw auto > 208 rw auto quick > 209 rw auto quick > -210 rw auto > -211 rw auto quick > -212 rw auto quick > -213 rw auto quick > +#210 rw auto > +#211 rw auto quick > +#212 rw auto quick > +#213 rw auto quick > 214 rw auto > 215 rw auto quick > 216 rw auto quick > -- > 2.13.6 > >
Am 29.05.2018 um 17:27 hat Jeff Cody geschrieben: > On Fri, May 25, 2018 at 06:33:17PM +0200, Kevin Wolf wrote: > > This changes the x-blockdev-create QMP command so that it doesn't block > > the monitor and the main loop any more, but starts a background job that > > performs the image creation. > > > > This comes down to "user error", but now that the monitor and main loop are > not blocked (which is good), should we try at all to catch/prevent a > blockdev-add that is issued before a blockdev-create completion? Hm, currently, the .bdrv_co_create implementations usually do this: blk = blk_new(BLK_PERM_WRITE | BLK_PERM_RESIZE, BLK_PERM_ALL); I'm not sure if BLK_PERM_ALL is the best options for the shared permissions, but anyway format drivers can't share BLK_PERM_WRITE on their part, so I think we already do prevent this by taking the BLK_PERM_WRITE permission. Kevin
diff --git a/qapi/block-core.json b/qapi/block-core.json index 7dfa77a05c..1ed3a82373 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -4013,14 +4013,18 @@ ## # @x-blockdev-create: # -# Create an image format on a given node. -# TODO Replace with something asynchronous (block job?) +# Starts a job to create an image format on a given node. The job is +# automatically finalized, but a manual job-dismiss is required. # -# Since: 2.12 +# @job-id: Identifier for the newly created job. +# +# @options: Options for the image creation. +# +# Since: 3.0 ## { 'command': 'x-blockdev-create', - 'data': 'BlockdevCreateOptions', - 'boxed': true } + 'data': { 'job-id': 'str', + 'options': 'BlockdevCreateOptions' } } ## # @blockdev-open-tray: diff --git a/qapi/job.json b/qapi/job.json index 970124de76..69c1970a58 100644 --- a/qapi/job.json +++ b/qapi/job.json @@ -17,10 +17,12 @@ # # @backup: drive backup job type, see "drive-backup" # +# @create: image creation job type, see "x-blockdev-create" (since 3.0) +# # Since: 1.7 ## { 'enum': 'JobType', - 'data': ['commit', 'stream', 'mirror', 'backup'] } + 'data': ['commit', 'stream', 'mirror', 'backup', 'create'] } ## # @JobStatus: diff --git a/block/create.c b/block/create.c index 8bd8a03719..87fdab3b72 100644 --- a/block/create.c +++ b/block/create.c @@ -24,28 +24,49 @@ #include "qemu/osdep.h" #include "block/block_int.h" +#include "qemu/job.h" #include "qapi/qapi-commands-block-core.h" +#include "qapi/qapi-visit-block-core.h" +#include "qapi/clone-visitor.h" #include "qapi/error.h" -typedef struct BlockdevCreateCo { +typedef struct BlockdevCreateJob { + Job common; BlockDriver *drv; BlockdevCreateOptions *opts; int ret; - Error **errp; -} BlockdevCreateCo; + Error *err; +} BlockdevCreateJob; -static void coroutine_fn bdrv_co_create_co_entry(void *opaque) +static void blockdev_create_complete(Job *job, void *opaque) { - BlockdevCreateCo *cco = opaque; - cco->ret = cco->drv->bdrv_co_create(cco->opts, cco->errp); + BlockdevCreateJob *s = container_of(job, BlockdevCreateJob, common); + + job_completed(job, s->ret, s->err); } -void qmp_x_blockdev_create(BlockdevCreateOptions *options, Error **errp) +static void coroutine_fn blockdev_create_run(void *opaque) { + BlockdevCreateJob *s = opaque; + + s->ret = s->drv->bdrv_co_create(s->opts, &s->err); + + qapi_free_BlockdevCreateOptions(s->opts); + job_defer_to_main_loop(&s->common, blockdev_create_complete, NULL); +} + +static const JobDriver blockdev_create_job_driver = { + .instance_size = sizeof(BlockdevCreateJob), + .job_type = JOB_TYPE_CREATE, + .start = blockdev_create_run, +}; + +void qmp_x_blockdev_create(const char *job_id, BlockdevCreateOptions *options, + Error **errp) +{ + BlockdevCreateJob *s; const char *fmt = BlockdevDriver_str(options->driver); BlockDriver *drv = bdrv_find_format(fmt); - Coroutine *co; - BlockdevCreateCo cco; /* If the driver is in the schema, we know that it exists. But it may not * be whitelisted. */ @@ -61,16 +82,16 @@ void qmp_x_blockdev_create(BlockdevCreateOptions *options, Error **errp) return; } - cco = (BlockdevCreateCo) { - .drv = drv, - .opts = options, - .ret = -EINPROGRESS, - .errp = errp, - }; - - co = qemu_coroutine_create(bdrv_co_create_co_entry, &cco); - qemu_coroutine_enter(co); - while (cco.ret == -EINPROGRESS) { - aio_poll(qemu_get_aio_context(), true); + /* Create the block job */ + s = job_create(job_id, &blockdev_create_job_driver, NULL, + qemu_get_aio_context(), JOB_DEFAULT | JOB_MANUAL_DISMISS, + NULL, NULL, errp); + if (!s) { + return; } + + s->drv = drv, + s->opts = QAPI_CLONE(BlockdevCreateOptions, options), + + job_start(&s->common); } diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group index 93f93d71ba..22b0082db3 100644 --- a/tests/qemu-iotests/group +++ b/tests/qemu-iotests/group @@ -204,14 +204,16 @@ 203 rw auto migration 204 rw auto quick 205 rw auto quick -206 rw auto -207 rw auto +# TODO The following commented out tests need to be reworked to work +# with the x-blockdev-create job +#206 rw auto +#207 rw auto 208 rw auto quick 209 rw auto quick -210 rw auto -211 rw auto quick -212 rw auto quick -213 rw auto quick +#210 rw auto +#211 rw auto quick +#212 rw auto quick +#213 rw auto quick 214 rw auto 215 rw auto quick 216 rw auto quick
This changes the x-blockdev-create QMP command so that it doesn't block the monitor and the main loop any more, but starts a background job that performs the image creation. The basic job as implemented here is all that is necessary to make image creation asynchronous and to provide a QMP interface that can be marked stable, but it still lacks a few features that jobs usually provide: The job will ignore pause commands and it doesn't publish progress yet (so both current-progress and total-progress stay at 0). These features can be added later without breaking compatibility. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- qapi/block-core.json | 14 +++++++---- qapi/job.json | 4 +++- block/create.c | 61 ++++++++++++++++++++++++++++++++---------------- tests/qemu-iotests/group | 14 ++++++----- 4 files changed, 61 insertions(+), 32 deletions(-)