[Qemu-devel] [PATCH 00/21] jobs: defer graph changes until finalize

[Qemu-devel] [PATCH 00/21] jobs: defer graph changes until finalize

[John Snow]

This series forces all jobs to use the "finalize" semantics that were
introduced previously, but only exposed via the backup jobs.
This series looks huge, but it's mostly mechanical changes broken out
into a series of much smaller commits so that the changes are easier
to follow at each step.

Patches one and two refactor jobs to allow a centralized call to
job_completed, which allows us to avoid calling this function from
underneath job_defer_to_main_loop_bh, which takes the aio_context lock.

Patches 3-10 take advantage of the new centralized job exit code, one
job at a time. This should be a net reduction in SLOC.

Patch 11 removes the old job_defer_to_main_loop code. If it wasn't a net
reduction in SLOC before, it is now.

Patch 12 is not necessary, but changes job entry routines to return
a status code instead of asking you to store the retcode in the job
object, so that coroutine functions read more like standard ones.
SLOC balance is a wash slightly against my favor.

Patches 13-15 add job creation flags to the commit, mirror, and stream
jobs respectively which previously did not filter these flags down to
job creation time.

Patches 16-18 refactor the completion code for commit, mirror and stream
respectively to allow graph manipulations to happen exclusively after the
finalization step instead of immediately.

Patches 19-21 expose the new job creation flags to users via QMP.

John Snow (21):
  jobs: canonize Error object
  jobs: add exit shim
  block/backup: utilize job_exit shim
  block/commit: utilize job_exit shim
  block/mirror: utilize job_exit shim
  block/stream: utilize job_exit shim
  block/create: utilize job_exit shim
  tests/test-blockjob-txn: utilize job_exit shim
  tests/test-blockjob: utilize job_exit shim
  tests/test-bdrv-drain: utilize job_exit shim
  jobs: remove job_defer_to_main_loop
  jobs: allow entrypoints to return status code
  block/commit: add block job creation flags
  block/mirror: add block job creation flags
  block/stream: add block job creation flags
  block/commit: refactor commit to use job callbacks
  block/mirror: conservative mirror_exit refactor
  block/commit: refactor stream to use job callbacks
  qapi/block-commit: expose new job properties
  qapi/block-mirror: expose new job properties
  qapi/block-stream: expose new job properties

 block/backup.c            |  21 ++-------
 block/commit.c            | 116 +++++++++++++++++++++++++++-------------------
 block/create.c            |  16 ++-----
 block/mirror.c            |  54 +++++++++++++--------
 block/stream.c            |  43 ++++++++---------
 blockdev.c                |  44 ++++++++++++++++--
 hmp.c                     |   5 +-
 include/block/block_int.h |  15 ++++--
 include/qemu/job.h        |  36 +++++---------
 job-qmp.c                 |   5 +-
 job.c                     |  64 ++++++++-----------------
 qapi/block-core.json      |  12 +++--
 tests/test-bdrv-drain.c   |  11 ++---
 tests/test-blockjob-txn.c |  23 ++++-----
 tests/test-blockjob.c     |  16 +++----
 15 files changed, 247 insertions(+), 234 deletions(-)

-- 
2.14.4

[Qemu-devel] [PATCH 01/21] jobs: canonize Error object

[John Snow]

Jobs presently use both an Error object in the case of the create job,
and char strings in the case of generic errors elsewhere.

Unify the two paths as just j->err, and remove the extra argument from
job_completed.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/backup.c            |  2 +-
 block/commit.c            |  2 +-
 block/create.c            |  5 ++---
 block/mirror.c            |  2 +-
 block/stream.c            |  2 +-
 include/qemu/job.h        | 10 ++++------
 job-qmp.c                 |  5 +++--
 job.c                     | 17 +++++------------
 tests/test-bdrv-drain.c   |  2 +-
 tests/test-blockjob-txn.c |  2 +-
 tests/test-blockjob.c     |  2 +-
 11 files changed, 21 insertions(+), 30 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 8630d32926..f3bf842423 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -388,7 +388,7 @@ static void backup_complete(Job *job, void *opaque)
 {
     BackupCompleteData *data = opaque;
 
-    job_completed(job, data->ret, NULL);
+    job_completed(job, data->ret);
     g_free(data);
 }
 
diff --git a/block/commit.c b/block/commit.c
index e1814d9693..620666161b 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -117,7 +117,7 @@ static void commit_complete(Job *job, void *opaque)
      * bdrv_set_backing_hd() to fail. */
     block_job_remove_all_bdrv(bjob);
 
-    job_completed(job, ret, NULL);
+    job_completed(job, ret);
     g_free(data);
 
     /* If bdrv_drop_intermediate() didn't already do that, remove the commit
diff --git a/block/create.c b/block/create.c
index 915cd41bcc..84bc74b7de 100644
--- a/block/create.c
+++ b/block/create.c
@@ -35,14 +35,13 @@ typedef struct BlockdevCreateJob {
     BlockDriver *drv;
     BlockdevCreateOptions *opts;
     int ret;
-    Error *err;
 } BlockdevCreateJob;
 
 static void blockdev_create_complete(Job *job, void *opaque)
 {
     BlockdevCreateJob *s = container_of(job, BlockdevCreateJob, common);
 
-    job_completed(job, s->ret, s->err);
+    job_completed(job, s->ret);
 }
 
 static void coroutine_fn blockdev_create_run(void *opaque)
@@ -50,7 +49,7 @@ static void coroutine_fn blockdev_create_run(void *opaque)
     BlockdevCreateJob *s = opaque;
 
     job_progress_set_remaining(&s->common, 1);
-    s->ret = s->drv->bdrv_co_create(s->opts, &s->err);
+    s->ret = s->drv->bdrv_co_create(s->opts, &s->common.err);
     job_progress_update(&s->common, 1);
 
     qapi_free_BlockdevCreateOptions(s->opts);
diff --git a/block/mirror.c b/block/mirror.c
index b48c3f8cf5..7c2c6ba67e 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -710,7 +710,7 @@ static void mirror_exit(Job *job, void *opaque)
     blk_insert_bs(bjob->blk, mirror_top_bs, &error_abort);
 
     bs_opaque->job = NULL;
-    job_completed(job, data->ret, NULL);
+    job_completed(job, data->ret);
 
     g_free(data);
     bdrv_drained_end(src);
diff --git a/block/stream.c b/block/stream.c
index 9264b68a1e..a5d6e0cf8a 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -93,7 +93,7 @@ out:
     }
 
     g_free(s->backing_file_str);
-    job_completed(job, data->ret, NULL);
+    job_completed(job, data->ret);
     g_free(data);
 }
 
diff --git a/include/qemu/job.h b/include/qemu/job.h
index 18c9223e31..845ad00c03 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -124,12 +124,12 @@ typedef struct Job {
     /** Estimated progress_current value at the completion of the job */
     int64_t progress_total;
 
-    /** Error string for a failed job (NULL if, and only if, job->ret == 0) */
-    char *error;
-
     /** ret code passed to job_completed. */
     int ret;
 
+    /** Error object for a failed job **/
+    Error *err;
+
     /** The completion function that will be called when the job completes.  */
     BlockCompletionFunc *cb;
 
@@ -484,15 +484,13 @@ void job_transition_to_ready(Job *job);
 /**
  * @job: The job being completed.
  * @ret: The status code.
- * @error: The error message for a failing job (only with @ret < 0). If @ret is
- *         negative, but NULL is given for @error, strerror() is used.
  *
  * Marks @job as completed. If @ret is non-zero, the job transaction it is part
  * of is aborted. If @ret is zero, the job moves into the WAITING state. If it
  * is the last job to complete in its transaction, all jobs in the transaction
  * move from WAITING to PENDING.
  */
-void job_completed(Job *job, int ret, Error *error);
+void job_completed(Job *job, int ret);
 
 /** Asynchronously complete the specified @job. */
 void job_complete(Job *job, Error **errp);
diff --git a/job-qmp.c b/job-qmp.c
index 410775df61..a969b2bbf0 100644
--- a/job-qmp.c
+++ b/job-qmp.c
@@ -146,8 +146,9 @@ static JobInfo *job_query_single(Job *job, Error **errp)
         .status             = job->status,
         .current_progress   = job->progress_current,
         .total_progress     = job->progress_total,
-        .has_error          = !!job->error,
-        .error              = g_strdup(job->error),
+        .has_error          = !!job->err,
+        .error              = job->err ? \
+                              g_strdup(error_get_pretty(job->err)) : NULL,
     };
 
     return info;
diff --git a/job.c b/job.c
index fa671b431a..b281f30375 100644
--- a/job.c
+++ b/job.c
@@ -369,7 +369,7 @@ void job_unref(Job *job)
 
         QLIST_REMOVE(job, job_list);
 
-        g_free(job->error);
+        error_free(job->err);
         g_free(job->id);
         g_free(job);
     }
@@ -535,7 +535,6 @@ void job_drain(Job *job)
     }
 }
 
-
 /**
  * All jobs must allow a pause point before entering their job proper. This
  * ensures that jobs can be paused prior to being started, then resumed later.
@@ -666,8 +665,8 @@ static void job_update_rc(Job *job)
         job->ret = -ECANCELED;
     }
     if (job->ret) {
-        if (!job->error) {
-            job->error = g_strdup(strerror(-job->ret));
+        if (!job->err) {
+            error_setg_errno(&job->err, -job->ret, "job failed");
         }
         job_state_transition(job, JOB_STATUS_ABORTING);
     }
@@ -865,17 +864,11 @@ static void job_completed_txn_success(Job *job)
     }
 }
 
-void job_completed(Job *job, int ret, Error *error)
+void job_completed(Job *job, int ret)
 {
     assert(job && job->txn && !job_is_completed(job));
 
     job->ret = ret;
-    if (error) {
-        assert(job->ret < 0);
-        job->error = g_strdup(error_get_pretty(error));
-        error_free(error);
-    }
-
     job_update_rc(job);
     trace_job_completed(job, ret, job->ret);
     if (job->ret) {
@@ -893,7 +886,7 @@ void job_cancel(Job *job, bool force)
     }
     job_cancel_async(job, force);
     if (!job_started(job)) {
-        job_completed(job, -ECANCELED, NULL);
+        job_completed(job, -ECANCELED);
     } else if (job->deferred_to_main_loop) {
         job_completed_txn_abort(job);
     } else {
diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
index 17bb8508ae..7b05082cae 100644
--- a/tests/test-bdrv-drain.c
+++ b/tests/test-bdrv-drain.c
@@ -754,7 +754,7 @@ typedef struct TestBlockJob {
 
 static void test_job_completed(Job *job, void *opaque)
 {
-    job_completed(job, 0, NULL);
+    job_completed(job, 0);
 }
 
 static void coroutine_fn test_job_start(void *opaque)
diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
index 58d9b87fb2..fce836639a 100644
--- a/tests/test-blockjob-txn.c
+++ b/tests/test-blockjob-txn.c
@@ -34,7 +34,7 @@ static void test_block_job_complete(Job *job, void *opaque)
         rc = -ECANCELED;
     }
 
-    job_completed(job, rc, NULL);
+    job_completed(job, rc);
     bdrv_unref(bs);
 }
 
diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
index cb42f06e61..e408d52351 100644
--- a/tests/test-blockjob.c
+++ b/tests/test-blockjob.c
@@ -167,7 +167,7 @@ static void cancel_job_completed(Job *job, void *opaque)
 {
     CancelJob *s = opaque;
     s->completed = true;
-    job_completed(job, 0, NULL);
+    job_completed(job, 0);
 }
 
 static void cancel_job_complete(Job *job, Error **errp)
-- 
2.14.4

[Qemu-devel] [PATCH 02/21] jobs: add exit shim

[John Snow]

Most jobs do the same thing when they leave their running loop:
- Store the return code in a structure
- wait to receive this structure in the main thread
- signal job completion via job_completed

More seriously, when we utilize job_defer_to_main_loop_bh to call
a function that calls job_completed, job_finalize_single will run
in a context where it has recursively taken the aio_context lock,
which can cause hangs if it puts down a reference that causes a flush.

The job infrastructure is perfectly capable of registering job
completion itself when we leave the job's entry point. In this
context, we can signal job completion from outside of the aio_context,
which should allow for job cleanup code to run with only one lock.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 include/qemu/job.h |  7 +++++++
 job.c              | 19 +++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/include/qemu/job.h b/include/qemu/job.h
index 845ad00c03..0c24e8704f 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -204,6 +204,13 @@ struct JobDriver {
      */
     void (*drain)(Job *job);
 
+    /**
+     * If the callback is not NULL, exit will be invoked from the main thread
+     * when the job's coroutine has finished, but before transactional
+     * convergence; before @prepare or @abort.
+     */
+    void (*exit)(Job *job);
+
     /**
      * If the callback is not NULL, prepare will be invoked when all the jobs
      * belonging to the same transaction complete; or upon this job's completion
diff --git a/job.c b/job.c
index b281f30375..cc5ac9ac30 100644
--- a/job.c
+++ b/job.c
@@ -535,6 +535,19 @@ void job_drain(Job *job)
     }
 }
 
+static void job_exit(void *opaque)
+{
+    Job *job = (Job *)opaque;
+    AioContext *aio_context = job->aio_context;
+
+    if (job->driver->exit) {
+        aio_context_acquire(aio_context);
+        job->driver->exit(job);
+        aio_context_release(aio_context);
+    }
+    job_completed(job, job->ret);
+}
+
 /**
  * All jobs must allow a pause point before entering their job proper. This
  * ensures that jobs can be paused prior to being started, then resumed later.
@@ -546,6 +559,12 @@ static void coroutine_fn job_co_entry(void *opaque)
     assert(job && job->driver && job->driver->start);
     job_pause_point(job);
     job->driver->start(job);
+    if (!job->deferred_to_main_loop) {
+        job->deferred_to_main_loop = true;
+        aio_bh_schedule_oneshot(qemu_get_aio_context(),
+                                job_exit,
+                                job);
+    }
 }
 
 
-- 
2.14.4

[Qemu-devel] [PATCH 03/21] block/backup: utilize job_exit shim

[John Snow]

Utilize the job_exit shim by simply not calling job_defer_to_main_loop.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/backup.c | 17 +----------------
 1 file changed, 1 insertion(+), 16 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index f3bf842423..af6ee5ab54 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -380,18 +380,6 @@ static BlockErrorAction backup_error_action(BackupBlockJob *job,
     }
 }
 
-typedef struct {
-    int ret;
-} BackupCompleteData;
-
-static void backup_complete(Job *job, void *opaque)
-{
-    BackupCompleteData *data = opaque;
-
-    job_completed(job, data->ret);
-    g_free(data);
-}
-
 static bool coroutine_fn yield_and_check(BackupBlockJob *job)
 {
     uint64_t delay_ns;
@@ -483,7 +471,6 @@ static void backup_incremental_init_copy_bitmap(BackupBlockJob *job)
 static void coroutine_fn backup_run(void *opaque)
 {
     BackupBlockJob *job = opaque;
-    BackupCompleteData *data;
     BlockDriverState *bs = blk_bs(job->common.blk);
     int64_t offset, nb_clusters;
     int ret = 0;
@@ -584,9 +571,7 @@ static void coroutine_fn backup_run(void *opaque)
     qemu_co_rwlock_unlock(&job->flush_rwlock);
     hbitmap_free(job->copy_bitmap);
 
-    data = g_malloc(sizeof(*data));
-    data->ret = ret;
-    job_defer_to_main_loop(&job->common.job, backup_complete, data);
+    job->common.job.ret = ret;
 }
 
 static const BlockJobDriver backup_job_driver = {
-- 
2.14.4

[Qemu-devel] [PATCH 04/21] block/commit: utilize job_exit shim

[John Snow]

Change the manual deferment to commit_complete into the implicit
callback to job_exit.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/commit.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index 620666161b..5bed098d5f 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -68,19 +68,14 @@ static int coroutine_fn commit_populate(BlockBackend *bs, BlockBackend *base,
     return 0;
 }
 
-typedef struct {
-    int ret;
-} CommitCompleteData;
-
-static void commit_complete(Job *job, void *opaque)
+static void commit_exit(Job *job)
 {
     CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
     BlockJob *bjob = &s->common;
-    CommitCompleteData *data = opaque;
     BlockDriverState *top = blk_bs(s->top);
     BlockDriverState *base = blk_bs(s->base);
     BlockDriverState *commit_top_bs = s->commit_top_bs;
-    int ret = data->ret;
+    int ret = job->ret;
     bool remove_commit_top_bs = false;
 
     /* Make sure commit_top_bs and top stay around until bdrv_replace_node() */
@@ -117,9 +112,6 @@ static void commit_complete(Job *job, void *opaque)
      * bdrv_set_backing_hd() to fail. */
     block_job_remove_all_bdrv(bjob);
 
-    job_completed(job, ret);
-    g_free(data);
-
     /* If bdrv_drop_intermediate() didn't already do that, remove the commit
      * filter driver from the backing chain. Do this as the final step so that
      * the 'consistent read' permission can be granted.  */
@@ -132,12 +124,12 @@ static void commit_complete(Job *job, void *opaque)
 
     bdrv_unref(commit_top_bs);
     bdrv_unref(top);
+    job->ret = ret;
 }
 
 static void coroutine_fn commit_run(void *opaque)
 {
     CommitBlockJob *s = opaque;
-    CommitCompleteData *data;
     int64_t offset;
     uint64_t delay_ns = 0;
     int ret = 0;
@@ -210,9 +202,7 @@ static void coroutine_fn commit_run(void *opaque)
 out:
     qemu_vfree(buf);
 
-    data = g_malloc(sizeof(*data));
-    data->ret = ret;
-    job_defer_to_main_loop(&s->common.job, commit_complete, data);
+    s->common.job.ret = ret;
 }
 
 static const BlockJobDriver commit_job_driver = {
@@ -223,6 +213,7 @@ static const BlockJobDriver commit_job_driver = {
         .user_resume   = block_job_user_resume,
         .drain         = block_job_drain,
         .start         = commit_run,
+        .exit          = commit_exit,
     },
 };
 
-- 
2.14.4

[Qemu-devel] [PATCH 05/21] block/mirror: utilize job_exit shim

[John Snow]

Change the manual deferment to mirror_exit into the implicit
callback to job_exit and the mirror_exit callback.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/mirror.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 7c2c6ba67e..e3404e2518 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -607,21 +607,17 @@ static void mirror_wait_for_all_io(MirrorBlockJob *s)
     }
 }
 
-typedef struct {
-    int ret;
-} MirrorExitData;
-
-static void mirror_exit(Job *job, void *opaque)
+static void mirror_exit(Job *job)
 {
     MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
     BlockJob *bjob = &s->common;
-    MirrorExitData *data = opaque;
     MirrorBDSOpaque *bs_opaque = s->mirror_top_bs->opaque;
     AioContext *replace_aio_context = NULL;
     BlockDriverState *src = s->mirror_top_bs->backing->bs;
     BlockDriverState *target_bs = blk_bs(s->target);
     BlockDriverState *mirror_top_bs = s->mirror_top_bs;
     Error *local_err = NULL;
+    int ret = job->ret;
 
     bdrv_release_dirty_bitmap(src, s->dirty_bitmap);
 
@@ -652,7 +648,7 @@ static void mirror_exit(Job *job, void *opaque)
             bdrv_set_backing_hd(target_bs, backing, &local_err);
             if (local_err) {
                 error_report_err(local_err);
-                data->ret = -EPERM;
+                ret = -EPERM;
             }
         }
     }
@@ -662,7 +658,7 @@ static void mirror_exit(Job *job, void *opaque)
         aio_context_acquire(replace_aio_context);
     }
 
-    if (s->should_complete && data->ret == 0) {
+    if (s->should_complete && ret == 0) {
         BlockDriverState *to_replace = src;
         if (s->to_replace) {
             to_replace = s->to_replace;
@@ -679,7 +675,7 @@ static void mirror_exit(Job *job, void *opaque)
         bdrv_drained_end(target_bs);
         if (local_err) {
             error_report_err(local_err);
-            data->ret = -EPERM;
+            ret = -EPERM;
         }
     }
     if (s->to_replace) {
@@ -710,12 +706,12 @@ static void mirror_exit(Job *job, void *opaque)
     blk_insert_bs(bjob->blk, mirror_top_bs, &error_abort);
 
     bs_opaque->job = NULL;
-    job_completed(job, data->ret);
 
-    g_free(data);
     bdrv_drained_end(src);
     bdrv_unref(mirror_top_bs);
     bdrv_unref(src);
+
+    job->ret = ret;
 }
 
 static void mirror_throttle(MirrorBlockJob *s)
@@ -815,7 +811,6 @@ static int mirror_flush(MirrorBlockJob *s)
 static void coroutine_fn mirror_run(void *opaque)
 {
     MirrorBlockJob *s = opaque;
-    MirrorExitData *data;
     BlockDriverState *bs = s->mirror_top_bs->backing->bs;
     BlockDriverState *target_bs = blk_bs(s->target);
     bool need_drain = true;
@@ -1035,13 +1030,11 @@ immediate_exit:
     g_free(s->in_flight_bitmap);
     bdrv_dirty_iter_free(s->dbi);
 
-    data = g_malloc(sizeof(*data));
-    data->ret = ret;
-
     if (need_drain) {
         bdrv_drained_begin(bs);
     }
-    job_defer_to_main_loop(&s->common.job, mirror_exit, data);
+
+    s->common.job.ret = ret;
 }
 
 static void mirror_complete(Job *job, Error **errp)
@@ -1139,6 +1132,7 @@ static const BlockJobDriver mirror_job_driver = {
         .user_resume            = block_job_user_resume,
         .drain                  = block_job_drain,
         .start                  = mirror_run,
+        .exit                   = mirror_exit,
         .pause                  = mirror_pause,
         .complete               = mirror_complete,
     },
@@ -1155,6 +1149,7 @@ static const BlockJobDriver commit_active_job_driver = {
         .user_resume            = block_job_user_resume,
         .drain                  = block_job_drain,
         .start                  = mirror_run,
+        .exit                   = mirror_exit,
         .pause                  = mirror_pause,
         .complete               = mirror_complete,
     },
-- 
2.14.4

[Qemu-devel] [PATCH 06/21] block/stream: utilize job_exit shim

[John Snow]

Change the manual deferment to stream_complete into the implicit
callback to job_exit.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/stream.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index a5d6e0cf8a..163cd6431c 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -54,20 +54,16 @@ static int coroutine_fn stream_populate(BlockBackend *blk,
     return blk_co_preadv(blk, offset, qiov.size, &qiov, BDRV_REQ_COPY_ON_READ);
 }
 
-typedef struct {
-    int ret;
-} StreamCompleteData;
-
-static void stream_complete(Job *job, void *opaque)
+static void stream_exit(Job *job)
 {
     StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
     BlockJob *bjob = &s->common;
-    StreamCompleteData *data = opaque;
     BlockDriverState *bs = blk_bs(bjob->blk);
     BlockDriverState *base = s->base;
     Error *local_err = NULL;
+    int ret = job->ret;
 
-    if (!job_is_cancelled(job) && bs->backing && data->ret == 0) {
+    if (!job_is_cancelled(job) && bs->backing && ret == 0) {
         const char *base_id = NULL, *base_fmt = NULL;
         if (base) {
             base_id = s->backing_file_str;
@@ -75,11 +71,11 @@ static void stream_complete(Job *job, void *opaque)
                 base_fmt = base->drv->format_name;
             }
         }
-        data->ret = bdrv_change_backing_file(bs, base_id, base_fmt);
+        ret = bdrv_change_backing_file(bs, base_id, base_fmt);
         bdrv_set_backing_hd(bs, base, &local_err);
         if (local_err) {
             error_report_err(local_err);
-            data->ret = -EPERM;
+            ret = -EPERM;
             goto out;
         }
     }
@@ -93,14 +89,12 @@ out:
     }
 
     g_free(s->backing_file_str);
-    job_completed(job, data->ret);
-    g_free(data);
+    job->ret = ret;
 }
 
 static void coroutine_fn stream_run(void *opaque)
 {
     StreamBlockJob *s = opaque;
-    StreamCompleteData *data;
     BlockBackend *blk = s->common.blk;
     BlockDriverState *bs = blk_bs(blk);
     BlockDriverState *base = s->base;
@@ -203,9 +197,7 @@ static void coroutine_fn stream_run(void *opaque)
 
 out:
     /* Modify backing chain and close BDSes in main loop */
-    data = g_malloc(sizeof(*data));
-    data->ret = ret;
-    job_defer_to_main_loop(&s->common.job, stream_complete, data);
+    s->common.job.ret = ret;
 }
 
 static const BlockJobDriver stream_job_driver = {
@@ -214,6 +206,7 @@ static const BlockJobDriver stream_job_driver = {
         .job_type      = JOB_TYPE_STREAM,
         .free          = block_job_free,
         .start         = stream_run,
+        .exit          = stream_exit,
         .user_resume   = block_job_user_resume,
         .drain         = block_job_drain,
     },
-- 
2.14.4

[Qemu-devel] [PATCH 07/21] block/create: utilize job_exit shim

[John Snow]

Utilize the job_exit shim by simply not calling job_defer_to_main_loop.
While we're here, we don't need to duplicate the core job object's `ret`.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/create.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/block/create.c b/block/create.c
index 84bc74b7de..3aeafc68cf 100644
--- a/block/create.c
+++ b/block/create.c
@@ -34,26 +34,17 @@ typedef struct BlockdevCreateJob {
     Job common;
     BlockDriver *drv;
     BlockdevCreateOptions *opts;
-    int ret;
 } BlockdevCreateJob;
 
-static void blockdev_create_complete(Job *job, void *opaque)
-{
-    BlockdevCreateJob *s = container_of(job, BlockdevCreateJob, common);
-
-    job_completed(job, s->ret);
-}
-
 static void coroutine_fn blockdev_create_run(void *opaque)
 {
     BlockdevCreateJob *s = opaque;
 
     job_progress_set_remaining(&s->common, 1);
-    s->ret = s->drv->bdrv_co_create(s->opts, &s->common.err);
+    s->common.ret = s->drv->bdrv_co_create(s->opts, &s->common.err);
     job_progress_update(&s->common, 1);
 
     qapi_free_BlockdevCreateOptions(s->opts);
-    job_defer_to_main_loop(&s->common, blockdev_create_complete, NULL);
 }
 
 static const JobDriver blockdev_create_job_driver = {
-- 
2.14.4

[Qemu-devel] [PATCH 08/21] tests/test-blockjob-txn: utilize job_exit shim

[John Snow]

This is safe to do because job_complete which will get called
implicitly already handles resetting the job error code if the job
gets cancelled, so this stanza was wasted effort.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/test-blockjob-txn.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
index fce836639a..307726d089 100644
--- a/tests/test-blockjob-txn.c
+++ b/tests/test-blockjob-txn.c
@@ -24,17 +24,11 @@ typedef struct {
     int *result;
 } TestBlockJob;
 
-static void test_block_job_complete(Job *job, void *opaque)
+static void test_block_job_exit(Job *job)
 {
     BlockJob *bjob = container_of(job, BlockJob, job);
     BlockDriverState *bs = blk_bs(bjob->blk);
-    int rc = (intptr_t)opaque;
 
-    if (job_is_cancelled(job)) {
-        rc = -ECANCELED;
-    }
-
-    job_completed(job, rc);
     bdrv_unref(bs);
 }
 
@@ -55,8 +49,7 @@ static void coroutine_fn test_block_job_run(void *opaque)
         }
     }
 
-    job_defer_to_main_loop(&job->job, test_block_job_complete,
-                           (void *)(intptr_t)s->rc);
+    s->common.job.ret = s->rc;
 }
 
 typedef struct {
@@ -81,6 +74,7 @@ static const BlockJobDriver test_block_job_driver = {
         .user_resume   = block_job_user_resume,
         .drain         = block_job_drain,
         .start         = test_block_job_run,
+        .exit          = test_block_job_exit,
     },
 };
 
-- 
2.14.4

[Qemu-devel] [PATCH 09/21] tests/test-blockjob: utilize job_exit shim

[John Snow]

Change the manual deferment to test_block_job_complete into the implicit
callback to job_exit.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/test-blockjob.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
index e408d52351..20563bde4f 100644
--- a/tests/test-blockjob.c
+++ b/tests/test-blockjob.c
@@ -163,11 +163,10 @@ typedef struct CancelJob {
     bool completed;
 } CancelJob;
 
-static void cancel_job_completed(Job *job, void *opaque)
+static void cancel_job_exit(Job *job)
 {
-    CancelJob *s = opaque;
+    CancelJob *s = container_of(job, CancelJob, common.job);
     s->completed = true;
-    job_completed(job, 0);
 }
 
 static void cancel_job_complete(Job *job, Error **errp)
@@ -182,7 +181,7 @@ static void coroutine_fn cancel_job_start(void *opaque)
 
     while (!s->should_complete) {
         if (job_is_cancelled(&s->common.job)) {
-            goto defer;
+            return;
         }
 
         if (!job_is_ready(&s->common.job) && s->should_converge) {
@@ -191,9 +190,6 @@ static void coroutine_fn cancel_job_start(void *opaque)
 
         job_sleep_ns(&s->common.job, 100000);
     }
-
- defer:
-    job_defer_to_main_loop(&s->common.job, cancel_job_completed, s);
 }
 
 static const BlockJobDriver test_cancel_driver = {
@@ -203,6 +199,7 @@ static const BlockJobDriver test_cancel_driver = {
         .user_resume   = block_job_user_resume,
         .drain         = block_job_drain,
         .start         = cancel_job_start,
+        .exit          = cancel_job_exit,
         .complete      = cancel_job_complete,
     },
 };
-- 
2.14.4

[Qemu-devel] [PATCH 10/21] tests/test-bdrv-drain: utilize job_exit shim

[John Snow]

Utilize the job_exit shim by simply not calling job_defer_to_main_loop.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/test-bdrv-drain.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
index 7b05082cae..f30310596c 100644
--- a/tests/test-bdrv-drain.c
+++ b/tests/test-bdrv-drain.c
@@ -752,11 +752,6 @@ typedef struct TestBlockJob {
     bool should_complete;
 } TestBlockJob;
 
-static void test_job_completed(Job *job, void *opaque)
-{
-    job_completed(job, 0);
-}
-
 static void coroutine_fn test_job_start(void *opaque)
 {
     TestBlockJob *s = opaque;
@@ -769,8 +764,6 @@ static void coroutine_fn test_job_start(void *opaque)
         qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 100000);
         job_pause_point(&s->common.job);
     }
-
-    job_defer_to_main_loop(&s->common.job, test_job_completed, NULL);
 }
 
 static void test_job_complete(Job *job, Error **errp)
-- 
2.14.4

[Qemu-devel] [PATCH 11/21] jobs: remove job_defer_to_main_loop

[John Snow]

The job infrastructure handles this now, so remove this call.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 include/qemu/job.h | 17 -----------------
 job.c              | 40 ++--------------------------------------
 2 files changed, 2 insertions(+), 55 deletions(-)

diff --git a/include/qemu/job.h b/include/qemu/job.h
index 0c24e8704f..02df75f45d 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -558,23 +558,6 @@ void job_finalize(Job *job, Error **errp);
  */
 void job_dismiss(Job **job, Error **errp);
 
-typedef void JobDeferToMainLoopFn(Job *job, void *opaque);
-
-/**
- * @job: The job
- * @fn: The function to run in the main loop
- * @opaque: The opaque value that is passed to @fn
- *
- * This function must be called by the main job coroutine just before it
- * returns.  @fn is executed in the main loop with the job AioContext acquired.
- *
- * Block jobs must call bdrv_unref(), bdrv_close(), and anything that uses
- * bdrv_drain_all() in the main loop.
- *
- * The @job AioContext is held while @fn executes.
- */
-void job_defer_to_main_loop(Job *job, JobDeferToMainLoopFn *fn, void *opaque);
-
 /**
  * Synchronously finishes the given @job. If @finish is given, it is called to
  * trigger completion or cancellation of the job.
diff --git a/job.c b/job.c
index cc5ac9ac30..fa6a1cd62c 100644
--- a/job.c
+++ b/job.c
@@ -559,12 +559,8 @@ static void coroutine_fn job_co_entry(void *opaque)
     assert(job && job->driver && job->driver->start);
     job_pause_point(job);
     job->driver->start(job);
-    if (!job->deferred_to_main_loop) {
-        job->deferred_to_main_loop = true;
-        aio_bh_schedule_oneshot(qemu_get_aio_context(),
-                                job_exit,
-                                job);
-    }
+    job->deferred_to_main_loop = true;
+    aio_bh_schedule_oneshot(qemu_get_aio_context(), job_exit, job);
 }
 
 
@@ -968,38 +964,6 @@ void job_complete(Job *job, Error **errp)
     job->driver->complete(job, errp);
 }
 
-
-typedef struct {
-    Job *job;
-    JobDeferToMainLoopFn *fn;
-    void *opaque;
-} JobDeferToMainLoopData;
-
-static void job_defer_to_main_loop_bh(void *opaque)
-{
-    JobDeferToMainLoopData *data = opaque;
-    Job *job = data->job;
-    AioContext *aio_context = job->aio_context;
-
-    aio_context_acquire(aio_context);
-    data->fn(data->job, data->opaque);
-    aio_context_release(aio_context);
-
-    g_free(data);
-}
-
-void job_defer_to_main_loop(Job *job, JobDeferToMainLoopFn *fn, void *opaque)
-{
-    JobDeferToMainLoopData *data = g_malloc(sizeof(*data));
-    data->job = job;
-    data->fn = fn;
-    data->opaque = opaque;
-    job->deferred_to_main_loop = true;
-
-    aio_bh_schedule_oneshot(qemu_get_aio_context(),
-                            job_defer_to_main_loop_bh, data);
-}
-
 int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), Error **errp)
 {
     Error *local_err = NULL;
-- 
2.14.4

[Qemu-devel] [PATCH 12/21] jobs: allow entrypoints to return status code

[John Snow]

Instead of awkwardly setting the return code in the job object, we can
just return this on the stack and catch it in the caller. This also has
the side-effect of changing the "opaque" type in the entrypoint to the
more specific "Job" type, so change function signatures accordingly.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/backup.c            |  6 +++---
 block/commit.c            |  6 +++---
 block/create.c            |  8 +++++---
 block/mirror.c            |  6 +++---
 block/stream.c            |  6 +++---
 include/qemu/job.h        |  2 +-
 job.c                     |  2 +-
 tests/test-bdrv-drain.c   |  6 ++++--
 tests/test-blockjob-txn.c | 13 ++++++-------
 tests/test-blockjob.c     |  9 ++++++---
 10 files changed, 35 insertions(+), 29 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index af6ee5ab54..c903416c5f 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -468,9 +468,9 @@ static void backup_incremental_init_copy_bitmap(BackupBlockJob *job)
     bdrv_dirty_iter_free(dbi);
 }
 
-static void coroutine_fn backup_run(void *opaque)
+static int coroutine_fn backup_run(Job *opaque_job)
 {
-    BackupBlockJob *job = opaque;
+    BackupBlockJob *job = container_of(opaque_job, BackupBlockJob, common.job);
     BlockDriverState *bs = blk_bs(job->common.blk);
     int64_t offset, nb_clusters;
     int ret = 0;
@@ -571,7 +571,7 @@ static void coroutine_fn backup_run(void *opaque)
     qemu_co_rwlock_unlock(&job->flush_rwlock);
     hbitmap_free(job->copy_bitmap);
 
-    job->common.job.ret = ret;
+    return ret;
 }
 
 static const BlockJobDriver backup_job_driver = {
diff --git a/block/commit.c b/block/commit.c
index 5bed098d5f..22712cac2d 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -127,9 +127,9 @@ static void commit_exit(Job *job)
     job->ret = ret;
 }
 
-static void coroutine_fn commit_run(void *opaque)
+static int coroutine_fn commit_run(Job *job)
 {
-    CommitBlockJob *s = opaque;
+    CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
     int64_t offset;
     uint64_t delay_ns = 0;
     int ret = 0;
@@ -202,7 +202,7 @@ static void coroutine_fn commit_run(void *opaque)
 out:
     qemu_vfree(buf);
 
-    s->common.job.ret = ret;
+    return ret;
 }
 
 static const BlockJobDriver commit_job_driver = {
diff --git a/block/create.c b/block/create.c
index 3aeafc68cf..9de55b973a 100644
--- a/block/create.c
+++ b/block/create.c
@@ -36,15 +36,17 @@ typedef struct BlockdevCreateJob {
     BlockdevCreateOptions *opts;
 } BlockdevCreateJob;
 
-static void coroutine_fn blockdev_create_run(void *opaque)
+static int coroutine_fn blockdev_create_run(Job *job)
 {
-    BlockdevCreateJob *s = opaque;
+    int ret;
+    BlockdevCreateJob *s = container_of(job, BlockdevCreateJob, common);
 
     job_progress_set_remaining(&s->common, 1);
-    s->common.ret = s->drv->bdrv_co_create(s->opts, &s->common.err);
+    ret = s->drv->bdrv_co_create(s->opts, &s->common.err);
     job_progress_update(&s->common, 1);
 
     qapi_free_BlockdevCreateOptions(s->opts);
+    return ret;
 }
 
 static const JobDriver blockdev_create_job_driver = {
diff --git a/block/mirror.c b/block/mirror.c
index e3404e2518..acf874393e 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -808,9 +808,9 @@ static int mirror_flush(MirrorBlockJob *s)
     return ret;
 }
 
-static void coroutine_fn mirror_run(void *opaque)
+static int coroutine_fn mirror_run(Job *job)
 {
-    MirrorBlockJob *s = opaque;
+    MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
     BlockDriverState *bs = s->mirror_top_bs->backing->bs;
     BlockDriverState *target_bs = blk_bs(s->target);
     bool need_drain = true;
@@ -1034,7 +1034,7 @@ immediate_exit:
         bdrv_drained_begin(bs);
     }
 
-    s->common.job.ret = ret;
+    return ret;
 }
 
 static void mirror_complete(Job *job, Error **errp)
diff --git a/block/stream.c b/block/stream.c
index 163cd6431c..2bdbca779d 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -92,9 +92,9 @@ out:
     job->ret = ret;
 }
 
-static void coroutine_fn stream_run(void *opaque)
+static int coroutine_fn stream_run(Job *job)
 {
-    StreamBlockJob *s = opaque;
+    StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
     BlockBackend *blk = s->common.blk;
     BlockDriverState *bs = blk_bs(blk);
     BlockDriverState *base = s->base;
@@ -197,7 +197,7 @@ static void coroutine_fn stream_run(void *opaque)
 
 out:
     /* Modify backing chain and close BDSes in main loop */
-    s->common.job.ret = ret;
+    return ret;
 }
 
 static const BlockJobDriver stream_job_driver = {
diff --git a/include/qemu/job.h b/include/qemu/job.h
index 02df75f45d..eef03bb06d 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -169,7 +169,7 @@ struct JobDriver {
     JobType job_type;
 
     /** Mandatory: Entrypoint for the Coroutine. */
-    CoroutineEntry *start;
+    int coroutine_fn (*start)(Job *job);
 
     /**
      * If the callback is not NULL, it will be invoked when the job transitions
diff --git a/job.c b/job.c
index fa6a1cd62c..09ce8900df 100644
--- a/job.c
+++ b/job.c
@@ -558,7 +558,7 @@ static void coroutine_fn job_co_entry(void *opaque)
 
     assert(job && job->driver && job->driver->start);
     job_pause_point(job);
-    job->driver->start(job);
+    job->ret = job->driver->start(job);
     job->deferred_to_main_loop = true;
     aio_bh_schedule_oneshot(qemu_get_aio_context(), job_exit, job);
 }
diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
index f30310596c..64188970e8 100644
--- a/tests/test-bdrv-drain.c
+++ b/tests/test-bdrv-drain.c
@@ -752,9 +752,9 @@ typedef struct TestBlockJob {
     bool should_complete;
 } TestBlockJob;
 
-static void coroutine_fn test_job_start(void *opaque)
+static int coroutine_fn test_job_start(Job *job)
 {
-    TestBlockJob *s = opaque;
+    TestBlockJob *s = container_of(job, TestBlockJob, common.job);
 
     job_transition_to_ready(&s->common.job);
     while (!s->should_complete) {
@@ -764,6 +764,8 @@ static void coroutine_fn test_job_start(void *opaque)
         qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 100000);
         job_pause_point(&s->common.job);
     }
+
+    return 0;
 }
 
 static void test_job_complete(Job *job, Error **errp)
diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
index 307726d089..1128490c19 100644
--- a/tests/test-blockjob-txn.c
+++ b/tests/test-blockjob-txn.c
@@ -32,24 +32,23 @@ static void test_block_job_exit(Job *job)
     bdrv_unref(bs);
 }
 
-static void coroutine_fn test_block_job_run(void *opaque)
+static int coroutine_fn test_block_job_run(Job *job)
 {
-    TestBlockJob *s = opaque;
-    BlockJob *job = &s->common;
+    TestBlockJob *s = container_of(job, TestBlockJob, common.job);
 
     while (s->iterations--) {
         if (s->use_timer) {
-            job_sleep_ns(&job->job, 0);
+            job_sleep_ns(job, 0);
         } else {
-            job_yield(&job->job);
+            job_yield(job);
         }
 
-        if (job_is_cancelled(&job->job)) {
+        if (job_is_cancelled(job)) {
             break;
         }
     }
 
-    s->common.job.ret = s->rc;
+    return s->rc;
 }
 
 typedef struct {
diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
index 20563bde4f..7f6df29bcf 100644
--- a/tests/test-blockjob.c
+++ b/tests/test-blockjob.c
@@ -175,13 +175,13 @@ static void cancel_job_complete(Job *job, Error **errp)
     s->should_complete = true;
 }
 
-static void coroutine_fn cancel_job_start(void *opaque)
+static int coroutine_fn cancel_job_start(Job *job)
 {
-    CancelJob *s = opaque;
+    CancelJob *s = container_of(job, CancelJob, common.job);
 
     while (!s->should_complete) {
         if (job_is_cancelled(&s->common.job)) {
-            return;
+            goto out;
         }
 
         if (!job_is_ready(&s->common.job) && s->should_converge) {
@@ -190,6 +190,9 @@ static void coroutine_fn cancel_job_start(void *opaque)
 
         job_sleep_ns(&s->common.job, 100000);
     }
+
+ out:
+    return 0;
 }
 
 static const BlockJobDriver test_cancel_driver = {
-- 
2.14.4

[Qemu-devel] [PATCH 13/21] block/commit: add block job creation flags

[John Snow]

Add support for taking and passing forward job creation flags.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/commit.c            | 5 +++--
 blockdev.c                | 7 ++++---
 include/block/block_int.h | 5 ++++-
 3 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index 22712cac2d..1a4a56795f 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -256,7 +256,8 @@ static BlockDriver bdrv_commit_top = {
 };
 
 void commit_start(const char *job_id, BlockDriverState *bs,
-                  BlockDriverState *base, BlockDriverState *top, int64_t speed,
+                  BlockDriverState *base, BlockDriverState *top,
+                  int creation_flags, int64_t speed,
                   BlockdevOnError on_error, const char *backing_file_str,
                   const char *filter_node_name, Error **errp)
 {
@@ -274,7 +275,7 @@ void commit_start(const char *job_id, BlockDriverState *bs,
     }
 
     s = block_job_create(job_id, &commit_job_driver, NULL, bs, 0, BLK_PERM_ALL,
-                         speed, JOB_DEFAULT, NULL, NULL, errp);
+                         speed, creation_flags, NULL, NULL, errp);
     if (!s) {
         return;
     }
diff --git a/blockdev.c b/blockdev.c
index dcf8c8d2ab..88ad8d96e5 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3324,6 +3324,7 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
      * BlockdevOnError change for blkmirror makes it in
      */
     BlockdevOnError on_error = BLOCKDEV_ON_ERROR_REPORT;
+    int job_flags = JOB_DEFAULT;
 
     if (!has_speed) {
         speed = 0;
@@ -3405,15 +3406,15 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
             goto out;
         }
         commit_active_start(has_job_id ? job_id : NULL, bs, base_bs,
-                            JOB_DEFAULT, speed, on_error,
+                            job_flags, speed, on_error,
                             filter_node_name, NULL, NULL, false, &local_err);
     } else {
         BlockDriverState *overlay_bs = bdrv_find_overlay(bs, top_bs);
         if (bdrv_op_is_blocked(overlay_bs, BLOCK_OP_TYPE_COMMIT_TARGET, errp)) {
             goto out;
         }
-        commit_start(has_job_id ? job_id : NULL, bs, base_bs, top_bs, speed,
-                     on_error, has_backing_file ? backing_file : NULL,
+        commit_start(has_job_id ? job_id : NULL, bs, base_bs, top_bs, job_flags,
+                     speed, on_error, has_backing_file ? backing_file : NULL,
                      filter_node_name, &local_err);
     }
     if (local_err != NULL) {
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 903b9c1034..ffab0b4d3e 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -980,6 +980,8 @@ void stream_start(const char *job_id, BlockDriverState *bs,
  * @bs: Active block device.
  * @top: Top block device to be committed.
  * @base: Block device that will be written into, and become the new top.
+ * @creation_flags: Flags that control the behavior of the Job lifetime.
+ *                  See @BlockJobCreateFlags
  * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
  * @on_error: The action to take upon error.
  * @backing_file_str: String to use as the backing file in @top's overlay
@@ -990,7 +992,8 @@ void stream_start(const char *job_id, BlockDriverState *bs,
  *
  */
 void commit_start(const char *job_id, BlockDriverState *bs,
-                  BlockDriverState *base, BlockDriverState *top, int64_t speed,
+                  BlockDriverState *base, BlockDriverState *top,
+                  int creation_flags, int64_t speed,
                   BlockdevOnError on_error, const char *backing_file_str,
                   const char *filter_node_name, Error **errp);
 /**
-- 
2.14.4

[Qemu-devel] [PATCH 14/21] block/mirror: add block job creation flags

[John Snow]

Add support for taking and passing forward job creaton flags.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/mirror.c            | 5 +++--
 blockdev.c                | 3 ++-
 include/block/block_int.h | 5 ++++-
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index acf874393e..639d66098a 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1639,7 +1639,8 @@ fail:
 
 void mirror_start(const char *job_id, BlockDriverState *bs,
                   BlockDriverState *target, const char *replaces,
-                  int64_t speed, uint32_t granularity, int64_t buf_size,
+                  int creation_flags, int64_t speed,
+                  uint32_t granularity, int64_t buf_size,
                   MirrorSyncMode mode, BlockMirrorBackingMode backing_mode,
                   BlockdevOnError on_source_error,
                   BlockdevOnError on_target_error,
@@ -1655,7 +1656,7 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
     }
     is_none_mode = mode == MIRROR_SYNC_MODE_NONE;
     base = mode == MIRROR_SYNC_MODE_TOP ? backing_bs(bs) : NULL;
-    mirror_start_job(job_id, bs, JOB_DEFAULT, target, replaces,
+    mirror_start_job(job_id, bs, creation_flags, target, replaces,
                      speed, granularity, buf_size, backing_mode,
                      on_source_error, on_target_error, unmap, NULL, NULL,
                      &mirror_job_driver, is_none_mode, base, false,
diff --git a/blockdev.c b/blockdev.c
index 88ad8d96e5..d31750b024 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3700,6 +3700,7 @@ static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
                                    bool has_copy_mode, MirrorCopyMode copy_mode,
                                    Error **errp)
 {
+    int job_flags = JOB_DEFAULT;
 
     if (!has_speed) {
         speed = 0;
@@ -3752,7 +3753,7 @@ static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
      * and will allow to check whether the node still exist at mirror completion
      */
     mirror_start(job_id, bs, target,
-                 has_replaces ? replaces : NULL,
+                 has_replaces ? replaces : NULL, job_flags,
                  speed, granularity, buf_size, sync, backing_mode,
                  on_source_error, on_target_error, unmap, filter_node_name,
                  copy_mode, errp);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index ffab0b4d3e..b40f0bfc9b 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1029,6 +1029,8 @@ void commit_active_start(const char *job_id, BlockDriverState *bs,
  * @target: Block device to write to.
  * @replaces: Block graph node name to replace once the mirror is done. Can
  *            only be used when full mirroring is selected.
+ * @creation_flags: Flags that control the behavior of the Job lifetime.
+ *                  See @BlockJobCreateFlags
  * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
  * @granularity: The chosen granularity for the dirty bitmap.
  * @buf_size: The amount of data that can be in flight at one time.
@@ -1050,7 +1052,8 @@ void commit_active_start(const char *job_id, BlockDriverState *bs,
  */
 void mirror_start(const char *job_id, BlockDriverState *bs,
                   BlockDriverState *target, const char *replaces,
-                  int64_t speed, uint32_t granularity, int64_t buf_size,
+                  int creation_flags, int64_t speed,
+                  uint32_t granularity, int64_t buf_size,
                   MirrorSyncMode mode, BlockMirrorBackingMode backing_mode,
                   BlockdevOnError on_source_error,
                   BlockdevOnError on_target_error,
-- 
2.14.4

[Qemu-devel] [PATCH 15/21] block/stream: add block job creation flags

[John Snow]

Add support for taking and passing forward job creaton flags.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/stream.c            | 5 +++--
 blockdev.c                | 3 ++-
 include/block/block_int.h | 5 ++++-
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index 2bdbca779d..af0a1d5d0c 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -214,7 +214,8 @@ static const BlockJobDriver stream_job_driver = {
 
 void stream_start(const char *job_id, BlockDriverState *bs,
                   BlockDriverState *base, const char *backing_file_str,
-                  int64_t speed, BlockdevOnError on_error, Error **errp)
+                  int creation_flags, int64_t speed,
+                  BlockdevOnError on_error, Error **errp)
 {
     StreamBlockJob *s;
     BlockDriverState *iter;
@@ -236,7 +237,7 @@ void stream_start(const char *job_id, BlockDriverState *bs,
                          BLK_PERM_GRAPH_MOD,
                          BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED |
                          BLK_PERM_WRITE,
-                         speed, JOB_DEFAULT, NULL, NULL, errp);
+                         speed, creation_flags, NULL, NULL, errp);
     if (!s) {
         goto fail;
     }
diff --git a/blockdev.c b/blockdev.c
index d31750b024..c2e6402a66 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3233,6 +3233,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
     AioContext *aio_context;
     Error *local_err = NULL;
     const char *base_name = NULL;
+    int job_flags = JOB_DEFAULT;
 
     if (!has_on_error) {
         on_error = BLOCKDEV_ON_ERROR_REPORT;
@@ -3295,7 +3296,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
     base_name = has_backing_file ? backing_file : base_name;
 
     stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
-                 has_speed ? speed : 0, on_error, &local_err);
+                 job_flags, has_speed ? speed : 0, on_error, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         goto out;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index b40f0bfc9b..4000d2af45 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -958,6 +958,8 @@ int is_windows_drive(const char *filename);
  * flatten the whole backing file chain onto @bs.
  * @backing_file_str: The file name that will be written to @bs as the
  * the new backing file if the job completes. Ignored if @base is %NULL.
+ * @creation_flags: Flags that control the behavior of the Job lifetime.
+ *                  See @BlockJobCreateFlags
  * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
  * @on_error: The action to take upon error.
  * @errp: Error object.
@@ -971,7 +973,8 @@ int is_windows_drive(const char *filename);
  */
 void stream_start(const char *job_id, BlockDriverState *bs,
                   BlockDriverState *base, const char *backing_file_str,
-                  int64_t speed, BlockdevOnError on_error, Error **errp);
+                  int creation_flags, int64_t speed,
+                  BlockdevOnError on_error, Error **errp);
 
 /**
  * commit_start:
-- 
2.14.4

[Qemu-devel] [PATCH 16/21] block/commit: refactor commit to use job callbacks

[John Snow]

Use the component callbacks; prepare, commit, abort, and clean.

NB: prepare is only called when the job has not yet failed;
and abort can be called after prepare.

complete -> prepare -> abort -> clean
complete -> abort -> clean

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/commit.c | 94 +++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 61 insertions(+), 33 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index 1a4a56795f..6673a0544e 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -35,7 +35,9 @@ typedef struct CommitBlockJob {
     BlockJob common;
     BlockDriverState *commit_top_bs;
     BlockBackend *top;
+    BlockDriverState *top_bs;
     BlockBackend *base;
+    BlockDriverState *base_bs;
     BlockdevOnError on_error;
     int base_flags;
     char *backing_file_str;
@@ -71,37 +73,37 @@ static int coroutine_fn commit_populate(BlockBackend *bs, BlockBackend *base,
 static void commit_exit(Job *job)
 {
     CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
-    BlockJob *bjob = &s->common;
-    BlockDriverState *top = blk_bs(s->top);
-    BlockDriverState *base = blk_bs(s->base);
-    BlockDriverState *commit_top_bs = s->commit_top_bs;
-    int ret = job->ret;
-    bool remove_commit_top_bs = false;
+
+    s->top_bs = blk_bs(s->top);
+    s->base_bs = blk_bs(s->base);
 
     /* Make sure commit_top_bs and top stay around until bdrv_replace_node() */
-    bdrv_ref(top);
-    bdrv_ref(commit_top_bs);
+    bdrv_ref(s->top_bs);
+    bdrv_ref(s->commit_top_bs);
+}
 
-    /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before
-     * the normal backing chain can be restored. */
-    blk_unref(s->base);
+static int commit_prepare(Job *job)
+{
+    CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
 
-    if (!job_is_cancelled(job) && ret == 0) {
-        /* success */
-        ret = bdrv_drop_intermediate(s->commit_top_bs, base,
-                                     s->backing_file_str);
-    } else {
-        /* XXX Can (or should) we somehow keep 'consistent read' blocked even
-         * after the failed/cancelled commit job is gone? If we already wrote
-         * something to base, the intermediate images aren't valid any more. */
-        remove_commit_top_bs = true;
-    }
+     /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before
+      * the normal backing chain can be restored. */
+     blk_unref(s->base);
+     s->base = NULL;
+
+     return bdrv_drop_intermediate(s->commit_top_bs, s->base_bs,
+                                   s->backing_file_str);
+}
+
+static void commit_exit_common(Job *job)
+{
+    CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
 
     /* restore base open flags here if appropriate (e.g., change the base back
      * to r/o). These reopens do not need to be atomic, since we won't abort
      * even on failure here */
-    if (s->base_flags != bdrv_get_flags(base)) {
-        bdrv_reopen(base, s->base_flags, NULL);
+    if (s->base_flags != bdrv_get_flags(s->base_bs)) {
+        bdrv_reopen(s->base_bs, s->base_flags, NULL);
     }
     g_free(s->backing_file_str);
     blk_unref(s->top);
@@ -110,21 +112,43 @@ static void commit_exit(Job *job)
      * job_finish_sync()), job_completed() won't free it and therefore the
      * blockers on the intermediate nodes remain. This would cause
      * bdrv_set_backing_hd() to fail. */
-    block_job_remove_all_bdrv(bjob);
+    block_job_remove_all_bdrv(&s->common);
+}
+
+static void commit_commit(Job *job)
+{
+    commit_exit_common(job);
+}
+
+static void commit_abort(Job *job)
+{
+    CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
+
+    if (s->base) {
+        blk_unref(s->base);
+    }
+
+    commit_exit_common(job);
+
+    /* XXX Can (or should) we somehow keep 'consistent read' blocked even
+     * after the failed/cancelled commit job is gone? If we already wrote
+     * something to base, the intermediate images aren't valid any more. */
 
     /* If bdrv_drop_intermediate() didn't already do that, remove the commit
      * filter driver from the backing chain. Do this as the final step so that
      * the 'consistent read' permission can be granted.  */
-    if (remove_commit_top_bs) {
-        bdrv_child_try_set_perm(commit_top_bs->backing, 0, BLK_PERM_ALL,
-                                &error_abort);
-        bdrv_replace_node(commit_top_bs, backing_bs(commit_top_bs),
-                          &error_abort);
-    }
+    bdrv_child_try_set_perm(s->commit_top_bs->backing, 0, BLK_PERM_ALL,
+                            &error_abort);
+    bdrv_replace_node(s->commit_top_bs, backing_bs(s->commit_top_bs),
+                      &error_abort);
+}
 
-    bdrv_unref(commit_top_bs);
-    bdrv_unref(top);
-    job->ret = ret;
+static void commit_clean(Job *job)
+{
+    CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
+
+    bdrv_unref(s->commit_top_bs);
+    bdrv_unref(s->top_bs);
 }
 
 static int coroutine_fn commit_run(Job *job)
@@ -214,6 +238,10 @@ static const BlockJobDriver commit_job_driver = {
         .drain         = block_job_drain,
         .start         = commit_run,
         .exit          = commit_exit,
+        .prepare       = commit_prepare,
+        .commit        = commit_commit,
+        .abort         = commit_abort,
+        .clean         = commit_clean
     },
 };
 
-- 
2.14.4

[Qemu-devel] [PATCH 17/21] block/mirror: conservative mirror_exit refactor

[John Snow]

For purposes of minimum code movement, refactor the mirror_exit
callback to use the post-finalization callbacks in a trivial way.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/mirror.c | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 639d66098a..914de0b7a0 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -79,6 +79,7 @@ typedef struct MirrorBlockJob {
     int max_iov;
     bool initial_zeroing_ongoing;
     int in_active_write_counter;
+    bool prepared;
 } MirrorBlockJob;
 
 typedef struct MirrorBDSOpaque {
@@ -607,7 +608,7 @@ static void mirror_wait_for_all_io(MirrorBlockJob *s)
     }
 }
 
-static void mirror_exit(Job *job)
+static int mirror_exit_common(Job *job)
 {
     MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
     BlockJob *bjob = &s->common;
@@ -619,6 +620,11 @@ static void mirror_exit(Job *job)
     Error *local_err = NULL;
     int ret = job->ret;
 
+    if (s->prepared) {
+        return ret;
+    }
+    s->prepared = true;
+
     bdrv_release_dirty_bitmap(src, s->dirty_bitmap);
 
     /* Make sure that the source BDS doesn't go away before we called
@@ -711,7 +717,17 @@ static void mirror_exit(Job *job)
     bdrv_unref(mirror_top_bs);
     bdrv_unref(src);
 
-    job->ret = ret;
+    return ret;
+}
+
+static int mirror_prepare(Job *job)
+{
+    return mirror_exit_common(job);
+}
+
+static void mirror_abort(Job *job)
+{
+    mirror_exit_common(job);
 }
 
 static void mirror_throttle(MirrorBlockJob *s)
@@ -1132,7 +1148,8 @@ static const BlockJobDriver mirror_job_driver = {
         .user_resume            = block_job_user_resume,
         .drain                  = block_job_drain,
         .start                  = mirror_run,
-        .exit                   = mirror_exit,
+        .prepare                = mirror_prepare,
+        .abort                  = mirror_abort,
         .pause                  = mirror_pause,
         .complete               = mirror_complete,
     },
@@ -1149,7 +1166,8 @@ static const BlockJobDriver commit_active_job_driver = {
         .user_resume            = block_job_user_resume,
         .drain                  = block_job_drain,
         .start                  = mirror_run,
-        .exit                   = mirror_exit,
+        .prepare                = mirror_prepare,
+        .abort                  = mirror_abort,
         .pause                  = mirror_pause,
         .complete               = mirror_complete,
     },
-- 
2.14.4

[Qemu-devel] [PATCH 18/21] block/commit: refactor stream to use job callbacks

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/stream.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index af0a1d5d0c..20192fac77 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -54,16 +54,16 @@ static int coroutine_fn stream_populate(BlockBackend *blk,
     return blk_co_preadv(blk, offset, qiov.size, &qiov, BDRV_REQ_COPY_ON_READ);
 }
 
-static void stream_exit(Job *job)
+static int stream_prepare(Job *job)
 {
     StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
     BlockJob *bjob = &s->common;
     BlockDriverState *bs = blk_bs(bjob->blk);
     BlockDriverState *base = s->base;
     Error *local_err = NULL;
-    int ret = job->ret;
+    int ret = 0;
 
-    if (!job_is_cancelled(job) && bs->backing && ret == 0) {
+    if (bs->backing) {
         const char *base_id = NULL, *base_fmt = NULL;
         if (base) {
             base_id = s->backing_file_str;
@@ -75,12 +75,19 @@ static void stream_exit(Job *job)
         bdrv_set_backing_hd(bs, base, &local_err);
         if (local_err) {
             error_report_err(local_err);
-            ret = -EPERM;
-            goto out;
+            return -EPERM;
         }
     }
 
-out:
+    return ret;
+}
+
+static void stream_clean(Job *job)
+{
+    StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
+    BlockJob *bjob = &s->common;
+    BlockDriverState *bs = blk_bs(bjob->blk);
+
     /* Reopen the image back in read-only mode if necessary */
     if (s->bs_flags != bdrv_get_flags(bs)) {
         /* Give up write permissions before making it read-only */
@@ -89,7 +96,6 @@ out:
     }
 
     g_free(s->backing_file_str);
-    job->ret = ret;
 }
 
 static int coroutine_fn stream_run(Job *job)
@@ -206,7 +212,8 @@ static const BlockJobDriver stream_job_driver = {
         .job_type      = JOB_TYPE_STREAM,
         .free          = block_job_free,
         .start         = stream_run,
-        .exit          = stream_exit,
+        .prepare       = stream_prepare,
+        .clean         = stream_clean,
         .user_resume   = block_job_user_resume,
         .drain         = block_job_drain,
     },
-- 
2.14.4

[Qemu-devel] [PATCH 19/21] qapi/block-commit: expose new job properties

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 blockdev.c           | 8 ++++++++
 qapi/block-core.json | 3 ++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index c2e6402a66..8efc47e178 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3314,6 +3314,8 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
                       bool has_backing_file, const char *backing_file,
                       bool has_speed, int64_t speed,
                       bool has_filter_node_name, const char *filter_node_name,
+                      bool has_auto_finalize, bool auto_finalize,
+                      bool has_auto_dismiss, bool auto_dismiss,
                       Error **errp)
 {
     BlockDriverState *bs;
@@ -3333,6 +3335,12 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
     if (!has_filter_node_name) {
         filter_node_name = NULL;
     }
+    if (has_auto_finalize && !auto_finalize) {
+        job_flags |= JOB_MANUAL_FINALIZE;
+    }
+    if (has_auto_dismiss && !auto_dismiss) {
+        job_flags |= JOB_MANUAL_DISMISS;
+    }
 
     /* Important Note:
      *  libvirt relies on the DeviceNotFound error class in order to probe for
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 5b9084a394..4ee58cfb28 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1518,7 +1518,8 @@
 { 'command': 'block-commit',
   'data': { '*job-id': 'str', 'device': 'str', '*base': 'str', '*top': 'str',
             '*backing-file': 'str', '*speed': 'int',
-            '*filter-node-name': 'str' } }
+            '*filter-node-name': 'str',
+            '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
 
 ##
 # @drive-backup:
-- 
2.14.4

[Qemu-devel] [PATCH 20/21] qapi/block-mirror: expose new job properties

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 blockdev.c           | 14 ++++++++++++++
 qapi/block-core.json |  6 ++++--
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 8efc47e178..bbb3279020 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3707,6 +3707,8 @@ static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
                                    bool has_filter_node_name,
                                    const char *filter_node_name,
                                    bool has_copy_mode, MirrorCopyMode copy_mode,
+                                   bool has_auto_finalize, bool auto_finalize,
+                                   bool has_auto_dismiss, bool auto_dismiss,
                                    Error **errp)
 {
     int job_flags = JOB_DEFAULT;
@@ -3735,6 +3737,12 @@ static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
     if (!has_copy_mode) {
         copy_mode = MIRROR_COPY_MODE_BACKGROUND;
     }
+    if (has_auto_finalize && !auto_finalize) {
+        job_flags |= JOB_MANUAL_FINALIZE;
+    }
+    if (has_auto_dismiss && !auto_dismiss) {
+        job_flags |= JOB_MANUAL_DISMISS;
+    }
 
     if (granularity != 0 && (granularity < 512 || granularity > 1048576 * 64)) {
         error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "granularity",
@@ -3912,6 +3920,8 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
                            arg->has_unmap, arg->unmap,
                            false, NULL,
                            arg->has_copy_mode, arg->copy_mode,
+                           arg->has_auto_finalize, arg->auto_finalize,
+                           arg->has_auto_dismiss, arg->auto_dismiss,
                            &local_err);
     bdrv_unref(target_bs);
     error_propagate(errp, local_err);
@@ -3933,6 +3943,8 @@ void qmp_blockdev_mirror(bool has_job_id, const char *job_id,
                          bool has_filter_node_name,
                          const char *filter_node_name,
                          bool has_copy_mode, MirrorCopyMode copy_mode,
+                         bool has_auto_finalize, bool auto_finalize,
+                         bool has_auto_dismiss, bool auto_dismiss,
                          Error **errp)
 {
     BlockDriverState *bs;
@@ -3966,6 +3978,8 @@ void qmp_blockdev_mirror(bool has_job_id, const char *job_id,
                            true, true,
                            has_filter_node_name, filter_node_name,
                            has_copy_mode, copy_mode,
+                           has_auto_finalize, auto_finalize,
+                           has_auto_dismiss, auto_dismiss,
                            &local_err);
     error_propagate(errp, local_err);
 
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 4ee58cfb28..bc53eaf2c0 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1728,7 +1728,8 @@
             '*speed': 'int', '*granularity': 'uint32',
             '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
             '*on-target-error': 'BlockdevOnError',
-            '*unmap': 'bool', '*copy-mode': 'MirrorCopyMode' } }
+            '*unmap': 'bool', '*copy-mode': 'MirrorCopyMode',
+            '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
 
 ##
 # @BlockDirtyBitmap:
@@ -2015,7 +2016,8 @@
             '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
             '*on-target-error': 'BlockdevOnError',
             '*filter-node-name': 'str',
-            '*copy-mode': 'MirrorCopyMode' } }
+            '*copy-mode': 'MirrorCopyMode',
+            '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
 
 ##
 # @block_set_io_throttle:
-- 
2.14.4

[Qemu-devel] [PATCH 21/21] qapi/block-stream: expose new job properties

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 blockdev.c           | 9 +++++++++
 hmp.c                | 5 +++--
 qapi/block-core.json | 3 ++-
 3 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index bbb3279020..806531dc20 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3226,6 +3226,8 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
                       bool has_backing_file, const char *backing_file,
                       bool has_speed, int64_t speed,
                       bool has_on_error, BlockdevOnError on_error,
+                      bool has_auto_finalize, bool auto_finalize,
+                      bool has_auto_dismiss, bool auto_dismiss,
                       Error **errp)
 {
     BlockDriverState *bs, *iter;
@@ -3295,6 +3297,13 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
     /* backing_file string overrides base bs filename */
     base_name = has_backing_file ? backing_file : base_name;
 
+    if (has_auto_finalize && !auto_finalize) {
+        job_flags |= JOB_MANUAL_FINALIZE;
+    }
+    if (has_auto_dismiss && !auto_dismiss) {
+        job_flags |= JOB_MANUAL_DISMISS;
+    }
+
     stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
                  job_flags, has_speed ? speed : 0, on_error, &local_err);
     if (local_err) {
diff --git a/hmp.c b/hmp.c
index 2aafb50e8e..e3c3ecd76f 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1865,8 +1865,9 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
     int64_t speed = qdict_get_try_int(qdict, "speed", 0);
 
     qmp_block_stream(true, device, device, base != NULL, base, false, NULL,
-                     false, NULL, qdict_haskey(qdict, "speed"), speed,
-                     true, BLOCKDEV_ON_ERROR_REPORT, &error);
+                     false, NULL, qdict_haskey(qdict, "speed"), speed, true,
+                     BLOCKDEV_ON_ERROR_REPORT, false, false, false, false,
+                     &error);
 
     hmp_handle_error(mon, &error);
 }
diff --git a/qapi/block-core.json b/qapi/block-core.json
index bc53eaf2c0..9d06509d0d 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2298,7 +2298,8 @@
 { 'command': 'block-stream',
   'data': { '*job-id': 'str', 'device': 'str', '*base': 'str',
             '*base-node': 'str', '*backing-file': 'str', '*speed': 'int',
-            '*on-error': 'BlockdevOnError' } }
+            '*on-error': 'BlockdevOnError',
+            '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
 
 ##
 # @block-job-set-speed:
-- 
2.14.4

Re: [Qemu-devel] [PATCH 19/21] qapi/block-commit: expose new job properties

[Eric Blake]

On 08/06/2018 11:33 PM, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   blockdev.c           | 8 ++++++++
>   qapi/block-core.json | 3 ++-
>   2 files changed, 10 insertions(+), 1 deletion(-)
> 

> +++ b/qapi/block-core.json
> @@ -1518,7 +1518,8 @@
>   { 'command': 'block-commit',
>     'data': { '*job-id': 'str', 'device': 'str', '*base': 'str', '*top': 'str',
>               '*backing-file': 'str', '*speed': 'int',
> -            '*filter-node-name': 'str' } }
> +            '*filter-node-name': 'str',
> +            '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }

Missing documentation, including a 'since 3.1' tag.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 19/21] qapi/block-commit: expose new job properties

[John Snow]

On 08/07/2018 10:52 AM, Eric Blake wrote:
> On 08/06/2018 11:33 PM, John Snow wrote:
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   blockdev.c           | 8 ++++++++
>>   qapi/block-core.json | 3 ++-
>>   2 files changed, 10 insertions(+), 1 deletion(-)
>>
> 
>> +++ b/qapi/block-core.json
>> @@ -1518,7 +1518,8 @@
>>   { 'command': 'block-commit',
>>     'data': { '*job-id': 'str', 'device': 'str', '*base': 'str',
>> '*top': 'str',
>>               '*backing-file': 'str', '*speed': 'int',
>> -            '*filter-node-name': 'str' } }
>> +            '*filter-node-name': 'str',
>> +            '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
> 
> Missing documentation, including a 'since 3.1' tag.
> 

OK, will fix for V2 but I won't respin until I get more feedback.

--js

Re: [Qemu-devel] [PATCH 04/21] block/commit: utilize job_exit shim

[Jeff Cody]

On Tue, Aug 07, 2018 at 12:33:32AM -0400, John Snow wrote:
> Change the manual deferment to commit_complete into the implicit
> callback to job_exit.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/commit.c | 19 +++++--------------
>  1 file changed, 5 insertions(+), 14 deletions(-)
> 
> diff --git a/block/commit.c b/block/commit.c
> index 620666161b..5bed098d5f 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -68,19 +68,14 @@ static int coroutine_fn commit_populate(BlockBackend *bs, BlockBackend *base,
>      return 0;
>  }
>  
> -typedef struct {
> -    int ret;
> -} CommitCompleteData;
> -
> -static void commit_complete(Job *job, void *opaque)
> +static void commit_exit(Job *job)
>  {
>      CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
>      BlockJob *bjob = &s->common;
> -    CommitCompleteData *data = opaque;
>      BlockDriverState *top = blk_bs(s->top);
>      BlockDriverState *base = blk_bs(s->base);
>      BlockDriverState *commit_top_bs = s->commit_top_bs;
> -    int ret = data->ret;
> +    int ret = job->ret;
>      bool remove_commit_top_bs = false;
>  
>      /* Make sure commit_top_bs and top stay around until bdrv_replace_node() */

Since 'ret' is only assigned to 'job->ret' outside of this line, we could
drop the local 'ret' and assign job->ret to the output of
brdrv_drop_intermediate() in this code below:

    if (!job_is_cancelled(job) && ret == 0) {
        /* success */
        ret = bdrv_drop_intermediate(s->commit_top_bs, base,
                                     s->backing_file_str);
    }

> @@ -117,9 +112,6 @@ static void commit_complete(Job *job, void *opaque)
>       * bdrv_set_backing_hd() to fail. */
>      block_job_remove_all_bdrv(bjob);
>  
> -    job_completed(job, ret);
> -    g_free(data);
> -
>      /* If bdrv_drop_intermediate() didn't already do that, remove the commit
>       * filter driver from the backing chain. Do this as the final step so that
>       * the 'consistent read' permission can be granted.  */
> @@ -132,12 +124,12 @@ static void commit_complete(Job *job, void *opaque)
>  
>      bdrv_unref(commit_top_bs);
>      bdrv_unref(top);
> +    job->ret = ret;

We could then drop this line as well.

(This may fall under bike shedding, so I don't feel strongly about it):

Reviewed-by: Jeff Cody <jcody@redhat.com>

>  }
>  
>  static void coroutine_fn commit_run(void *opaque)
>  {
>      CommitBlockJob *s = opaque;
> -    CommitCompleteData *data;
>      int64_t offset;
>      uint64_t delay_ns = 0;
>      int ret = 0;
> @@ -210,9 +202,7 @@ static void coroutine_fn commit_run(void *opaque)
>  out:
>      qemu_vfree(buf);
>  
> -    data = g_malloc(sizeof(*data));
> -    data->ret = ret;
> -    job_defer_to_main_loop(&s->common.job, commit_complete, data);
> +    s->common.job.ret = ret;
>  }
>  
>  static const BlockJobDriver commit_job_driver = {
> @@ -223,6 +213,7 @@ static const BlockJobDriver commit_job_driver = {
>          .user_resume   = block_job_user_resume,
>          .drain         = block_job_drain,
>          .start         = commit_run,
> +        .exit          = commit_exit,
>      },
>  };
>  
> -- 
> 2.14.4
> 

Re: [Qemu-devel] [PATCH 05/21] block/mirror: utilize job_exit shim

[Jeff Cody]

On Tue, Aug 07, 2018 at 12:33:33AM -0400, John Snow wrote:
> Change the manual deferment to mirror_exit into the implicit
> callback to job_exit and the mirror_exit callback.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Jeff Cody <jcody@redhat.com>

> ---
>  block/mirror.c | 27 +++++++++++----------------
>  1 file changed, 11 insertions(+), 16 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 7c2c6ba67e..e3404e2518 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -607,21 +607,17 @@ static void mirror_wait_for_all_io(MirrorBlockJob *s)
>      }
>  }
>  
> -typedef struct {
> -    int ret;
> -} MirrorExitData;
> -
> -static void mirror_exit(Job *job, void *opaque)
> +static void mirror_exit(Job *job)
>  {
>      MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
>      BlockJob *bjob = &s->common;
> -    MirrorExitData *data = opaque;
>      MirrorBDSOpaque *bs_opaque = s->mirror_top_bs->opaque;
>      AioContext *replace_aio_context = NULL;
>      BlockDriverState *src = s->mirror_top_bs->backing->bs;
>      BlockDriverState *target_bs = blk_bs(s->target);
>      BlockDriverState *mirror_top_bs = s->mirror_top_bs;
>      Error *local_err = NULL;
> +    int ret = job->ret;
>  
>      bdrv_release_dirty_bitmap(src, s->dirty_bitmap);
>  
> @@ -652,7 +648,7 @@ static void mirror_exit(Job *job, void *opaque)
>              bdrv_set_backing_hd(target_bs, backing, &local_err);
>              if (local_err) {
>                  error_report_err(local_err);
> -                data->ret = -EPERM;
> +                ret = -EPERM;
>              }
>          }
>      }
> @@ -662,7 +658,7 @@ static void mirror_exit(Job *job, void *opaque)
>          aio_context_acquire(replace_aio_context);
>      }
>  
> -    if (s->should_complete && data->ret == 0) {
> +    if (s->should_complete && ret == 0) {
>          BlockDriverState *to_replace = src;
>          if (s->to_replace) {
>              to_replace = s->to_replace;
> @@ -679,7 +675,7 @@ static void mirror_exit(Job *job, void *opaque)
>          bdrv_drained_end(target_bs);
>          if (local_err) {
>              error_report_err(local_err);
> -            data->ret = -EPERM;
> +            ret = -EPERM;
>          }
>      }
>      if (s->to_replace) {
> @@ -710,12 +706,12 @@ static void mirror_exit(Job *job, void *opaque)
>      blk_insert_bs(bjob->blk, mirror_top_bs, &error_abort);
>  
>      bs_opaque->job = NULL;
> -    job_completed(job, data->ret);
>  
> -    g_free(data);
>      bdrv_drained_end(src);
>      bdrv_unref(mirror_top_bs);
>      bdrv_unref(src);
> +
> +    job->ret = ret;
>  }
>  
>  static void mirror_throttle(MirrorBlockJob *s)
> @@ -815,7 +811,6 @@ static int mirror_flush(MirrorBlockJob *s)
>  static void coroutine_fn mirror_run(void *opaque)
>  {
>      MirrorBlockJob *s = opaque;
> -    MirrorExitData *data;
>      BlockDriverState *bs = s->mirror_top_bs->backing->bs;
>      BlockDriverState *target_bs = blk_bs(s->target);
>      bool need_drain = true;
> @@ -1035,13 +1030,11 @@ immediate_exit:
>      g_free(s->in_flight_bitmap);
>      bdrv_dirty_iter_free(s->dbi);
>  
> -    data = g_malloc(sizeof(*data));
> -    data->ret = ret;
> -
>      if (need_drain) {
>          bdrv_drained_begin(bs);
>      }
> -    job_defer_to_main_loop(&s->common.job, mirror_exit, data);
> +
> +    s->common.job.ret = ret;
>  }
>  
>  static void mirror_complete(Job *job, Error **errp)
> @@ -1139,6 +1132,7 @@ static const BlockJobDriver mirror_job_driver = {
>          .user_resume            = block_job_user_resume,
>          .drain                  = block_job_drain,
>          .start                  = mirror_run,
> +        .exit                   = mirror_exit,
>          .pause                  = mirror_pause,
>          .complete               = mirror_complete,
>      },
> @@ -1155,6 +1149,7 @@ static const BlockJobDriver commit_active_job_driver = {
>          .user_resume            = block_job_user_resume,
>          .drain                  = block_job_drain,
>          .start                  = mirror_run,
> +        .exit                   = mirror_exit,
>          .pause                  = mirror_pause,
>          .complete               = mirror_complete,
>      },
> -- 
> 2.14.4
> 

Re: [Qemu-devel] [PATCH 06/21] block/stream: utilize job_exit shim

[Jeff Cody]

On Tue, Aug 07, 2018 at 12:33:34AM -0400, John Snow wrote:
> Change the manual deferment to stream_complete into the implicit
> callback to job_exit.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Jeff Cody <jcody@redhat.com>

> ---
>  block/stream.c | 23 ++++++++---------------
>  1 file changed, 8 insertions(+), 15 deletions(-)
> 
> diff --git a/block/stream.c b/block/stream.c
> index a5d6e0cf8a..163cd6431c 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -54,20 +54,16 @@ static int coroutine_fn stream_populate(BlockBackend *blk,
>      return blk_co_preadv(blk, offset, qiov.size, &qiov, BDRV_REQ_COPY_ON_READ);
>  }
>  
> -typedef struct {
> -    int ret;
> -} StreamCompleteData;
> -
> -static void stream_complete(Job *job, void *opaque)
> +static void stream_exit(Job *job)
>  {
>      StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
>      BlockJob *bjob = &s->common;
> -    StreamCompleteData *data = opaque;
>      BlockDriverState *bs = blk_bs(bjob->blk);
>      BlockDriverState *base = s->base;
>      Error *local_err = NULL;
> +    int ret = job->ret;
>  
> -    if (!job_is_cancelled(job) && bs->backing && data->ret == 0) {
> +    if (!job_is_cancelled(job) && bs->backing && ret == 0) {
>          const char *base_id = NULL, *base_fmt = NULL;
>          if (base) {
>              base_id = s->backing_file_str;
> @@ -75,11 +71,11 @@ static void stream_complete(Job *job, void *opaque)
>                  base_fmt = base->drv->format_name;
>              }
>          }
> -        data->ret = bdrv_change_backing_file(bs, base_id, base_fmt);
> +        ret = bdrv_change_backing_file(bs, base_id, base_fmt);
>          bdrv_set_backing_hd(bs, base, &local_err);
>          if (local_err) {
>              error_report_err(local_err);
> -            data->ret = -EPERM;
> +            ret = -EPERM;
>              goto out;
>          }
>      }
> @@ -93,14 +89,12 @@ out:
>      }
>  
>      g_free(s->backing_file_str);
> -    job_completed(job, data->ret);
> -    g_free(data);
> +    job->ret = ret;
>  }
>  
>  static void coroutine_fn stream_run(void *opaque)
>  {
>      StreamBlockJob *s = opaque;
> -    StreamCompleteData *data;
>      BlockBackend *blk = s->common.blk;
>      BlockDriverState *bs = blk_bs(blk);
>      BlockDriverState *base = s->base;
> @@ -203,9 +197,7 @@ static void coroutine_fn stream_run(void *opaque)
>  
>  out:
>      /* Modify backing chain and close BDSes in main loop */
> -    data = g_malloc(sizeof(*data));
> -    data->ret = ret;
> -    job_defer_to_main_loop(&s->common.job, stream_complete, data);
> +    s->common.job.ret = ret;
>  }
>  
>  static const BlockJobDriver stream_job_driver = {
> @@ -214,6 +206,7 @@ static const BlockJobDriver stream_job_driver = {
>          .job_type      = JOB_TYPE_STREAM,
>          .free          = block_job_free,
>          .start         = stream_run,
> +        .exit          = stream_exit,
>          .user_resume   = block_job_user_resume,
>          .drain         = block_job_drain,
>      },
> -- 
> 2.14.4
> 

Re: [Qemu-devel] [PATCH 02/21] jobs: add exit shim

[Jeff Cody]

On Tue, Aug 07, 2018 at 12:33:30AM -0400, John Snow wrote:
> Most jobs do the same thing when they leave their running loop:
> - Store the return code in a structure
> - wait to receive this structure in the main thread
> - signal job completion via job_completed
> 
> More seriously, when we utilize job_defer_to_main_loop_bh to call
> a function that calls job_completed, job_finalize_single will run
> in a context where it has recursively taken the aio_context lock,
> which can cause hangs if it puts down a reference that causes a flush.
> 
> The job infrastructure is perfectly capable of registering job
> completion itself when we leave the job's entry point. In this
> context, we can signal job completion from outside of the aio_context,
> which should allow for job cleanup code to run with only one lock.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

I like the simplification, both in SLOC and in exit logic (as seen in
patches 3-7).

> ---
>  include/qemu/job.h |  7 +++++++
>  job.c              | 19 +++++++++++++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/include/qemu/job.h b/include/qemu/job.h
> index 845ad00c03..0c24e8704f 100644
> --- a/include/qemu/job.h
> +++ b/include/qemu/job.h
> @@ -204,6 +204,13 @@ struct JobDriver {
>       */
>      void (*drain)(Job *job);
>  
> +    /**
> +     * If the callback is not NULL, exit will be invoked from the main thread
> +     * when the job's coroutine has finished, but before transactional
> +     * convergence; before @prepare or @abort.
> +     */
> +    void (*exit)(Job *job);
> +
>      /**
>       * If the callback is not NULL, prepare will be invoked when all the jobs
>       * belonging to the same transaction complete; or upon this job's completion
> diff --git a/job.c b/job.c
> index b281f30375..cc5ac9ac30 100644
> --- a/job.c
> +++ b/job.c
> @@ -535,6 +535,19 @@ void job_drain(Job *job)
>      }
>  }
>  
> +static void job_exit(void *opaque)
> +{
> +    Job *job = (Job *)opaque;
> +    AioContext *aio_context = job->aio_context;
> +
> +    if (job->driver->exit) {
> +        aio_context_acquire(aio_context);
> +        job->driver->exit(job);
> +        aio_context_release(aio_context);
> +    }
> +    job_completed(job, job->ret);
> +}
> +
>  /**
>   * All jobs must allow a pause point before entering their job proper. This
>   * ensures that jobs can be paused prior to being started, then resumed later.
> @@ -546,6 +559,12 @@ static void coroutine_fn job_co_entry(void *opaque)
>      assert(job && job->driver && job->driver->start);
>      job_pause_point(job);
>      job->driver->start(job);

One nit-picky observation here, that is unrelated to this patch: reading
through, it may not be so obvious that 'start' is really a 'run' or
'execute', (linguistically, to me 'start' implies a kick-off rather than
ongoing execution).

Just some bike-shedding again, though, and not even for this patch.  So
nothing to do here :)

Reviewed-by: Jeff Cody <jcody@redhat.com>


> +    if (!job->deferred_to_main_loop) {
> +        job->deferred_to_main_loop = true;
> +        aio_bh_schedule_oneshot(qemu_get_aio_context(),
> +                                job_exit,
> +                                job);
> +    }
>  }
>  
>  
> -- 
> 2.14.4
> 

Re: [Qemu-devel] [PATCH 02/21] jobs: add exit shim

[John Snow]

On 08/08/2018 12:02 AM, Jeff Cody wrote:
> On Tue, Aug 07, 2018 at 12:33:30AM -0400, John Snow wrote:
>> Most jobs do the same thing when they leave their running loop:
>> - Store the return code in a structure
>> - wait to receive this structure in the main thread
>> - signal job completion via job_completed
>>
>> More seriously, when we utilize job_defer_to_main_loop_bh to call
>> a function that calls job_completed, job_finalize_single will run
>> in a context where it has recursively taken the aio_context lock,
>> which can cause hangs if it puts down a reference that causes a flush.
>>
>> The job infrastructure is perfectly capable of registering job
>> completion itself when we leave the job's entry point. In this
>> context, we can signal job completion from outside of the aio_context,
>> which should allow for job cleanup code to run with only one lock.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
> 
> I like the simplification, both in SLOC and in exit logic (as seen in
> patches 3-7).
> 
>> ---
>>  include/qemu/job.h |  7 +++++++
>>  job.c              | 19 +++++++++++++++++++
>>  2 files changed, 26 insertions(+)
>>
>> diff --git a/include/qemu/job.h b/include/qemu/job.h
>> index 845ad00c03..0c24e8704f 100644
>> --- a/include/qemu/job.h
>> +++ b/include/qemu/job.h
>> @@ -204,6 +204,13 @@ struct JobDriver {
>>       */
>>      void (*drain)(Job *job);
>>  
>> +    /**
>> +     * If the callback is not NULL, exit will be invoked from the main thread
>> +     * when the job's coroutine has finished, but before transactional
>> +     * convergence; before @prepare or @abort.
>> +     */
>> +    void (*exit)(Job *job);
>> +
>>      /**
>>       * If the callback is not NULL, prepare will be invoked when all the jobs
>>       * belonging to the same transaction complete; or upon this job's completion
>> diff --git a/job.c b/job.c
>> index b281f30375..cc5ac9ac30 100644
>> --- a/job.c
>> +++ b/job.c
>> @@ -535,6 +535,19 @@ void job_drain(Job *job)
>>      }
>>  }
>>  
>> +static void job_exit(void *opaque)
>> +{
>> +    Job *job = (Job *)opaque;
>> +    AioContext *aio_context = job->aio_context;
>> +
>> +    if (job->driver->exit) {
>> +        aio_context_acquire(aio_context);
>> +        job->driver->exit(job);
>> +        aio_context_release(aio_context);
>> +    }
>> +    job_completed(job, job->ret);
>> +}
>> +
>>  /**
>>   * All jobs must allow a pause point before entering their job proper. This
>>   * ensures that jobs can be paused prior to being started, then resumed later.
>> @@ -546,6 +559,12 @@ static void coroutine_fn job_co_entry(void *opaque)
>>      assert(job && job->driver && job->driver->start);
>>      job_pause_point(job);
>>      job->driver->start(job);
> 
> One nit-picky observation here, that is unrelated to this patch: reading
> through, it may not be so obvious that 'start' is really a 'run' or
> 'execute', (linguistically, to me 'start' implies a kick-off rather than
> ongoing execution).
> 
> Just some bike-shedding again, though, and not even for this patch.  So
> nothing to do here :)
> 
> Reviewed-by: Jeff Cody <jcody@redhat.com>

I agree with you and thought the same as I was going through these
changes. I think with patch 12 in particular where I really solidify the
idea that this is the main execution loop for the coroutine makes it
obvious that it should be named .run or similar.

Also, while we're bikeshedding naming, it's weird that the flow here is:

.run/.start
.exit
.prepare
.commit
.abort
.clean

...I might want to emphasize a few points here:

(A) exit occurs prior to "finalization" and should not modify the graph
(B) "prepare" really means "prepare to finalize"

I just don't always have good terminology.

> 
> 
>> +    if (!job->deferred_to_main_loop) {
>> +        job->deferred_to_main_loop = true;
>> +        aio_bh_schedule_oneshot(qemu_get_aio_context(),
>> +                                job_exit,
>> +                                job);
>> +    }
>>  }
>>  
>>  
>> -- 
>> 2.14.4
>>
> 

Re: [Qemu-devel] [PATCH 04/21] block/commit: utilize job_exit shim

[John Snow]

On 08/07/2018 11:41 PM, Jeff Cody wrote:
> On Tue, Aug 07, 2018 at 12:33:32AM -0400, John Snow wrote:
>> Change the manual deferment to commit_complete into the implicit
>> callback to job_exit.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  block/commit.c | 19 +++++--------------
>>  1 file changed, 5 insertions(+), 14 deletions(-)
>>
>> diff --git a/block/commit.c b/block/commit.c
>> index 620666161b..5bed098d5f 100644
>> --- a/block/commit.c
>> +++ b/block/commit.c
>> @@ -68,19 +68,14 @@ static int coroutine_fn commit_populate(BlockBackend *bs, BlockBackend *base,
>>      return 0;
>>  }
>>  
>> -typedef struct {
>> -    int ret;
>> -} CommitCompleteData;
>> -
>> -static void commit_complete(Job *job, void *opaque)
>> +static void commit_exit(Job *job)
>>  {
>>      CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
>>      BlockJob *bjob = &s->common;
>> -    CommitCompleteData *data = opaque;
>>      BlockDriverState *top = blk_bs(s->top);
>>      BlockDriverState *base = blk_bs(s->base);
>>      BlockDriverState *commit_top_bs = s->commit_top_bs;
>> -    int ret = data->ret;
>> +    int ret = job->ret;
>>      bool remove_commit_top_bs = false;
>>  
>>      /* Make sure commit_top_bs and top stay around until bdrv_replace_node() */
> 
> Since 'ret' is only assigned to 'job->ret' outside of this line, we could
> drop the local 'ret' and assign job->ret to the output of
> brdrv_drop_intermediate() in this code below:
> 
>     if (!job_is_cancelled(job) && ret == 0) {
>         /* success */
>         ret = bdrv_drop_intermediate(s->commit_top_bs, base,
>                                      s->backing_file_str);
>     }
> 
>> @@ -117,9 +112,6 @@ static void commit_complete(Job *job, void *opaque)
>>       * bdrv_set_backing_hd() to fail. */
>>      block_job_remove_all_bdrv(bjob);
>>  
>> -    job_completed(job, ret);
>> -    g_free(data);
>> -
>>      /* If bdrv_drop_intermediate() didn't already do that, remove the commit
>>       * filter driver from the backing chain. Do this as the final step so that
>>       * the 'consistent read' permission can be granted.  */
>> @@ -132,12 +124,12 @@ static void commit_complete(Job *job, void *opaque)
>>  
>>      bdrv_unref(commit_top_bs);
>>      bdrv_unref(top);
>> +    job->ret = ret;
> 
> We could then drop this line as well.
> 
> (This may fall under bike shedding, so I don't feel strongly about it):

You're right; though most of it gets blown out of the water in #16
anyway, so it doesn't stay a problem for long.

--js

> 
> Reviewed-by: Jeff Cody <jcody@redhat.com>
> 

Thanks!

>>  }
>>  
>>  static void coroutine_fn commit_run(void *opaque)
>>  {
>>      CommitBlockJob *s = opaque;
>> -    CommitCompleteData *data;
>>      int64_t offset;
>>      uint64_t delay_ns = 0;
>>      int ret = 0;
>> @@ -210,9 +202,7 @@ static void coroutine_fn commit_run(void *opaque)
>>  out:
>>      qemu_vfree(buf);
>>  
>> -    data = g_malloc(sizeof(*data));
>> -    data->ret = ret;
>> -    job_defer_to_main_loop(&s->common.job, commit_complete, data);
>> +    s->common.job.ret = ret;
>>  }
>>  
>>  static const BlockJobDriver commit_job_driver = {
>> @@ -223,6 +213,7 @@ static const BlockJobDriver commit_job_driver = {
>>          .user_resume   = block_job_user_resume,
>>          .drain         = block_job_drain,
>>          .start         = commit_run,
>> +        .exit          = commit_exit,
>>      },
>>  };
>>  
>> -- 
>> 2.14.4
>>

Re: [Qemu-devel] [PATCH 01/21] jobs: canonize Error object

[Kevin Wolf]

Am 07.08.2018 um 06:33 hat John Snow geschrieben:
> Jobs presently use both an Error object in the case of the create job,
> and char strings in the case of generic errors elsewhere.
> 
> Unify the two paths as just j->err, and remove the extra argument from
> job_completed.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Hm, not sure. Overall, this feels like a step backwards.

> diff --git a/include/qemu/job.h b/include/qemu/job.h
> index 18c9223e31..845ad00c03 100644
> --- a/include/qemu/job.h
> +++ b/include/qemu/job.h
> @@ -124,12 +124,12 @@ typedef struct Job {
>      /** Estimated progress_current value at the completion of the job */
>      int64_t progress_total;
>  
> -    /** Error string for a failed job (NULL if, and only if, job->ret == 0) */
> -    char *error;
> -
>      /** ret code passed to job_completed. */
>      int ret;
>  
> +    /** Error object for a failed job **/
> +    Error *err;
> +
>      /** The completion function that will be called when the job completes.  */
>      BlockCompletionFunc *cb;

This is the change that I could agree with, though I don't think it
makes a big difference: Whether you store the string immediately or an
Error object from which you get the string later, doesn't really make a
big difference.

Maybe we find more uses and having an Error object is common practice in
QEMU, so no objections to this change.

> @@ -484,15 +484,13 @@ void job_transition_to_ready(Job *job);
>  /**
>   * @job: The job being completed.
>   * @ret: The status code.
> - * @error: The error message for a failing job (only with @ret < 0). If @ret is
> - *         negative, but NULL is given for @error, strerror() is used.
>   *
>   * Marks @job as completed. If @ret is non-zero, the job transaction it is part
>   * of is aborted. If @ret is zero, the job moves into the WAITING state. If it
>   * is the last job to complete in its transaction, all jobs in the transaction
>   * move from WAITING to PENDING.
>   */
> -void job_completed(Job *job, int ret, Error *error);
> +void job_completed(Job *job, int ret);

I don't like this one, though.

Before this change, job_completed(..., NULL) was a clear sign that the
error message probably needed an improvement, because an errno string
doesn't usually describe error situations very well. We may not have a
much better message in some cases, but in most cases we just don't pass
one because an error message after job creation is still a quite new
thing in the QAPI schema.

What we should get rid of in the long term is the int ret, not the Error
*error. I suspect callers really just distinguish success/error without
actually looking at the error code.

With this change applied, what's your new conversion plan for making
sure that every failing caller of job_completed() has set job->error
first?

> @@ -666,8 +665,8 @@ static void job_update_rc(Job *job)
>          job->ret = -ECANCELED;
>      }
>      if (job->ret) {
> -        if (!job->error) {
> -            job->error = g_strdup(strerror(-job->ret));
> +        if (!job->err) {
> +            error_setg_errno(&job->err, -job->ret, "job failed");
>          }
>          job_state_transition(job, JOB_STATUS_ABORTING);
>      }

This hunk just makes the error message more verbose with a "job failed"
prefix that doesn't add information. If it's the error string for a job,
of course the job failed.

Kevin

Re: [Qemu-devel] [PATCH 02/21] jobs: add exit shim

[Kevin Wolf]

Am 08.08.2018 um 06:02 hat Jeff Cody geschrieben:
> On Tue, Aug 07, 2018 at 12:33:30AM -0400, John Snow wrote:
> > Most jobs do the same thing when they leave their running loop:
> > - Store the return code in a structure
> > - wait to receive this structure in the main thread
> > - signal job completion via job_completed
> > 
> > More seriously, when we utilize job_defer_to_main_loop_bh to call
> > a function that calls job_completed, job_finalize_single will run
> > in a context where it has recursively taken the aio_context lock,
> > which can cause hangs if it puts down a reference that causes a flush.
> > 
> > The job infrastructure is perfectly capable of registering job
> > completion itself when we leave the job's entry point. In this
> > context, we can signal job completion from outside of the aio_context,
> > which should allow for job cleanup code to run with only one lock.
> > 
> > Signed-off-by: John Snow <jsnow@redhat.com>
> 
> I like the simplification, both in SLOC and in exit logic (as seen in
> patches 3-7).

I agree, unifying this seems like a good idea.

Like in the first patch, I'm not convinced of the details, though.
Essentially, this is my objection regarding job->err extended to
job->ret: You rely on jobs setting job->ret and job->err, but the
interfaces don't really show this.

> > @@ -546,6 +559,12 @@ static void coroutine_fn job_co_entry(void *opaque)
> >      assert(job && job->driver && job->driver->start);
> >      job_pause_point(job);
> >      job->driver->start(job);
> 
> One nit-picky observation here, that is unrelated to this patch: reading
> through, it may not be so obvious that 'start' is really a 'run' or
> 'execute', (linguistically, to me 'start' implies a kick-off rather than
> ongoing execution).

I had exactly the same thought. My proposal is to change the existing...

    CoroutineEntry *start;

...which is just short for...

    void coroutine_fn start(void *opaque);

...into this one:

    int coroutine_fn run(void *opaque, Error **errp);

I see that at the end of the series, you actually introduced an int
return value already. I would have done that from the start, but as long
the final state makes sense, I won't insist.

But can we have the Error **errp addition, too? Pretty please?

Kevin

Re: [Qemu-devel] [PATCH 02/21] jobs: add exit shim

[John Snow]

On 08/08/2018 11:23 AM, Kevin Wolf wrote:
> Am 08.08.2018 um 06:02 hat Jeff Cody geschrieben:
>> On Tue, Aug 07, 2018 at 12:33:30AM -0400, John Snow wrote:
>>> Most jobs do the same thing when they leave their running loop:
>>> - Store the return code in a structure
>>> - wait to receive this structure in the main thread
>>> - signal job completion via job_completed
>>>
>>> More seriously, when we utilize job_defer_to_main_loop_bh to call
>>> a function that calls job_completed, job_finalize_single will run
>>> in a context where it has recursively taken the aio_context lock,
>>> which can cause hangs if it puts down a reference that causes a flush.
>>>
>>> The job infrastructure is perfectly capable of registering job
>>> completion itself when we leave the job's entry point. In this
>>> context, we can signal job completion from outside of the aio_context,
>>> which should allow for job cleanup code to run with only one lock.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>
>> I like the simplification, both in SLOC and in exit logic (as seen in
>> patches 3-7).
> 
> I agree, unifying this seems like a good idea.
> 
> Like in the first patch, I'm not convinced of the details, though.
> Essentially, this is my objection regarding job->err extended to
> job->ret: You rely on jobs setting job->ret and job->err, but the
> interfaces don't really show this.
> 
>>> @@ -546,6 +559,12 @@ static void coroutine_fn job_co_entry(void *opaque)
>>>      assert(job && job->driver && job->driver->start);
>>>      job_pause_point(job);
>>>      job->driver->start(job);
>>
>> One nit-picky observation here, that is unrelated to this patch: reading
>> through, it may not be so obvious that 'start' is really a 'run' or
>> 'execute', (linguistically, to me 'start' implies a kick-off rather than
>> ongoing execution).
> 
> I had exactly the same thought. My proposal is to change the existing...
> 
>     CoroutineEntry *start;
> 
> ...which is just short for...
> 
>     void coroutine_fn start(void *opaque);
> 
> ...into this one:
> 
>     int coroutine_fn run(void *opaque, Error **errp);
> 
> I see that at the end of the series, you actually introduced an int
> return value already. I would have done that from the start, but as long
> the final state makes sense, I won't insist.
> 
> But can we have the Error **errp addition, too? Pretty please?
> 
> Kevin
> 

I'm actually glad you want that addition, I was considering very
strongly adding it but I felt like I had made the series long enough
already and didn't want to change too much all at once.

The basic thought was just:

"It'd sure be nice to have a generic function entry point that looks
like it returns the same error information as our non-coroutine functions."

I can absolutely work that in, and break this series into two parts:

(1) Rework jobs infrastructure to use the new run signature, and
(2) Rework jobs to use the finalization callbacks.

Sound good?

--js

Re: [Qemu-devel] [PATCH 02/21] jobs: add exit shim

[Kevin Wolf]

Am 08.08.2018 um 17:38 hat John Snow geschrieben:
> On 08/08/2018 11:23 AM, Kevin Wolf wrote:
> > Am 08.08.2018 um 06:02 hat Jeff Cody geschrieben:
> >> On Tue, Aug 07, 2018 at 12:33:30AM -0400, John Snow wrote:
> >>> Most jobs do the same thing when they leave their running loop:
> >>> - Store the return code in a structure
> >>> - wait to receive this structure in the main thread
> >>> - signal job completion via job_completed
> >>>
> >>> More seriously, when we utilize job_defer_to_main_loop_bh to call
> >>> a function that calls job_completed, job_finalize_single will run
> >>> in a context where it has recursively taken the aio_context lock,
> >>> which can cause hangs if it puts down a reference that causes a flush.
> >>>
> >>> The job infrastructure is perfectly capable of registering job
> >>> completion itself when we leave the job's entry point. In this
> >>> context, we can signal job completion from outside of the aio_context,
> >>> which should allow for job cleanup code to run with only one lock.
> >>>
> >>> Signed-off-by: John Snow <jsnow@redhat.com>
> >>
> >> I like the simplification, both in SLOC and in exit logic (as seen in
> >> patches 3-7).
> > 
> > I agree, unifying this seems like a good idea.
> > 
> > Like in the first patch, I'm not convinced of the details, though.
> > Essentially, this is my objection regarding job->err extended to
> > job->ret: You rely on jobs setting job->ret and job->err, but the
> > interfaces don't really show this.
> > 
> >>> @@ -546,6 +559,12 @@ static void coroutine_fn job_co_entry(void *opaque)
> >>>      assert(job && job->driver && job->driver->start);
> >>>      job_pause_point(job);
> >>>      job->driver->start(job);
> >>
> >> One nit-picky observation here, that is unrelated to this patch: reading
> >> through, it may not be so obvious that 'start' is really a 'run' or
> >> 'execute', (linguistically, to me 'start' implies a kick-off rather than
> >> ongoing execution).
> > 
> > I had exactly the same thought. My proposal is to change the existing...
> > 
> >     CoroutineEntry *start;
> > 
> > ...which is just short for...
> > 
> >     void coroutine_fn start(void *opaque);
> > 
> > ...into this one:
> > 
> >     int coroutine_fn run(void *opaque, Error **errp);
> > 
> > I see that at the end of the series, you actually introduced an int
> > return value already. I would have done that from the start, but as long
> > the final state makes sense, I won't insist.
> > 
> > But can we have the Error **errp addition, too? Pretty please?
> > 
> > Kevin
> > 
> 
> I'm actually glad you want that addition, I was considering very
> strongly adding it but I felt like I had made the series long enough
> already and didn't want to change too much all at once.
> 
> The basic thought was just:
> 
> "It'd sure be nice to have a generic function entry point that looks
> like it returns the same error information as our non-coroutine functions."
> 
> I can absolutely work that in, and break this series into two parts:
> 
> (1) Rework jobs infrastructure to use the new run signature, and
> (2) Rework jobs to use the finalization callbacks.
> 
> Sound good?

I haven't looked at the rest of the series yet, but so far this sounds
good to me.

Kevin

Re: [Qemu-devel] [PATCH 01/21] jobs: canonize Error object

[John Snow]

On 08/08/2018 10:57 AM, Kevin Wolf wrote:
> Am 07.08.2018 um 06:33 hat John Snow geschrieben:
>> Jobs presently use both an Error object in the case of the create job,
>> and char strings in the case of generic errors elsewhere.
>>
>> Unify the two paths as just j->err, and remove the extra argument from
>> job_completed.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
> 
> Hm, not sure. Overall, this feels like a step backwards.
> 
>> diff --git a/include/qemu/job.h b/include/qemu/job.h
>> index 18c9223e31..845ad00c03 100644
>> --- a/include/qemu/job.h
>> +++ b/include/qemu/job.h
>> @@ -124,12 +124,12 @@ typedef struct Job {
>>      /** Estimated progress_current value at the completion of the job */
>>      int64_t progress_total;
>>  
>> -    /** Error string for a failed job (NULL if, and only if, job->ret == 0) */
>> -    char *error;
>> -
>>      /** ret code passed to job_completed. */
>>      int ret;
>>  
>> +    /** Error object for a failed job **/
>> +    Error *err;
>> +
>>      /** The completion function that will be called when the job completes.  */
>>      BlockCompletionFunc *cb;
> 
> This is the change that I could agree with, though I don't think it
> makes a big difference: Whether you store the string immediately or an
> Error object from which you get the string later, doesn't really make a
> big difference.
> 
> Maybe we find more uses and having an Error object is common practice in
> QEMU, so no objections to this change.
> 
>> @@ -484,15 +484,13 @@ void job_transition_to_ready(Job *job);
>>  /**
>>   * @job: The job being completed.
>>   * @ret: The status code.
>> - * @error: The error message for a failing job (only with @ret < 0). If @ret is
>> - *         negative, but NULL is given for @error, strerror() is used.
>>   *
>>   * Marks @job as completed. If @ret is non-zero, the job transaction it is part
>>   * of is aborted. If @ret is zero, the job moves into the WAITING state. If it
>>   * is the last job to complete in its transaction, all jobs in the transaction
>>   * move from WAITING to PENDING.
>>   */
>> -void job_completed(Job *job, int ret, Error *error);
>> +void job_completed(Job *job, int ret);
> 
> I don't like this one, though.
> 
> Before this change, job_completed(..., NULL) was a clear sign that the
> error message probably needed an improvement, because an errno string
> doesn't usually describe error situations very well. We may not have a
> much better message in some cases, but in most cases we just don't pass
> one because an error message after job creation is still a quite new
> thing in the QAPI schema.
> 
> What we should get rid of in the long term is the int ret, not the Error
> *error. I suspect callers really just distinguish success/error without
> actually looking at the error code.
> 
> With this change applied, what's your new conversion plan for making
> sure that every failing caller of job_completed() has set job->error
> first?
> 

Getting rid of job_completed and moving to our fairly ubiquitous "ret &
Error *" combo.

>> @@ -666,8 +665,8 @@ static void job_update_rc(Job *job)
>>          job->ret = -ECANCELED;
>>      }
>>      if (job->ret) {
>> -        if (!job->error) {
>> -            job->error = g_strdup(strerror(-job->ret));
>> +        if (!job->err) {
>> +            error_setg_errno(&job->err, -job->ret, "job failed");
>>          }
>>          job_state_transition(job, JOB_STATUS_ABORTING);
>>      }
> 
> This hunk just makes the error message more verbose with a "job failed"
> prefix that doesn't add information. If it's the error string for a job,
> of course the job failed.
> 
> Kevin
> 

Yeah, it's not a good prefix, but if I wanted to use the error object in
a general way across all jobs, I needed _some_ kind of prefix there...

Re: [Qemu-devel] [PATCH 01/21] jobs: canonize Error object

[Kevin Wolf]

Am 08.08.2018 um 17:50 hat John Snow geschrieben:
> 
> 
> On 08/08/2018 10:57 AM, Kevin Wolf wrote:
> > Am 07.08.2018 um 06:33 hat John Snow geschrieben:
> >> Jobs presently use both an Error object in the case of the create job,
> >> and char strings in the case of generic errors elsewhere.
> >>
> >> Unify the two paths as just j->err, and remove the extra argument from
> >> job_completed.
> >>
> >> Signed-off-by: John Snow <jsnow@redhat.com>
> > 
> > Hm, not sure. Overall, this feels like a step backwards.
> > 
> >> diff --git a/include/qemu/job.h b/include/qemu/job.h
> >> index 18c9223e31..845ad00c03 100644
> >> --- a/include/qemu/job.h
> >> +++ b/include/qemu/job.h
> >> @@ -124,12 +124,12 @@ typedef struct Job {
> >>      /** Estimated progress_current value at the completion of the job */
> >>      int64_t progress_total;
> >>  
> >> -    /** Error string for a failed job (NULL if, and only if, job->ret == 0) */
> >> -    char *error;
> >> -
> >>      /** ret code passed to job_completed. */
> >>      int ret;
> >>  
> >> +    /** Error object for a failed job **/
> >> +    Error *err;
> >> +
> >>      /** The completion function that will be called when the job completes.  */
> >>      BlockCompletionFunc *cb;
> > 
> > This is the change that I could agree with, though I don't think it
> > makes a big difference: Whether you store the string immediately or an
> > Error object from which you get the string later, doesn't really make a
> > big difference.
> > 
> > Maybe we find more uses and having an Error object is common practice in
> > QEMU, so no objections to this change.
> > 
> >> @@ -484,15 +484,13 @@ void job_transition_to_ready(Job *job);
> >>  /**
> >>   * @job: The job being completed.
> >>   * @ret: The status code.
> >> - * @error: The error message for a failing job (only with @ret < 0). If @ret is
> >> - *         negative, but NULL is given for @error, strerror() is used.
> >>   *
> >>   * Marks @job as completed. If @ret is non-zero, the job transaction it is part
> >>   * of is aborted. If @ret is zero, the job moves into the WAITING state. If it
> >>   * is the last job to complete in its transaction, all jobs in the transaction
> >>   * move from WAITING to PENDING.
> >>   */
> >> -void job_completed(Job *job, int ret, Error *error);
> >> +void job_completed(Job *job, int ret);
> > 
> > I don't like this one, though.
> > 
> > Before this change, job_completed(..., NULL) was a clear sign that the
> > error message probably needed an improvement, because an errno string
> > doesn't usually describe error situations very well. We may not have a
> > much better message in some cases, but in most cases we just don't pass
> > one because an error message after job creation is still a quite new
> > thing in the QAPI schema.
> > 
> > What we should get rid of in the long term is the int ret, not the Error
> > *error. I suspect callers really just distinguish success/error without
> > actually looking at the error code.
> > 
> > With this change applied, what's your new conversion plan for making
> > sure that every failing caller of job_completed() has set job->error
> > first?
> > 
> 
> Getting rid of job_completed and moving to our fairly ubiquitous "ret &
> Error *" combo.

Yup, with the context of the discussion for patch 2, if you make .start
(or .run) take an Error **errp, that sounds like a good plan to me.

> >> @@ -666,8 +665,8 @@ static void job_update_rc(Job *job)
> >>          job->ret = -ECANCELED;
> >>      }
> >>      if (job->ret) {
> >> -        if (!job->error) {
> >> -            job->error = g_strdup(strerror(-job->ret));
> >> +        if (!job->err) {
> >> +            error_setg_errno(&job->err, -job->ret, "job failed");
> >>          }
> >>          job_state_transition(job, JOB_STATUS_ABORTING);
> >>      }
> > 
> > This hunk just makes the error message more verbose with a "job failed"
> > prefix that doesn't add information. If it's the error string for a job,
> > of course the job failed.
> > 
> > Kevin
> > 
> 
> Yeah, it's not a good prefix, but if I wanted to use the error object in
> a general way across all jobs, I needed _some_ kind of prefix there...

Shouldn't this one work?

    error_setg(&job->err, strerror(-job->ret));

Kevin

Re: [Qemu-devel] [PATCH 04/21] block/commit: utilize job_exit shim

[Kevin Wolf]

Am 07.08.2018 um 06:33 hat John Snow geschrieben:
> Change the manual deferment to commit_complete into the implicit
> callback to job_exit.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

There is one tricky thing in this patch that the commit message could be
a bit more explicit about, which is moving job_completed() to a later
point.

This is the code that happens between the old call of job_completed()
and the new one:

    /* If bdrv_drop_intermediate() didn't already do that, remove the commit
     * filter driver from the backing chain. Do this as the final step so that
     * the 'consistent read' permission can be granted.  */
    if (remove_commit_top_bs) {
        bdrv_child_try_set_perm(commit_top_bs->backing, 0, BLK_PERM_ALL,
                                &error_abort);
        bdrv_replace_node(commit_top_bs, backing_bs(commit_top_bs),
                          &error_abort);
    }

    bdrv_unref(commit_top_bs);
    bdrv_unref(top);

As the comment states, bdrv_replace_node() requires that the permission
restrictions that the commit job made are already lifted. The most
important part is done by the explicit block_job_remove_all_bdrv() call
right before this hunk. It still leaves bjob->blk around, which could
have implications, but luckily we didn't take any permissions for that
one:

    s = block_job_create(job_id, &commit_job_driver, NULL, bs, 0, BLK_PERM_ALL,
                         speed, JOB_DEFAULT, NULL, NULL, errp);

So I think we got everything out of the way and bdrv_replace_node() can
do what it wants to do.

Kevin

Re: [Qemu-devel] [PATCH 16/21] block/commit: refactor commit to use job callbacks

[Kevin Wolf]

Am 07.08.2018 um 06:33 hat John Snow geschrieben:
> Use the component callbacks; prepare, commit, abort, and clean.
> 
> NB: prepare is only called when the job has not yet failed;
> and abort can be called after prepare.
> 
> complete -> prepare -> abort -> clean
> complete -> abort -> clean

I always found this confusing. After converting all jobs to use the
infrastructure, do you think that this design is helpful? You seem to be
calling *_common function from commit and abort, with an almost empty
prepare, which looks like a hint that maybe we should reorganise things.

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/commit.c | 94 +++++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 61 insertions(+), 33 deletions(-)
> 
> diff --git a/block/commit.c b/block/commit.c
> index 1a4a56795f..6673a0544e 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -35,7 +35,9 @@ typedef struct CommitBlockJob {
>      BlockJob common;
>      BlockDriverState *commit_top_bs;
>      BlockBackend *top;
> +    BlockDriverState *top_bs;
>      BlockBackend *base;
> +    BlockDriverState *base_bs;
>      BlockdevOnError on_error;
>      int base_flags;
>      char *backing_file_str;

More state. :-(

Can we at least move the new fields to the end of the struct with a
comment that they are only valid after .exit()?

> @@ -71,37 +73,37 @@ static int coroutine_fn commit_populate(BlockBackend *bs, BlockBackend *base,
>  static void commit_exit(Job *job)
>  {
>      CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
> -    BlockJob *bjob = &s->common;
> -    BlockDriverState *top = blk_bs(s->top);
> -    BlockDriverState *base = blk_bs(s->base);
> -    BlockDriverState *commit_top_bs = s->commit_top_bs;
> -    int ret = job->ret;
> -    bool remove_commit_top_bs = false;
> +
> +    s->top_bs = blk_bs(s->top);
> +    s->base_bs = blk_bs(s->base);
>  
>      /* Make sure commit_top_bs and top stay around until bdrv_replace_node() */
> -    bdrv_ref(top);
> -    bdrv_ref(commit_top_bs);
> +    bdrv_ref(s->top_bs);
> +    bdrv_ref(s->commit_top_bs);
> +}
>  
> -    /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before
> -     * the normal backing chain can be restored. */
> -    blk_unref(s->base);
> +static int commit_prepare(Job *job)
> +{
> +    CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
>  
> -    if (!job_is_cancelled(job) && ret == 0) {
> -        /* success */
> -        ret = bdrv_drop_intermediate(s->commit_top_bs, base,
> -                                     s->backing_file_str);
> -    } else {
> -        /* XXX Can (or should) we somehow keep 'consistent read' blocked even
> -         * after the failed/cancelled commit job is gone? If we already wrote
> -         * something to base, the intermediate images aren't valid any more. */
> -        remove_commit_top_bs = true;
> -    }
> +     /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before
> +      * the normal backing chain can be restored. */
> +     blk_unref(s->base);
> +     s->base = NULL;
> +
> +     return bdrv_drop_intermediate(s->commit_top_bs, s->base_bs,
> +                                   s->backing_file_str);

The indentation is off here (which is weird because the additional space
seems to be the only change to most of the lines).

> +}
> +
> +static void commit_exit_common(Job *job)
> +{
> +    CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
>  
>      /* restore base open flags here if appropriate (e.g., change the base back
>       * to r/o). These reopens do not need to be atomic, since we won't abort
>       * even on failure here */
> -    if (s->base_flags != bdrv_get_flags(base)) {
> -        bdrv_reopen(base, s->base_flags, NULL);
> +    if (s->base_flags != bdrv_get_flags(s->base_bs)) {
> +        bdrv_reopen(s->base_bs, s->base_flags, NULL);
>      }
>      g_free(s->backing_file_str);
>      blk_unref(s->top);

Feels like general cleanup, so intuitively, I'd expect this in .clean.
The only thing that could make this impossible is the ordering.

bdrv_reopen() of s->base_bs should be safe to be deferred, the node
is still referenced after the job is concluded and we don't rely on it
being read-only again in any of the following completion code.

g_free() is safe to be moved anyway.

blk_unref(s->top) doesn't change the graph because we did a
bdrv_ref(s->top_bs). The permissions of the BlockBackend could still be
a problem. However, it doesn't take any permissions:

    s->top = blk_new(0, BLK_PERM_ALL);

So I think moving this first part of commit_exit_common() to .clean
should be safe.

> @@ -110,21 +112,43 @@ static void commit_exit(Job *job)
>       * job_finish_sync()), job_completed() won't free it and therefore the
>       * blockers on the intermediate nodes remain. This would cause
>       * bdrv_set_backing_hd() to fail. */
> -    block_job_remove_all_bdrv(bjob);
> +    block_job_remove_all_bdrv(&s->common);

There hasn't been any bdrv_set_backing_hd() close to this comment for a
while. Might be time to update it.

I suppose the update would refer to bdrv_replace_node(), which only
happens in .abort, so should block_job_remove_all_bdrv() move, too?

With these changes, commit_exit_common() would be gone.

> +}
> +
> +static void commit_commit(Job *job)
> +{
> +    commit_exit_common(job);
> +}

(I think I've answered this question now, by effectively proposing to do
away with .commit, but I'll leave it there anyway.)

Is there any reason for the split between .prepare and .commit as it is
or is this mostly arbitrary? Probably the latter because commit isn't
even transactionable?

> +static void commit_abort(Job *job)
> +{
> +    CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
> +
> +    if (s->base) {
> +        blk_unref(s->base);
> +    }
> +
> +    commit_exit_common(job);
> +
> +    /* XXX Can (or should) we somehow keep 'consistent read' blocked even
> +     * after the failed/cancelled commit job is gone? If we already wrote
> +     * something to base, the intermediate images aren't valid any more. */
>  
>      /* If bdrv_drop_intermediate() didn't already do that, remove the commit
>       * filter driver from the backing chain. Do this as the final step so that
>       * the 'consistent read' permission can be granted.  */
> -    if (remove_commit_top_bs) {
> -        bdrv_child_try_set_perm(commit_top_bs->backing, 0, BLK_PERM_ALL,
> -                                &error_abort);
> -        bdrv_replace_node(commit_top_bs, backing_bs(commit_top_bs),
> -                          &error_abort);
> -    }
> +    bdrv_child_try_set_perm(s->commit_top_bs->backing, 0, BLK_PERM_ALL,
> +                            &error_abort);
> +    bdrv_replace_node(s->commit_top_bs, backing_bs(s->commit_top_bs),
> +                      &error_abort);
> +}
>  
> -    bdrv_unref(commit_top_bs);
> -    bdrv_unref(top);
> -    job->ret = ret;
> +static void commit_clean(Job *job)
> +{
> +    CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
> +
> +    bdrv_unref(s->commit_top_bs);
> +    bdrv_unref(s->top_bs);
>  }
>  
>  static int coroutine_fn commit_run(Job *job)
> @@ -214,6 +238,10 @@ static const BlockJobDriver commit_job_driver = {
>          .drain         = block_job_drain,
>          .start         = commit_run,
>          .exit          = commit_exit,
> +        .prepare       = commit_prepare,
> +        .commit        = commit_commit,
> +        .abort         = commit_abort,
> +        .clean         = commit_clean
>      },
>  };

Kevin

Re: [Qemu-devel] [Qemu-block] [PATCH 16/21] block/commit: refactor commit to use job callbacks

[Kevin Wolf]

Noticed one other thing...

Am 09.08.2018 um 17:12 hat Kevin Wolf geschrieben:
> Am 07.08.2018 um 06:33 hat John Snow geschrieben:
> > Use the component callbacks; prepare, commit, abort, and clean.
> > 
> > NB: prepare is only called when the job has not yet failed;
> > and abort can be called after prepare.
> > 
> > complete -> prepare -> abort -> clean
> > complete -> abort -> clean
> 
> I always found this confusing. After converting all jobs to use the
> infrastructure, do you think that this design is helpful? You seem to be
> calling *_common function from commit and abort, with an almost empty
> prepare, which looks like a hint that maybe we should reorganise things.
> 
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  block/commit.c | 94 +++++++++++++++++++++++++++++++++++++---------------------
> >  1 file changed, 61 insertions(+), 33 deletions(-)
> > 
> > diff --git a/block/commit.c b/block/commit.c
> > index 1a4a56795f..6673a0544e 100644
> > --- a/block/commit.c
> > +++ b/block/commit.c
> > @@ -35,7 +35,9 @@ typedef struct CommitBlockJob {
> >      BlockJob common;
> >      BlockDriverState *commit_top_bs;
> >      BlockBackend *top;
> > +    BlockDriverState *top_bs;
> >      BlockBackend *base;
> > +    BlockDriverState *base_bs;
> >      BlockdevOnError on_error;
> >      int base_flags;
> >      char *backing_file_str;
> 
> More state. :-(
> 
> Can we at least move the new fields to the end of the struct with a
> comment that they are only valid after .exit()?
> 
> > @@ -71,37 +73,37 @@ static int coroutine_fn commit_populate(BlockBackend *bs, BlockBackend *base,
> >  static void commit_exit(Job *job)
> >  {
> >      CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
> > -    BlockJob *bjob = &s->common;
> > -    BlockDriverState *top = blk_bs(s->top);
> > -    BlockDriverState *base = blk_bs(s->base);
> > -    BlockDriverState *commit_top_bs = s->commit_top_bs;
> > -    int ret = job->ret;
> > -    bool remove_commit_top_bs = false;
> > +
> > +    s->top_bs = blk_bs(s->top);
> > +    s->base_bs = blk_bs(s->base);
> >  
> >      /* Make sure commit_top_bs and top stay around until bdrv_replace_node() */
> > -    bdrv_ref(top);
> > -    bdrv_ref(commit_top_bs);
> > +    bdrv_ref(s->top_bs);
> > +    bdrv_ref(s->commit_top_bs);
> > +}

These bdrv_refs() protect against nodes going away...

> > -    /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before
> > -     * the normal backing chain can be restored. */
> > -    blk_unref(s->base);
> > +static int commit_prepare(Job *job)
> > +{
> > +    CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
> >  
> > -    if (!job_is_cancelled(job) && ret == 0) {
> > -        /* success */
> > -        ret = bdrv_drop_intermediate(s->commit_top_bs, base,
> > -                                     s->backing_file_str);
> > -    } else {
> > -        /* XXX Can (or should) we somehow keep 'consistent read' blocked even
> > -         * after the failed/cancelled commit job is gone? If we already wrote
> > -         * something to base, the intermediate images aren't valid any more. */
> > -        remove_commit_top_bs = true;
> > -    }
> > +     /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before
> > +      * the normal backing chain can be restored. */
> > +     blk_unref(s->base);
> > +     s->base = NULL;
> > +
> > +     return bdrv_drop_intermediate(s->commit_top_bs, s->base_bs,
> > +                                   s->backing_file_str);
> 
> The indentation is off here (which is weird because the additional space
> seems to be the only change to most of the lines).
> 
> > +}
> > +
> > +static void commit_exit_common(Job *job)
> > +{
> > +    CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
> >  
> >      /* restore base open flags here if appropriate (e.g., change the base back
> >       * to r/o). These reopens do not need to be atomic, since we won't abort
> >       * even on failure here */
> > -    if (s->base_flags != bdrv_get_flags(base)) {
> > -        bdrv_reopen(base, s->base_flags, NULL);
> > +    if (s->base_flags != bdrv_get_flags(s->base_bs)) {
> > +        bdrv_reopen(s->base_bs, s->base_flags, NULL);
> >      }
> >      g_free(s->backing_file_str);
> >      blk_unref(s->top);
> 
> Feels like general cleanup, so intuitively, I'd expect this in .clean.
> The only thing that could make this impossible is the ordering.
> 
> bdrv_reopen() of s->base_bs should be safe to be deferred, the node
> is still referenced after the job is concluded and we don't rely on it
> being read-only again in any of the following completion code.
> 
> g_free() is safe to be moved anyway.
> 
> blk_unref(s->top) doesn't change the graph because we did a
> bdrv_ref(s->top_bs). The permissions of the BlockBackend could still be
> a problem. However, it doesn't take any permissions:
> 
>     s->top = blk_new(0, BLK_PERM_ALL);
> 
> So I think moving this first part of commit_exit_common() to .clean
> should be safe.
> 
> > @@ -110,21 +112,43 @@ static void commit_exit(Job *job)
> >       * job_finish_sync()), job_completed() won't free it and therefore the
> >       * blockers on the intermediate nodes remain. This would cause
> >       * bdrv_set_backing_hd() to fail. */
> > -    block_job_remove_all_bdrv(bjob);
> > +    block_job_remove_all_bdrv(&s->common);

...during block_job_remove_all_bdrv(), I think. Before we do this, we
have a reference to each node in the subchain anyway.

> There hasn't been any bdrv_set_backing_hd() close to this comment for a
> while. Might be time to update it.
> 
> I suppose the update would refer to bdrv_replace_node(), which only
> happens in .abort, so should block_job_remove_all_bdrv() move, too?
> 
> With these changes, commit_exit_common() would be gone.

So if we move block_job_remove_all_bdrv() to .abort, the bdrv_ref()
could become local to .abort as well. This wouldn't only get us rid of
the additional state in CommitBlockJob, but in fact it would make .exit
empty.

And as it happens, commit is the only job that still has an .exit after
this series, so the callback can go away altogether. Which poses the
question whether it's really worth introducing it in the first place.
Does it simplify the conversion much?

But anyway, introducing it just for a short time and removing it again
in the same series would be okay with me, too, if it makes your life any
easier.

Kevin

> > +}
> > +
> > +static void commit_commit(Job *job)
> > +{
> > +    commit_exit_common(job);
> > +}
> 
> (I think I've answered this question now, by effectively proposing to do
> away with .commit, but I'll leave it there anyway.)
> 
> Is there any reason for the split between .prepare and .commit as it is
> or is this mostly arbitrary? Probably the latter because commit isn't
> even transactionable?
> 
> > +static void commit_abort(Job *job)
> > +{
> > +    CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
> > +
> > +    if (s->base) {
> > +        blk_unref(s->base);
> > +    }
> > +
> > +    commit_exit_common(job);
> > +
> > +    /* XXX Can (or should) we somehow keep 'consistent read' blocked even
> > +     * after the failed/cancelled commit job is gone? If we already wrote
> > +     * something to base, the intermediate images aren't valid any more. */
> >  
> >      /* If bdrv_drop_intermediate() didn't already do that, remove the commit
> >       * filter driver from the backing chain. Do this as the final step so that
> >       * the 'consistent read' permission can be granted.  */
> > -    if (remove_commit_top_bs) {
> > -        bdrv_child_try_set_perm(commit_top_bs->backing, 0, BLK_PERM_ALL,
> > -                                &error_abort);
> > -        bdrv_replace_node(commit_top_bs, backing_bs(commit_top_bs),
> > -                          &error_abort);
> > -    }
> > +    bdrv_child_try_set_perm(s->commit_top_bs->backing, 0, BLK_PERM_ALL,
> > +                            &error_abort);
> > +    bdrv_replace_node(s->commit_top_bs, backing_bs(s->commit_top_bs),
> > +                      &error_abort);
> > +}
> >  
> > -    bdrv_unref(commit_top_bs);
> > -    bdrv_unref(top);
> > -    job->ret = ret;
> > +static void commit_clean(Job *job)
> > +{
> > +    CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
> > +
> > +    bdrv_unref(s->commit_top_bs);
> > +    bdrv_unref(s->top_bs);
> >  }
> >  
> >  static int coroutine_fn commit_run(Job *job)
> > @@ -214,6 +238,10 @@ static const BlockJobDriver commit_job_driver = {
> >          .drain         = block_job_drain,
> >          .start         = commit_run,
> >          .exit          = commit_exit,
> > +        .prepare       = commit_prepare,
> > +        .commit        = commit_commit,
> > +        .abort         = commit_abort,
> > +        .clean         = commit_clean
> >      },
> >  };
> 
> Kevin
> 

Re: [Qemu-devel] [PATCH 19/21] qapi/block-commit: expose new job properties

[Kevin Wolf]

Am 07.08.2018 um 16:52 hat Eric Blake geschrieben:
> On 08/06/2018 11:33 PM, John Snow wrote:
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   blockdev.c           | 8 ++++++++
> >   qapi/block-core.json | 3 ++-
> >   2 files changed, 10 insertions(+), 1 deletion(-)
> > 
> 
> > +++ b/qapi/block-core.json
> > @@ -1518,7 +1518,8 @@
> >   { 'command': 'block-commit',
> >     'data': { '*job-id': 'str', 'device': 'str', '*base': 'str', '*top': 'str',
> >               '*backing-file': 'str', '*speed': 'int',
> > -            '*filter-node-name': 'str' } }
> > +            '*filter-node-name': 'str',
> > +            '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } }
> 
> Missing documentation, including a 'since 3.1' tag.

And the same for the other two commands.

Kevin

Re: [Qemu-devel] [Qemu-block] [PATCH 00/21] jobs: defer graph changes until finalize

[Peter Krempa]

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

On Tue, Aug 07, 2018 at 00:33:28 -0400, John Snow wrote:
> This series forces all jobs to use the "finalize" semantics that were
> introduced previously, but only exposed via the backup jobs.
> This series looks huge, but it's mostly mechanical changes broken out
> into a series of much smaller commits so that the changes are easier
> to follow at each step.

Please note that due to the semantics of the commands 'block-job-cancel'
and 'block-job-complete' at least in case of the drive/blockdev-mirror
job are technically blockjobs of it's own since they may fail or finish
in an unbounded amount of time and the actual return value on the
monitor does not reflect the result of the operation itself.

It would be very helpful if qemu treas them as such.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 00/21] jobs: defer graph changes until finalize

[Kevin Wolf]

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

Am 15.08.2018 um 16:44 hat Peter Krempa geschrieben:
> On Tue, Aug 07, 2018 at 00:33:28 -0400, John Snow wrote:
> > This series forces all jobs to use the "finalize" semantics that were
> > introduced previously, but only exposed via the backup jobs.
> > This series looks huge, but it's mostly mechanical changes broken out
> > into a series of much smaller commits so that the changes are easier
> > to follow at each step.
> 
> Please note that due to the semantics of the commands 'block-job-cancel'
> and 'block-job-complete' at least in case of the drive/blockdev-mirror
> job are technically blockjobs of it's own since they may fail or finish
> in an unbounded amount of time and the actual return value on the
> monitor does not reflect the result of the operation itself.
> 
> It would be very helpful if qemu treas them as such.

'block-job-cancel' and 'block-job-complete' return immediately. They
just move the given job into the next phase and return. That job may
then still take some time until it finally completes (successfully or
with an error), but that's not part of 'block-job-cancel/complete' any
more.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 00/21] jobs: defer graph changes until finalize

[Peter Krempa]

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

On Wed, Aug 15, 2018 at 17:00:29 +0200, Kevin Wolf wrote:
> Am 15.08.2018 um 16:44 hat Peter Krempa geschrieben:
> > On Tue, Aug 07, 2018 at 00:33:28 -0400, John Snow wrote:
> > > This series forces all jobs to use the "finalize" semantics that were
> > > introduced previously, but only exposed via the backup jobs.
> > > This series looks huge, but it's mostly mechanical changes broken out
> > > into a series of much smaller commits so that the changes are easier
> > > to follow at each step.
> > 
> > Please note that due to the semantics of the commands 'block-job-cancel'
> > and 'block-job-complete' at least in case of the drive/blockdev-mirror
> > job are technically blockjobs of it's own since they may fail or finish
> > in an unbounded amount of time and the actual return value on the
> > monitor does not reflect the result of the operation itself.
> > 
> > It would be very helpful if qemu treas them as such.
> 
> 'block-job-cancel' and 'block-job-complete' return immediately. They
> just move the given job into the next phase and return. That job may
> then still take some time until it finally completes (successfully or
> with an error), but that's not part of 'block-job-cancel/complete' any
> more.

Ah I see. That means that we just need to modify the semantics/language
used in libvirt for this.

Thanks for the clarification.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 04/21] block/commit: utilize job_exit shim

[John Snow]

On 08/08/2018 12:29 PM, Kevin Wolf wrote:
> Am 07.08.2018 um 06:33 hat John Snow geschrieben:
>> Change the manual deferment to commit_complete into the implicit
>> callback to job_exit.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
> 
> There is one tricky thing in this patch that the commit message could be
> a bit more explicit about, which is moving job_completed() to a later
> point.
> 
> This is the code that happens between the old call of job_completed()
> and the new one:
> 
>     /* If bdrv_drop_intermediate() didn't already do that, remove the commit
>      * filter driver from the backing chain. Do this as the final step so that
>      * the 'consistent read' permission can be granted.  */
>     if (remove_commit_top_bs) {
>         bdrv_child_try_set_perm(commit_top_bs->backing, 0, BLK_PERM_ALL,
>                                 &error_abort);
>         bdrv_replace_node(commit_top_bs, backing_bs(commit_top_bs),
>                           &error_abort);
>     }
> 
>     bdrv_unref(commit_top_bs);
>     bdrv_unref(top);
> 
> As the comment states, bdrv_replace_node() requires that the permission
> restrictions that the commit job made are already lifted. The most
> important part is done by the explicit block_job_remove_all_bdrv() call
> right before this hunk. It still leaves bjob->blk around, which could
> have implications, but luckily we didn't take any permissions for that
> one:
> 
>     s = block_job_create(job_id, &commit_job_driver, NULL, bs, 0, BLK_PERM_ALL,
>                          speed, JOB_DEFAULT, NULL, NULL, errp);
> 
> So I think we got everything out of the way and bdrv_replace_node() can
> do what it wants to do.
> 
> Kevin
> 

I suppose it will be up to the author of a job to be aware of any
permissions they pick up at creation time that might have an effect on
the cleanup they wish to do during completion time.

--js

Re: [Qemu-devel] [PATCH 16/21] block/commit: refactor commit to use job callbacks

[John Snow]

On 08/09/2018 11:12 AM, Kevin Wolf wrote:
> Am 07.08.2018 um 06:33 hat John Snow geschrieben:
>> Use the component callbacks; prepare, commit, abort, and clean.
>>
>> NB: prepare is only called when the job has not yet failed;
>> and abort can be called after prepare.
>>
>> complete -> prepare -> abort -> clean
>> complete -> abort -> clean
> 
> I always found this confusing. After converting all jobs to use the
> infrastructure, do you think that this design is helpful? You seem to be
> calling *_common function from commit and abort, with an almost empty
> prepare, which looks like a hint that maybe we should reorganise things.
> 

After rewriting things a bit, I think it would be nicer to call prepare
regardless of circumstances. The callbacks will be nicer for it.

When I wrote it the first time, the thought process was something like:

"Well, we won't need to prepare anything if we've already failed, we
just need to speed along to abort."

I wasn't considering so carefully the common cleanup case that needs to
occur post-finalization which appears to actually happen in a good
number of jobs.

>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  block/commit.c | 94 +++++++++++++++++++++++++++++++++++++---------------------
>>  1 file changed, 61 insertions(+), 33 deletions(-)
>>
>> diff --git a/block/commit.c b/block/commit.c
>> index 1a4a56795f..6673a0544e 100644
>> --- a/block/commit.c
>> +++ b/block/commit.c
>> @@ -35,7 +35,9 @@ typedef struct CommitBlockJob {
>>      BlockJob common;
>>      BlockDriverState *commit_top_bs;
>>      BlockBackend *top;
>> +    BlockDriverState *top_bs;
>>      BlockBackend *base;
>> +    BlockDriverState *base_bs;
>>      BlockdevOnError on_error;
>>      int base_flags;
>>      char *backing_file_str;
> 
> More state. :-(
> 
> Can we at least move the new fields to the end of the struct with a
> comment that they are only valid after .exit()?
> 

Sure ... admittedly this is just a crutch because I was not precisely
sure exactly when these might change out from underneath me. If there
are some clever ways to avoid needing the state, feel free to suggest them.

>> @@ -71,37 +73,37 @@ static int coroutine_fn commit_populate(BlockBackend *bs, BlockBackend *base,
>>  static void commit_exit(Job *job)
>>  {
>>      CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
>> -    BlockJob *bjob = &s->common;
>> -    BlockDriverState *top = blk_bs(s->top);
>> -    BlockDriverState *base = blk_bs(s->base);
>> -    BlockDriverState *commit_top_bs = s->commit_top_bs;
>> -    int ret = job->ret;
>> -    bool remove_commit_top_bs = false;
>> +
>> +    s->top_bs = blk_bs(s->top);
>> +    s->base_bs = blk_bs(s->base);
>>  
>>      /* Make sure commit_top_bs and top stay around until bdrv_replace_node() */
>> -    bdrv_ref(top);
>> -    bdrv_ref(commit_top_bs);
>> +    bdrv_ref(s->top_bs);
>> +    bdrv_ref(s->commit_top_bs);
>> +}
>>  
>> -    /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before
>> -     * the normal backing chain can be restored. */
>> -    blk_unref(s->base);
>> +static int commit_prepare(Job *job)
>> +{
>> +    CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
>>  
>> -    if (!job_is_cancelled(job) && ret == 0) {
>> -        /* success */
>> -        ret = bdrv_drop_intermediate(s->commit_top_bs, base,
>> -                                     s->backing_file_str);
>> -    } else {
>> -        /* XXX Can (or should) we somehow keep 'consistent read' blocked even
>> -         * after the failed/cancelled commit job is gone? If we already wrote
>> -         * something to base, the intermediate images aren't valid any more. */
>> -        remove_commit_top_bs = true;
>> -    }
>> +     /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before
>> +      * the normal backing chain can be restored. */
>> +     blk_unref(s->base);
>> +     s->base = NULL;
>> +
>> +     return bdrv_drop_intermediate(s->commit_top_bs, s->base_bs,
>> +                                   s->backing_file_str);
> 
> The indentation is off here (which is weird because the additional space
> seems to be the only change to most of the lines).
> 
>> +}
>> +
>> +static void commit_exit_common(Job *job)
>> +{
>> +    CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
>>  
>>      /* restore base open flags here if appropriate (e.g., change the base back
>>       * to r/o). These reopens do not need to be atomic, since we won't abort
>>       * even on failure here */
>> -    if (s->base_flags != bdrv_get_flags(base)) {
>> -        bdrv_reopen(base, s->base_flags, NULL);
>> +    if (s->base_flags != bdrv_get_flags(s->base_bs)) {
>> +        bdrv_reopen(s->base_bs, s->base_flags, NULL);
>>      }
>>      g_free(s->backing_file_str);
>>      blk_unref(s->top);
> 
> Feels like general cleanup, so intuitively, I'd expect this in .clean.
> The only thing that could make this impossible is the ordering.
> 
> bdrv_reopen() of s->base_bs should be safe to be deferred, the node
> is still referenced after the job is concluded and we don't rely on it
> being read-only again in any of the following completion code.
> 
> g_free() is safe to be moved anyway.
> 
> blk_unref(s->top) doesn't change the graph because we did a
> bdrv_ref(s->top_bs). The permissions of the BlockBackend could still be
> a problem. However, it doesn't take any permissions:
> 
>     s->top = blk_new(0, BLK_PERM_ALL);
> 
> So I think moving this first part of commit_exit_common() to .clean
> should be safe.
> 

OK, I'll try to do this a little more intelligently. This is definitely
a bit of a hack-and-slash job; I'll look at your comments in the second
reply here too and try to eliminate .exit which I agree is likely not
strictly needed especially if we make prepare something that's always
called.

>> @@ -110,21 +112,43 @@ static void commit_exit(Job *job)
>>       * job_finish_sync()), job_completed() won't free it and therefore the
>>       * blockers on the intermediate nodes remain. This would cause
>>       * bdrv_set_backing_hd() to fail. */
>> -    block_job_remove_all_bdrv(bjob);
>> +    block_job_remove_all_bdrv(&s->common);
> 
> There hasn't been any bdrv_set_backing_hd() close to this comment for a
> while. Might be time to update it.
> 
> I suppose the update would refer to bdrv_replace_node(), which only
> happens in .abort, so should block_job_remove_all_bdrv() move, too?
> 
> With these changes, commit_exit_common() would be gone.
> 
>> +}
>> +
>> +static void commit_commit(Job *job)
>> +{
>> +    commit_exit_common(job);
>> +}
> 
> (I think I've answered this question now, by effectively proposing to do
> away with .commit, but I'll leave it there anyway.)
> 
> Is there any reason for the split between .prepare and .commit as it is
> or is this mostly arbitrary? Probably the latter because commit isn't
> even transactionable?
> 

Just historical, yeah -- we only had commit and abort for a while, and
prepare didn't join the party until we wanted finalize and it became
apparent we needed a way to check the return code and still be able to
fail the transaction in time to be able to do a final commit or still
abort very, very late in the process.

Probably there's no real meaningful reason that prepare and commit need
to be separate callbacks.

It is possible that:

prepare --> [abort] --> clean

would be entirely sufficient for all current cases.

>> +static void commit_abort(Job *job)
>> +{
>> +    CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
>> +
>> +    if (s->base) {
>> +        blk_unref(s->base);
>> +    }
>> +
>> +    commit_exit_common(job);
>> +
>> +    /* XXX Can (or should) we somehow keep 'consistent read' blocked even
>> +     * after the failed/cancelled commit job is gone? If we already wrote
>> +     * something to base, the intermediate images aren't valid any more. */
>>  
>>      /* If bdrv_drop_intermediate() didn't already do that, remove the commit
>>       * filter driver from the backing chain. Do this as the final step so that
>>       * the 'consistent read' permission can be granted.  */
>> -    if (remove_commit_top_bs) {
>> -        bdrv_child_try_set_perm(commit_top_bs->backing, 0, BLK_PERM_ALL,
>> -                                &error_abort);
>> -        bdrv_replace_node(commit_top_bs, backing_bs(commit_top_bs),
>> -                          &error_abort);
>> -    }
>> +    bdrv_child_try_set_perm(s->commit_top_bs->backing, 0, BLK_PERM_ALL,
>> +                            &error_abort);
>> +    bdrv_replace_node(s->commit_top_bs, backing_bs(s->commit_top_bs),
>> +                      &error_abort);
>> +}
>>  
>> -    bdrv_unref(commit_top_bs);
>> -    bdrv_unref(top);
>> -    job->ret = ret;
>> +static void commit_clean(Job *job)
>> +{
>> +    CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
>> +
>> +    bdrv_unref(s->commit_top_bs);
>> +    bdrv_unref(s->top_bs);
>>  }
>>  
>>  static int coroutine_fn commit_run(Job *job)
>> @@ -214,6 +238,10 @@ static const BlockJobDriver commit_job_driver = {
>>          .drain         = block_job_drain,
>>          .start         = commit_run,
>>          .exit          = commit_exit,
>> +        .prepare       = commit_prepare,
>> +        .commit        = commit_commit,
>> +        .abort         = commit_abort,
>> +        .clean         = commit_clean
>>      },
>>  };
> 
> Kevin
> 

-- 
—js

Re: [Qemu-devel] [PATCH 04/21] block/commit: utilize job_exit shim

[Kevin Wolf]

Am 15.08.2018 um 22:52 hat John Snow geschrieben:
> On 08/08/2018 12:29 PM, Kevin Wolf wrote:
> > Am 07.08.2018 um 06:33 hat John Snow geschrieben:
> >> Change the manual deferment to commit_complete into the implicit
> >> callback to job_exit.
> >>
> >> Signed-off-by: John Snow <jsnow@redhat.com>
> > 
> > There is one tricky thing in this patch that the commit message could be
> > a bit more explicit about, which is moving job_completed() to a later
> > point.
> > 
> > This is the code that happens between the old call of job_completed()
> > and the new one:
> > 
> >     /* If bdrv_drop_intermediate() didn't already do that, remove the commit
> >      * filter driver from the backing chain. Do this as the final step so that
> >      * the 'consistent read' permission can be granted.  */
> >     if (remove_commit_top_bs) {
> >         bdrv_child_try_set_perm(commit_top_bs->backing, 0, BLK_PERM_ALL,
> >                                 &error_abort);
> >         bdrv_replace_node(commit_top_bs, backing_bs(commit_top_bs),
> >                           &error_abort);
> >     }
> > 
> >     bdrv_unref(commit_top_bs);
> >     bdrv_unref(top);
> > 
> > As the comment states, bdrv_replace_node() requires that the permission
> > restrictions that the commit job made are already lifted. The most
> > important part is done by the explicit block_job_remove_all_bdrv() call
> > right before this hunk. It still leaves bjob->blk around, which could
> > have implications, but luckily we didn't take any permissions for that
> > one:
> > 
> >     s = block_job_create(job_id, &commit_job_driver, NULL, bs, 0, BLK_PERM_ALL,
> >                          speed, JOB_DEFAULT, NULL, NULL, errp);
> > 
> > So I think we got everything out of the way and bdrv_replace_node() can
> > do what it wants to do.
> > 
> > Kevin
> > 
> 
> I suppose it will be up to the author of a job to be aware of any
> permissions they pick up at creation time that might have an effect on
> the cleanup they wish to do during completion time.

I'm really only talking about the commit job, the specific conversion
in this patch and the terse commit message.

Kevin

Re: [Qemu-devel] [PATCH 16/21] block/commit: refactor commit to use job callbacks

[Kevin Wolf]

Am 15.08.2018 um 23:17 hat John Snow geschrieben:
> On 08/09/2018 11:12 AM, Kevin Wolf wrote:
> > Am 07.08.2018 um 06:33 hat John Snow geschrieben:
> >> Use the component callbacks; prepare, commit, abort, and clean.
> >>
> >> NB: prepare is only called when the job has not yet failed;
> >> and abort can be called after prepare.
> >>
> >> complete -> prepare -> abort -> clean
> >> complete -> abort -> clean
> > 
> > I always found this confusing. After converting all jobs to use the
> > infrastructure, do you think that this design is helpful? You seem to be
> > calling *_common function from commit and abort, with an almost empty
> > prepare, which looks like a hint that maybe we should reorganise things.
> > 
> 
> After rewriting things a bit, I think it would be nicer to call prepare
> regardless of circumstances. The callbacks will be nicer for it.
> 
> When I wrote it the first time, the thought process was something like:
> 
> "Well, we won't need to prepare anything if we've already failed, we
> just need to speed along to abort."
> 
> I wasn't considering so carefully the common cleanup case that needs to
> occur post-finalization which appears to actually happen in a good
> number of jobs.

Maybe let's see how things turn out when we actually make some more jobs
transactionable. For now, it seems that the *_common function can go
away at least for commit; and we didn't even try to properly split the
completion of the other two jobs.

> >> Signed-off-by: John Snow <jsnow@redhat.com>
> >> ---
> >>  block/commit.c | 94 +++++++++++++++++++++++++++++++++++++---------------------
> >>  1 file changed, 61 insertions(+), 33 deletions(-)
> >>
> >> diff --git a/block/commit.c b/block/commit.c
> >> index 1a4a56795f..6673a0544e 100644
> >> --- a/block/commit.c
> >> +++ b/block/commit.c
> >> @@ -35,7 +35,9 @@ typedef struct CommitBlockJob {
> >>      BlockJob common;
> >>      BlockDriverState *commit_top_bs;
> >>      BlockBackend *top;
> >> +    BlockDriverState *top_bs;
> >>      BlockBackend *base;
> >> +    BlockDriverState *base_bs;
> >>      BlockdevOnError on_error;
> >>      int base_flags;
> >>      char *backing_file_str;
> > 
> > More state. :-(
> > 
> > Can we at least move the new fields to the end of the struct with a
> > comment that they are only valid after .exit()?
> > 
> 
> Sure ... admittedly this is just a crutch because I was not precisely
> sure exactly when these might change out from underneath me. If there
> are some clever ways to avoid needing the state, feel free to suggest them.

I did, in the reply to my own mail. Everything that would need the state
can actually live in .abort, so it can be local.

> >> +}
> >> +
> >> +static void commit_commit(Job *job)
> >> +{
> >> +    commit_exit_common(job);
> >> +}
> > 
> > (I think I've answered this question now, by effectively proposing to do
> > away with .commit, but I'll leave it there anyway.)
> > 
> > Is there any reason for the split between .prepare and .commit as it is
> > or is this mostly arbitrary? Probably the latter because commit isn't
> > even transactionable?
> > 
> 
> Just historical, yeah -- we only had commit and abort for a while, and
> prepare didn't join the party until we wanted finalize and it became
> apparent we needed a way to check the return code and still be able to
> fail the transaction in time to be able to do a final commit or still
> abort very, very late in the process.
> 
> Probably there's no real meaningful reason that prepare and commit need
> to be separate callbacks.
> 
> It is possible that:
> 
> prepare --> [abort] --> clean
> 
> would be entirely sufficient for all current cases.

I think jobs that actually support transactions (i.e. backup currently)
do in fact need commit. The other ones might not.

Kevin