Re: [Qemu-devel] [PATCH 04/14] block/create: Make x-blockdev-create a job

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5007 bytes --]

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);
>  }


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]