diff mbox

[04/14] block/create: Make x-blockdev-create a job

Message ID 20180525163327.23097-5-kwolf@redhat.com
State New
Headers show

Commit Message

Kevin Wolf May 25, 2018, 4:33 p.m. UTC
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(-)

Comments

Max Reitz May 29, 2018, 11:38 a.m. UTC | #1
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);
>  }
Jeff Cody May 29, 2018, 3:27 p.m. UTC | #2
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
> 
>
Kevin Wolf May 29, 2018, 3:40 p.m. UTC | #3
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 mbox

Patch

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