Series comparison

-[PULL 0/3] Block patches
+[Qemu-devel] [PULL v2 00/10] Block patches
-The following changes since commit 9cf289af47bcfae5c75de37d8e5d6fd23705322c:
+The following changes since commit 19b599f7664b2ebfd0f405fb79c14dd241557452:
-  Merge tag 'qga-pull-request' of gitlab.com:marcandre.lureau/qemu into staging (2022-05-04 03:42:49 -0700)
+  Merge remote-tracking branch 'remotes/armbru/tags/pull-error-2018-08-27-v2' into staging (2018-08-27 16:44:20 +0100)
 are available in the Git repository at:
-  https://gitlab.com/stefanha/qemu.git tags/block-pull-request
+  https://git.xanclic.moe/XanClic/qemu.git tags/pull-block-2018-08-31-v2
-for you to fetch changes up to bef2e050d6a7feb865854c65570c496ac5a8cf53:
+for you to fetch changes up to e21a1c9831fc80ae3f3c1affdfa43350035d8588:
-  util/event-loop-base: Introduce options to set the thread pool size (2022-05-04 17:02:19 +0100)
+  jobs: remove job_defer_to_main_loop (2018-08-31 16:28:33 +0200)
 ----------------------------------------------------------------
-Pull request
+Block patches:
+- (Block) job exit refactoring, part 1
-Add new thread-pool-min/thread-pool-max parameters to control the thread pool
+  (removing job_defer_to_main_loop())
-used for async I/O.
+- test-bdrv-drain leak fix
 ----------------------------------------------------------------
+John Snow (9):
+  jobs: change start callback to run callback
+  jobs: canonize Error object
+  jobs: add exit shim
+  block/commit: utilize job_exit shim
+  block/mirror: utilize job_exit shim
+  jobs: utilize job_exit shim
+  block/backup: make function variables consistently named
+  jobs: remove ret argument to job_completed; privatize it
+  jobs: remove job_defer_to_main_loop
-Nicolas Saenz Julienne (3):
+Marc-André Lureau (1):
-  Introduce event-loop-base abstract class
+  tests: fix bdrv-drain leak
   util/main-loop: Introduce the main loop into QOM
   util/event-loop-base: Introduce options to set the thread pool size
- qapi/qom.json                    |  43 ++++++++--
+ include/qemu/job.h        | 70 ++++++++++++++++-----------------
- meson.build                      |  26 +++---
+ block/backup.c            | 81 ++++++++++++++++-----------------------
- include/block/aio.h              |  10 +++
+ block/commit.c            | 29 +++++---------
- include/block/thread-pool.h      |   3 +
+ block/create.c            | 19 +++------
- include/qemu/main-loop.h         |  10 +++
+ block/mirror.c            | 39 ++++++++-----------
- include/sysemu/event-loop-base.h |  41 +++++++++
+ block/stream.c            | 29 ++++++--------
- include/sysemu/iothread.h        |   6 +-
+ job-qmp.c                 |  5 ++-
- event-loop-base.c                | 140 +++++++++++++++++++++++++++++++
+ job.c                     | 73 ++++++++++++-----------------------
- iothread.c                       |  68 +++++----------
+ tests/test-bdrv-drain.c   | 14 +++----
- util/aio-posix.c                 |   1 +
+ tests/test-blockjob-txn.c | 25 +++++-------
- util/async.c                     |  20 +++++
+ tests/test-blockjob.c     | 17 ++++----
- util/main-loop.c                 |  65 ++++++++++++++
+ trace-events              |  2 +-
- util/thread-pool.c               |  55 +++++++++++-
+files changed, 161 insertions(+), 242 deletions(-)
 files changed, 419 insertions(+), 69 deletions(-)
  create mode 100644 include/sysemu/event-loop-base.h
  create mode 100644 event-loop-base.c
 --
-.35.1
+.17.1

-New patch
+[Qemu-devel] [PULL v2 01/10] tests: fix bdrv-drain leak
+From: Marc-André Lureau <marcandre.lureau@redhat.com>
+Spotted by ASAN:
+=================================================================
+==5378==ERROR: LeakSanitizer: detected memory leaks
+Direct leak of 65536 byte(s) in 1 object(s) allocated from:
+    #0 0x7f788f83bc48 in malloc (/lib64/libasan.so.5+0xeec48)
+    #1 0x7f788c9923c5 in g_malloc (/lib64/libglib-2.0.so.0+0x523c5)
+    #2 0x5622a1fe37bc in coroutine_trampoline /home/elmarco/src/qq/util/coroutine-ucontext.c:116
+    #3 0x7f788a15d75f in __correctly_grouped_prefixwc (/lib64/libc.so.6+0x4c75f)
+(Broken in commit 4c8158e359d.)
+Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
+Message-id: 20180809114417.28718-3-marcandre.lureau@redhat.com
+Signed-off-by: Max Reitz <mreitz@redhat.com>
+---
+ tests/test-bdrv-drain.c | 1 +
+file changed, 1 insertion(+)
+diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
+index XXXXXXX..XXXXXXX 100644
+--- a/tests/test-bdrv-drain.c
++++ b/tests/test-bdrv-drain.c
+@@ -XXX,XX +XXX,XX @@ static void coroutine_fn test_co_delete_by_drain(void *opaque)
+     }
+     dbdd->done = true;
++    g_free(buffer);
+ }
+ /**
+--
+.17.1

-New patch
+[Qemu-devel] [PULL v2 02/10] jobs: change start callback to run callback
+From: John Snow <jsnow@redhat.com>
 Presently we codify the entry point for a job as the "start" callback,
 but a more apt name would be "run" to clarify the idea that when this
 function returns we consider the job to have "finished," except for
 any cleanup which occurs in separate callbacks later.
 As part of this clarification, change the signature to include an error
 object and a return code. The error ptr is not yet used, and the return
 code while captured, will be overwritten by actions in the job_completed
 function.
 Signed-off-by: John Snow <jsnow@redhat.com>
 Reviewed-by: Max Reitz <mreitz@redhat.com>
 Message-id: 20180830015734.19765-2-jsnow@redhat.com
 Reviewed-by: Jeff Cody <jcody@redhat.com>
 Signed-off-by: Max Reitz <mreitz@redhat.com>
 ---
  include/qemu/job.h        |  2 +-
  block/backup.c            |  7 ++++---
  block/commit.c            |  7 ++++---
  block/create.c            |  8 +++++---
  block/mirror.c            | 10 ++++++----
  block/stream.c            |  7 ++++---
  job.c                     |  6 +++---
  tests/test-bdrv-drain.c   |  7 ++++---
  tests/test-blockjob-txn.c | 16 ++++++++--------
  tests/test-blockjob.c     |  7 ++++---
 files changed, 43 insertions(+), 34 deletions(-)
 diff --git a/include/qemu/job.h b/include/qemu/job.h
 index XXXXXXX..XXXXXXX 100644
 --- a/include/qemu/job.h
 +++ b/include/qemu/job.h
@@ -XXX,XX +XXX,XX @@ struct JobDriver {
      JobType job_type;
      /** Mandatory: Entrypoint for the Coroutine. */
 -    CoroutineEntry *start;
 +    int coroutine_fn (*run)(Job *job, Error **errp);
      /**
       * If the callback is not NULL, it will be invoked when the job transitions
 diff --git a/block/backup.c b/block/backup.c
 index XXXXXXX..XXXXXXX 100644
 --- a/block/backup.c
 +++ b/block/backup.c
@@ -XXX,XX +XXX,XX @@ 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, Error **errp)
  {
 -    BackupBlockJob *job = opaque;
 +    BackupBlockJob *job = container_of(opaque_job, BackupBlockJob, common.job);
      BackupCompleteData *data;
      BlockDriverState *bs = blk_bs(job->common.blk);
      int64_t offset, nb_clusters;
@@ -XXX,XX +XXX,XX @@ static void coroutine_fn backup_run(void *opaque)
      data = g_malloc(sizeof(*data));
      data->ret = ret;
      job_defer_to_main_loop(&job->common.job, backup_complete, data);
 +    return ret;
  }
  static const BlockJobDriver backup_job_driver = {
@@ -XXX,XX +XXX,XX @@ static const BlockJobDriver backup_job_driver = {
          .free                   = block_job_free,
          .user_resume            = block_job_user_resume,
          .drain                  = block_job_drain,
 -        .start                  = backup_run,
 +        .run                    = backup_run,
          .commit                 = backup_commit,
          .abort                  = backup_abort,
          .clean                  = backup_clean,
 diff --git a/block/commit.c b/block/commit.c
 index XXXXXXX..XXXXXXX 100644
 --- a/block/commit.c
 +++ b/block/commit.c
@@ -XXX,XX +XXX,XX @@ static void commit_complete(Job *job, void *opaque)
      bdrv_unref(top);
  }
 -static void coroutine_fn commit_run(void *opaque)
 +static int coroutine_fn commit_run(Job *job, Error **errp)
  {
 -    CommitBlockJob *s = opaque;
 +    CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
      CommitCompleteData *data;
      int64_t offset;
      uint64_t delay_ns = 0;
@@ -XXX,XX +XXX,XX @@ out:
      data = g_malloc(sizeof(*data));
      data->ret = ret;
      job_defer_to_main_loop(&s->common.job, commit_complete, data);
 +    return ret;
  }
  static const BlockJobDriver commit_job_driver = {
@@ -XXX,XX +XXX,XX @@ static const BlockJobDriver commit_job_driver = {
          .free          = block_job_free,
          .user_resume   = block_job_user_resume,
          .drain         = block_job_drain,
 -        .start         = commit_run,
 +        .run           = commit_run,
      },
  };
 diff --git a/block/create.c b/block/create.c
 index XXXXXXX..XXXXXXX 100644
 --- a/block/create.c
 +++ b/block/create.c
@@ -XXX,XX +XXX,XX @@ static void blockdev_create_complete(Job *job, void *opaque)
      job_completed(job, s->ret, s->err);
  }
 -static void coroutine_fn blockdev_create_run(void *opaque)
 +static int coroutine_fn blockdev_create_run(Job *job, Error **errp)
  {
 -    BlockdevCreateJob *s = opaque;
 +    BlockdevCreateJob *s = container_of(job, BlockdevCreateJob, common);
      job_progress_set_remaining(&s->common, 1);
      s->ret = s->drv->bdrv_co_create(s->opts, &s->err);
@@ -XXX,XX +XXX,XX @@ static void coroutine_fn blockdev_create_run(void *opaque)
      qapi_free_BlockdevCreateOptions(s->opts);
      job_defer_to_main_loop(&s->common, blockdev_create_complete, NULL);
 +
 +    return s->ret;
  }
  static const JobDriver blockdev_create_job_driver = {
      .instance_size = sizeof(BlockdevCreateJob),
      .job_type      = JOB_TYPE_CREATE,
 -    .start         = blockdev_create_run,
 +    .run           = blockdev_create_run,
  };
  void qmp_blockdev_create(const char *job_id, BlockdevCreateOptions *options,
 diff --git a/block/mirror.c b/block/mirror.c
 index XXXXXXX..XXXXXXX 100644
 --- a/block/mirror.c
 +++ b/block/mirror.c
@@ -XXX,XX +XXX,XX @@ static int mirror_flush(MirrorBlockJob *s)
      return ret;
  }
 -static void coroutine_fn mirror_run(void *opaque)
 +static int coroutine_fn mirror_run(Job *job, Error **errp)
  {
 -    MirrorBlockJob *s = opaque;
 +    MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
      MirrorExitData *data;
      BlockDriverState *bs = s->mirror_top_bs->backing->bs;
      BlockDriverState *target_bs = blk_bs(s->target);
@@ -XXX,XX +XXX,XX @@ immediate_exit:
      if (need_drain) {
          bdrv_drained_begin(bs);
      }
 +
      job_defer_to_main_loop(&s->common.job, mirror_exit, data);
 +    return ret;
  }
  static void mirror_complete(Job *job, Error **errp)
@@ -XXX,XX +XXX,XX @@ static const BlockJobDriver mirror_job_driver = {
          .free                   = block_job_free,
          .user_resume            = block_job_user_resume,
          .drain                  = block_job_drain,
 -        .start                  = mirror_run,
 +        .run                    = mirror_run,
          .pause                  = mirror_pause,
          .complete               = mirror_complete,
      },
@@ -XXX,XX +XXX,XX @@ static const BlockJobDriver commit_active_job_driver = {
          .free                   = block_job_free,
          .user_resume            = block_job_user_resume,
          .drain                  = block_job_drain,
 -        .start                  = mirror_run,
 +        .run                    = mirror_run,
          .pause                  = mirror_pause,
          .complete               = mirror_complete,
      },
 diff --git a/block/stream.c b/block/stream.c
 index XXXXXXX..XXXXXXX 100644
 --- a/block/stream.c
 +++ b/block/stream.c
@@ -XXX,XX +XXX,XX @@ out:
      g_free(data);
  }
 -static void coroutine_fn stream_run(void *opaque)
 +static int coroutine_fn stream_run(Job *job, Error **errp)
  {
 -    StreamBlockJob *s = opaque;
 +    StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
      StreamCompleteData *data;
      BlockBackend *blk = s->common.blk;
      BlockDriverState *bs = blk_bs(blk);
@@ -XXX,XX +XXX,XX @@ out:
      data = g_malloc(sizeof(*data));
      data->ret = ret;
      job_defer_to_main_loop(&s->common.job, stream_complete, data);
 +    return ret;
  }
  static const BlockJobDriver stream_job_driver = {
@@ -XXX,XX +XXX,XX @@ static const BlockJobDriver stream_job_driver = {
          .instance_size = sizeof(StreamBlockJob),
          .job_type      = JOB_TYPE_STREAM,
          .free          = block_job_free,
 -        .start         = stream_run,
 +        .run           = stream_run,
          .user_resume   = block_job_user_resume,
          .drain         = block_job_drain,
      },
 diff --git a/job.c b/job.c
 index XXXXXXX..XXXXXXX 100644
 --- a/job.c
 +++ b/job.c
@@ -XXX,XX +XXX,XX @@ static void coroutine_fn job_co_entry(void *opaque)
  {
      Job *job = opaque;
 -    assert(job && job->driver && job->driver->start);
 +    assert(job && job->driver && job->driver->run);
      job_pause_point(job);
 -    job->driver->start(job);
 +    job->ret = job->driver->run(job, NULL);
  }
  void job_start(Job *job)
  {
      assert(job && !job_started(job) && job->paused &&
 -           job->driver && job->driver->start);
 +           job->driver && job->driver->run);
      job->co = qemu_coroutine_create(job_co_entry, job);
      job->pause_count--;
      job->busy = true;
 diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
 index XXXXXXX..XXXXXXX 100644
 --- a/tests/test-bdrv-drain.c
 +++ b/tests/test-bdrv-drain.c
@@ -XXX,XX +XXX,XX @@ static void test_job_completed(Job *job, void *opaque)
      job_completed(job, 0, NULL);
  }
 -static void coroutine_fn test_job_start(void *opaque)
 +static int coroutine_fn test_job_run(Job *job, Error **errp)
  {
 -    TestBlockJob *s = opaque;
 +    TestBlockJob *s = container_of(job, TestBlockJob, common.job);
      job_transition_to_ready(&s->common.job);
      while (!s->should_complete) {
@@ -XXX,XX +XXX,XX @@ static void coroutine_fn test_job_start(void *opaque)
      }
      job_defer_to_main_loop(&s->common.job, test_job_completed, NULL);
 +    return 0;
  }
  static void test_job_complete(Job *job, Error **errp)
@@ -XXX,XX +XXX,XX @@ BlockJobDriver test_job_driver = {
          .free           = block_job_free,
          .user_resume    = block_job_user_resume,
          .drain          = block_job_drain,
 -        .start          = test_job_start,
 +        .run            = test_job_run,
          .complete       = test_job_complete,
      },
  };
 diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
 index XXXXXXX..XXXXXXX 100644
 --- a/tests/test-blockjob-txn.c
 +++ b/tests/test-blockjob-txn.c
@@ -XXX,XX +XXX,XX @@ static void test_block_job_complete(Job *job, void *opaque)
      bdrv_unref(bs);
  }
 -static void coroutine_fn test_block_job_run(void *opaque)
 +static int coroutine_fn test_block_job_run(Job *job, Error **errp)
  {
 -    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;
          }
      }
 -    job_defer_to_main_loop(&job->job, test_block_job_complete,
 +    job_defer_to_main_loop(job, test_block_job_complete,
                             (void *)(intptr_t)s->rc);
 +    return s->rc;
  }
  typedef struct {
@@ -XXX,XX +XXX,XX @@ static const BlockJobDriver test_block_job_driver = {
          .free          = block_job_free,
          .user_resume   = block_job_user_resume,
          .drain         = block_job_drain,
 -        .start         = test_block_job_run,
 +        .run           = test_block_job_run,
      },
  };
 diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
 index XXXXXXX..XXXXXXX 100644
 --- a/tests/test-blockjob.c
 +++ b/tests/test-blockjob.c
@@ -XXX,XX +XXX,XX @@ 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_run(Job *job, Error **errp)
  {
 -    CancelJob *s = opaque;
 +    CancelJob *s = container_of(job, CancelJob, common.job);
      while (!s->should_complete) {
          if (job_is_cancelled(&s->common.job)) {
@@ -XXX,XX +XXX,XX @@ static void coroutine_fn cancel_job_start(void *opaque)
   defer:
      job_defer_to_main_loop(&s->common.job, cancel_job_completed, s);
 +    return 0;
  }
  static const BlockJobDriver test_cancel_driver = {
@@ -XXX,XX +XXX,XX @@ static const BlockJobDriver test_cancel_driver = {
          .free          = block_job_free,
          .user_resume   = block_job_user_resume,
          .drain         = block_job_drain,
 -        .start         = cancel_job_start,
 +        .run           = cancel_job_run,
          .complete      = cancel_job_complete,
      },
  };
 --
 .17.1

-New patch
+[Qemu-devel] [PULL v2 03/10] jobs: canonize Error object
+From: John Snow <jsnow@redhat.com>
 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. The integer error code for job_completed is kept for now,
 to be removed shortly in a separate patch.
 Signed-off-by: John Snow <jsnow@redhat.com>
 Message-id: 20180830015734.19765-3-jsnow@redhat.com
 [mreitz: Dropped a superfluous g_strdup()]
 Reviewed-by: Eric Blake <eblake@redhat.com>
 Signed-off-by: Max Reitz <mreitz@redhat.com>
 ---
  include/qemu/job.h        | 14 ++++++++------
  block/backup.c            |  2 +-
  block/commit.c            |  2 +-
  block/create.c            |  5 ++---
  block/mirror.c            |  2 +-
  block/stream.c            |  2 +-
  job-qmp.c                 |  5 +++--
  job.c                     | 18 ++++++------------
  tests/test-bdrv-drain.c   |  2 +-
  tests/test-blockjob-txn.c |  2 +-
  tests/test-blockjob.c     |  2 +-
 files changed, 26 insertions(+), 30 deletions(-)
 diff --git a/include/qemu/job.h b/include/qemu/job.h
 index XXXXXXX..XXXXXXX 100644
 --- a/include/qemu/job.h
 +++ b/include/qemu/job.h
@@ -XXX,XX +XXX,XX @@ 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.
 +     * If job->ret is nonzero and an error object was not set, it will be set
 +     * to strerror(-job->ret) during job_completed.
 +     */
 +    Error *err;
 +
      /** The completion function that will be called when the job completes.  */
      BlockCompletionFunc *cb;
@@ -XXX,XX +XXX,XX @@ 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/block/backup.c b/block/backup.c
 index XXXXXXX..XXXXXXX 100644
 --- a/block/backup.c
 +++ b/block/backup.c
@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644
 --- a/block/commit.c
 +++ b/block/commit.c
@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644
 --- a/block/create.c
 +++ b/block/create.c
@@ -XXX,XX +XXX,XX @@ 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 int coroutine_fn blockdev_create_run(Job *job, Error **errp)
@@ -XXX,XX +XXX,XX @@ static int coroutine_fn blockdev_create_run(Job *job, Error **errp)
      BlockdevCreateJob *s = container_of(job, BlockdevCreateJob, common);
      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, errp);
      job_progress_update(&s->common, 1);
      qapi_free_BlockdevCreateOptions(s->opts);
 diff --git a/block/mirror.c b/block/mirror.c
 index XXXXXXX..XXXXXXX 100644
 --- a/block/mirror.c
 +++ b/block/mirror.c
@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644
 --- a/block/stream.c
 +++ b/block/stream.c
@@ -XXX,XX +XXX,XX @@ out:
      }
      g_free(s->backing_file_str);
 -    job_completed(job, data->ret, NULL);
 +    job_completed(job, data->ret);
      g_free(data);
  }
 diff --git a/job-qmp.c b/job-qmp.c
 index XXXXXXX..XXXXXXX 100644
 --- a/job-qmp.c
 +++ b/job-qmp.c
@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644
 --- a/job.c
 +++ b/job.c
@@ -XXX,XX +XXX,XX @@ 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);
      }
@@ -XXX,XX +XXX,XX @@ static void coroutine_fn job_co_entry(void *opaque)
      assert(job && job->driver && job->driver->run);
      job_pause_point(job);
 -    job->ret = job->driver->run(job, NULL);
 +    job->ret = job->driver->run(job, &job->err);
  }
@@ -XXX,XX +XXX,XX @@ 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(&job->err, "%s", strerror(-job->ret));
          }
          job_state_transition(job, JOB_STATUS_ABORTING);
      }
@@ -XXX,XX +XXX,XX @@ 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) {
@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644
 --- a/tests/test-bdrv-drain.c
 +++ b/tests/test-bdrv-drain.c
@@ -XXX,XX +XXX,XX @@ typedef struct TestBlockJob {
  static void test_job_completed(Job *job, void *opaque)
  {
 -    job_completed(job, 0, NULL);
 +    job_completed(job, 0);
  }
  static int coroutine_fn test_job_run(Job *job, Error **errp)
 diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
 index XXXXXXX..XXXXXXX 100644
 --- a/tests/test-blockjob-txn.c
 +++ b/tests/test-blockjob-txn.c
@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644
 --- a/tests/test-blockjob.c
 +++ b/tests/test-blockjob.c
@@ -XXX,XX +XXX,XX @@ 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)
 --
 .17.1

-New patch
+[Qemu-devel] [PULL v2 04/10] jobs: add exit shim
+From: John Snow <jsnow@redhat.com>
+All 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
+Few jobs do anything beyond exactly this. Consolidate this exit
+logic for a net reduction in SLOC.
+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.
+You can observe this in practice by looking at mirror_exit's careful
+placement of job_completed and bdrv_unref calls.
+If we centralize job exiting, we can signal job completion from outside
+of the aio_context, which should allow for job cleanup code to run with
+only one lock, which makes cleanup callbacks less tricky to write.
+Signed-off-by: John Snow <jsnow@redhat.com>
+Reviewed-by: Max Reitz <mreitz@redhat.com>
+Message-id: 20180830015734.19765-4-jsnow@redhat.com
+Reviewed-by: Jeff Cody <jcody@redhat.com>
+Signed-off-by: Max Reitz <mreitz@redhat.com>
+---
+ include/qemu/job.h | 11 +++++++++++
+ job.c              | 18 ++++++++++++++++++
+files changed, 29 insertions(+)
+diff --git a/include/qemu/job.h b/include/qemu/job.h
+index XXXXXXX..XXXXXXX 100644
+--- a/include/qemu/job.h
++++ b/include/qemu/job.h
+@@ -XXX,XX +XXX,XX @@ 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.
++     *
++     * FIXME TODO: This callback is only temporary to transition remaining jobs
++     * to prepare/commit/abort/clean callbacks and will be removed before 3.1.
++     * is released.
++     */
++    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 XXXXXXX..XXXXXXX 100644
+--- a/job.c
++++ b/job.c
+@@ -XXX,XX +XXX,XX @@ 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
+@@ -XXX,XX +XXX,XX @@ static void coroutine_fn job_co_entry(void *opaque)
+     assert(job && job->driver && job->driver->run);
+     job_pause_point(job);
+     job->ret = job->driver->run(job, &job->err);
++    if (!job->deferred_to_main_loop) {
++        job->deferred_to_main_loop = true;
++        aio_bh_schedule_oneshot(qemu_get_aio_context(),
++                                job_exit,
++                                job);
++    }
+ }
+--
+.17.1

-New patch
+[Qemu-devel] [PULL v2 05/10] block/commit: utilize job_exit shim
+From: John Snow <jsnow@redhat.com>
+Change the manual deferment to commit_complete into the implicit
+callback to job_exit, renaming commit_complete to commit_exit.
+This conversion does change the timing of when job_completed is
+called to after the bdrv_replace_node and bdrv_unref calls, which
+could have implications for bjob->blk which will now be put down
+after this cleanup.
+Kevin highlights that we did not take any permissions for that backend
+at job creation time, so it is safe to reorder these operations.
+Signed-off-by: John Snow <jsnow@redhat.com>
+Reviewed-by: Max Reitz <mreitz@redhat.com>
+Message-id: 20180830015734.19765-5-jsnow@redhat.com
+Reviewed-by: Jeff Cody <jcody@redhat.com>
+Signed-off-by: Max Reitz <mreitz@redhat.com>
+---
+ block/commit.c | 22 +++++-----------------
+file changed, 5 insertions(+), 17 deletions(-)
+diff --git a/block/commit.c b/block/commit.c
+index XXXXXXX..XXXXXXX 100644
+--- a/block/commit.c
++++ b/block/commit.c
+@@ -XXX,XX +XXX,XX @@ 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;
+     bool remove_commit_top_bs = false;
+     /* Make sure commit_top_bs and top stay around until bdrv_replace_node() */
+@@ -XXX,XX +XXX,XX @@ static void commit_complete(Job *job, void *opaque)
+      * the normal backing chain can be restored. */
+     blk_unref(s->base);
+-    if (!job_is_cancelled(job) && ret == 0) {
++    if (!job_is_cancelled(job) && job->ret == 0) {
+         /* success */
+-        ret = bdrv_drop_intermediate(s->commit_top_bs, base,
+-                                     s->backing_file_str);
++        job->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
+@@ -XXX,XX +XXX,XX @@ 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.  */
+@@ -XXX,XX +XXX,XX @@ static void commit_complete(Job *job, void *opaque)
+ static int coroutine_fn commit_run(Job *job, Error **errp)
+ {
+     CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
+-    CommitCompleteData *data;
+     int64_t offset;
+     uint64_t delay_ns = 0;
+     int ret = 0;
+@@ -XXX,XX +XXX,XX @@ static int coroutine_fn commit_run(Job *job, Error **errp)
+ out:
+     qemu_vfree(buf);
+-    data = g_malloc(sizeof(*data));
+-    data->ret = ret;
+-    job_defer_to_main_loop(&s->common.job, commit_complete, data);
+     return ret;
+ }
+@@ -XXX,XX +XXX,XX @@ static const BlockJobDriver commit_job_driver = {
+         .user_resume   = block_job_user_resume,
+         .drain         = block_job_drain,
+         .run           = commit_run,
++        .exit          = commit_exit,
+     },
+ };
+--
+.17.1

-[PULL 3/3] util/event-loop-base: Introduce options to set the thread pool size
+[Qemu-devel] [PULL v2 06/10] block/mirror: utilize job_exit shim
-From: Nicolas Saenz Julienne <nsaenzju@redhat.com>
+From: John Snow <jsnow@redhat.com>
-The thread pool regulates itself: when idle, it kills threads until
+Change the manual deferment to mirror_exit into the implicit
-empty, when in demand, it creates new threads until full. This behaviour
+callback to job_exit and the mirror_exit callback.
 doesn't play well with latency sensitive workloads where the price of
 creating a new thread is too high. For example, when paired with qemu's
 '-mlock', or using safety features like SafeStack, creating a new thread
 has been measured take multiple milliseconds.
-In order to mitigate this let's introduce a new 'EventLoopBase'
+This does change the order of some bdrv_unref calls and job_completed,
-property to set the thread pool size. The threads will be created during
+but thanks to the new context in which we call .exit, this is safe to
-the pool's initialization or upon updating the property's value, remain
+defer the possible flushing of any nodes to the job_finalize_single
-available during its lifetime regardless of demand, and destroyed upon
+cleanup stage.
 freeing it. A properly characterized workload will then be able to
 configure the pool to avoid any latency spikes.
-Signed-off-by: Nicolas Saenz Julienne <nsaenzju@redhat.com>
+Signed-off-by: John Snow <jsnow@redhat.com>
-Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
+Message-id: 20180830015734.19765-6-jsnow@redhat.com
-Acked-by: Markus Armbruster <armbru@redhat.com>
+Reviewed-by: Max Reitz <mreitz@redhat.com>
-Message-id: 20220425075723.20019-4-nsaenzju@redhat.com
+Reviewed-by: Jeff Cody <jcody@redhat.com>
-Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
+Signed-off-by: Max Reitz <mreitz@redhat.com>
 ---
- qapi/qom.json                    | 10 +++++-
+ block/mirror.c | 29 +++++++++++------------------
- include/block/aio.h              | 10 ++++++
+file changed, 11 insertions(+), 18 deletions(-)
  include/block/thread-pool.h      |  3 ++
  include/sysemu/event-loop-base.h |  4 +++
  event-loop-base.c                | 23 +++++++++++++
  iothread.c                       |  3 ++
  util/aio-posix.c                 |  1 +
  util/async.c                     | 20 ++++++++++++
  util/main-loop.c                 |  9 ++++++
  util/thread-pool.c               | 55 +++++++++++++++++++++++++++++---
 files changed, 133 insertions(+), 5 deletions(-)
-diff --git a/qapi/qom.json b/qapi/qom.json
+diff --git a/block/mirror.c b/block/mirror.c
 index XXXXXXX..XXXXXXX 100644
---- a/qapi/qom.json
+--- a/block/mirror.c
-+++ b/qapi/qom.json
++++ b/block/mirror.c
-@@ -XXX,XX +XXX,XX @@
+@@ -XXX,XX +XXX,XX @@ static void mirror_wait_for_all_io(MirrorBlockJob *s)
- #                 0 means that the engine will use its default.
+     }
- #                 (default: 0)
+ }
- #
-+# @thread-pool-min: minimum number of threads reserved in the thread pool
+-typedef struct {
-+#                   (default:0)
+-    int ret;
-+#
+-} MirrorExitData;
-+# @thread-pool-max: maximum number of threads the thread pool can contain
+-
-+#                   (default:64)
+-static void mirror_exit(Job *job, void *opaque)
-+#
++static void mirror_exit(Job *job)
- # Since: 7.1
+ {
- ##
+     MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
- { 'struct': 'EventLoopBaseProperties',
+     BlockJob *bjob = &s->common;
--  'data': { '*aio-max-batch': 'int' } }
+-    MirrorExitData *data = opaque;
-+  'data': { '*aio-max-batch': 'int',
+     MirrorBDSOpaque *bs_opaque = s->mirror_top_bs->opaque;
-+            '*thread-pool-min': 'int',
+     AioContext *replace_aio_context = NULL;
-+            '*thread-pool-max': 'int' } }
+     BlockDriverState *src = s->mirror_top_bs->backing->bs;
+     BlockDriverState *target_bs = blk_bs(s->target);
- ##
+     BlockDriverState *mirror_top_bs = s->mirror_top_bs;
- # @IothreadProperties:
+     Error *local_err = NULL;
-diff --git a/include/block/aio.h b/include/block/aio.h
++    int ret = job->ret;
-index XXXXXXX..XXXXXXX 100644
---- a/include/block/aio.h
+     bdrv_release_dirty_bitmap(src, s->dirty_bitmap);
-+++ b/include/block/aio.h
-@@ -XXX,XX +XXX,XX @@ struct AioContext {
+-    /* Make sure that the source BDS doesn't go away before we called
-     QSLIST_HEAD(, Coroutine) scheduled_coroutines;
+-     * job_completed(). */
-     QEMUBH *co_schedule_bh;
++    /* Make sure that the source BDS doesn't go away during bdrv_replace_node,
++     * before we can call bdrv_drained_end */
-+    int thread_pool_min;
+     bdrv_ref(src);
-+    int thread_pool_max;
+     bdrv_ref(mirror_top_bs);
-     /* Thread pool for performing work and receiving completion callbacks.
+     bdrv_ref(target_bs);
-      * Has its own locking.
+@@ -XXX,XX +XXX,XX @@ static void mirror_exit(Job *job, void *opaque)
-      */
+             bdrv_set_backing_hd(target_bs, backing, &local_err);
-@@ -XXX,XX +XXX,XX @@ void aio_context_set_poll_params(AioContext *ctx, int64_t max_ns,
+             if (local_err) {
- void aio_context_set_aio_params(AioContext *ctx, int64_t max_batch,
+                 error_report_err(local_err);
-                                 Error **errp);
+-                data->ret = -EPERM;
++                ret = -EPERM;
-+/**
+             }
-+ * aio_context_set_thread_pool_params:
+         }
-+ * @ctx: the aio context
+     }
-+ * @min: min number of threads to have readily available in the thread pool
+@@ -XXX,XX +XXX,XX @@ static void mirror_exit(Job *job, void *opaque)
-+ * @min: max number of threads the thread pool can contain
+         aio_context_acquire(replace_aio_context);
-+ */
+     }
-+void aio_context_set_thread_pool_params(AioContext *ctx, int64_t min,
-+                                        int64_t max, Error **errp);
+-    if (s->should_complete && data->ret == 0) {
- #endif
++    if (s->should_complete && ret == 0) {
-diff --git a/include/block/thread-pool.h b/include/block/thread-pool.h
+         BlockDriverState *to_replace = src;
-index XXXXXXX..XXXXXXX 100644
+         if (s->to_replace) {
---- a/include/block/thread-pool.h
+             to_replace = s->to_replace;
-+++ b/include/block/thread-pool.h
+@@ -XXX,XX +XXX,XX @@ static void mirror_exit(Job *job, void *opaque)
-@@ -XXX,XX +XXX,XX @@
+         bdrv_drained_end(target_bs);
+         if (local_err) {
- #include "block/block.h"
+             error_report_err(local_err);
+-            data->ret = -EPERM;
-+#define THREAD_POOL_MAX_THREADS_DEFAULT         64
++            ret = -EPERM;
          }
      }
      if (s->to_replace) {
@@ -XXX,XX +XXX,XX @@ 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);
 +
- typedef int ThreadPoolFunc(void *opaque);
++    job->ret = ret;
  typedef struct ThreadPool ThreadPool;
@@ -XXX,XX +XXX,XX @@ BlockAIOCB *thread_pool_submit_aio(ThreadPool *pool,
  int coroutine_fn thread_pool_submit_co(ThreadPool *pool,
          ThreadPoolFunc *func, void *arg);
  void thread_pool_submit(ThreadPool *pool, ThreadPoolFunc *func, void *arg);
 +void thread_pool_update_params(ThreadPool *pool, struct AioContext *ctx);
  #endif
 diff --git a/include/sysemu/event-loop-base.h b/include/sysemu/event-loop-base.h
 index XXXXXXX..XXXXXXX 100644
 --- a/include/sysemu/event-loop-base.h
 +++ b/include/sysemu/event-loop-base.h
@@ -XXX,XX +XXX,XX @@ struct EventLoopBase {
      /* AioContext AIO engine parameters */
      int64_t aio_max_batch;
 +
 +    /* AioContext thread pool parameters */
 +    int64_t thread_pool_min;
 +    int64_t thread_pool_max;
  };
  #endif
 diff --git a/event-loop-base.c b/event-loop-base.c
 index XXXXXXX..XXXXXXX 100644
 --- a/event-loop-base.c
 +++ b/event-loop-base.c
@@ -XXX,XX +XXX,XX @@
  #include "qemu/osdep.h"
  #include "qom/object_interfaces.h"
  #include "qapi/error.h"
 +#include "block/thread-pool.h"
  #include "sysemu/event-loop-base.h"
  typedef struct {
@@ -XXX,XX +XXX,XX @@ typedef struct {
      ptrdiff_t offset; /* field's byte offset in EventLoopBase struct */
  } EventLoopBaseParamInfo;
 +static void event_loop_base_instance_init(Object *obj)
 +{
 +    EventLoopBase *base = EVENT_LOOP_BASE(obj);
 +
 +    base->thread_pool_max = THREAD_POOL_MAX_THREADS_DEFAULT;
 +}
 +
  static EventLoopBaseParamInfo aio_max_batch_info = {
      "aio-max-batch", offsetof(EventLoopBase, aio_max_batch),
  };
 +static EventLoopBaseParamInfo thread_pool_min_info = {
 +    "thread-pool-min", offsetof(EventLoopBase, thread_pool_min),
 +};
 +static EventLoopBaseParamInfo thread_pool_max_info = {
 +    "thread-pool-max", offsetof(EventLoopBase, thread_pool_max),
 +};
  static void event_loop_base_get_param(Object *obj, Visitor *v,
          const char *name, void *opaque, Error **errp)
@@ -XXX,XX +XXX,XX @@ static void event_loop_base_class_init(ObjectClass *klass, void *class_data)
                                event_loop_base_get_param,
                                event_loop_base_set_param,
                                NULL, &aio_max_batch_info);
 +    object_class_property_add(klass, "thread-pool-min", "int",
 +                              event_loop_base_get_param,
 +                              event_loop_base_set_param,
 +                              NULL, &thread_pool_min_info);
 +    object_class_property_add(klass, "thread-pool-max", "int",
 +                              event_loop_base_get_param,
 +                              event_loop_base_set_param,
 +                              NULL, &thread_pool_max_info);
  }
- static const TypeInfo event_loop_base_info = {
+ static void mirror_throttle(MirrorBlockJob *s)
-     .name = TYPE_EVENT_LOOP_BASE,
+@@ -XXX,XX +XXX,XX @@ static int mirror_flush(MirrorBlockJob *s)
-     .parent = TYPE_OBJECT,
+ static int coroutine_fn mirror_run(Job *job, Error **errp)
-     .instance_size = sizeof(EventLoopBase),
+ {
-+    .instance_init = event_loop_base_instance_init,
+     MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
-     .class_size = sizeof(EventLoopBaseClass),
+-    MirrorExitData *data;
-     .class_init = event_loop_base_class_init,
+     BlockDriverState *bs = s->mirror_top_bs->backing->bs;
-     .abstract = true,
+     BlockDriverState *target_bs = blk_bs(s->target);
-diff --git a/iothread.c b/iothread.c
+     bool need_drain = true;
-index XXXXXXX..XXXXXXX 100644
+@@ -XXX,XX +XXX,XX @@ immediate_exit:
---- a/iothread.c
+     g_free(s->in_flight_bitmap);
-+++ b/iothread.c
+     bdrv_dirty_iter_free(s->dbi);
-@@ -XXX,XX +XXX,XX @@ static void iothread_set_aio_context_params(EventLoopBase *base, Error **errp)
-     aio_context_set_aio_params(iothread->ctx,
+-    data = g_malloc(sizeof(*data));
-                                iothread->parent_obj.aio_max_batch,
+-    data->ret = ret;
-                                errp);
+-
-+
+     if (need_drain) {
-+    aio_context_set_thread_pool_params(iothread->ctx, base->thread_pool_min,
+         bdrv_drained_begin(bs);
-+                                       base->thread_pool_max, errp);
+     }
 -    job_defer_to_main_loop(&s->common.job, mirror_exit, data);
      return ret;
  }
+@@ -XXX,XX +XXX,XX @@ static const BlockJobDriver mirror_job_driver = {
-diff --git a/util/aio-posix.c b/util/aio-posix.c
+         .user_resume            = block_job_user_resume,
-index XXXXXXX..XXXXXXX 100644
+         .drain                  = block_job_drain,
---- a/util/aio-posix.c
+         .run                    = mirror_run,
-+++ b/util/aio-posix.c
++        .exit                   = mirror_exit,
-@@ -XXX,XX +XXX,XX @@
+         .pause                  = mirror_pause,
+         .complete               = mirror_complete,
- #include "qemu/osdep.h"
+     },
- #include "block/block.h"
+@@ -XXX,XX +XXX,XX @@ static const BlockJobDriver commit_active_job_driver = {
-+#include "block/thread-pool.h"
+         .user_resume            = block_job_user_resume,
- #include "qemu/main-loop.h"
+         .drain                  = block_job_drain,
- #include "qemu/rcu.h"
+         .run                    = mirror_run,
- #include "qemu/rcu_queue.h"
++        .exit                   = mirror_exit,
-diff --git a/util/async.c b/util/async.c
+         .pause                  = mirror_pause,
-index XXXXXXX..XXXXXXX 100644
+         .complete               = mirror_complete,
---- a/util/async.c
+     },
 +++ b/util/async.c
@@ -XXX,XX +XXX,XX @@ AioContext *aio_context_new(Error **errp)
      ctx->aio_max_batch = 0;
 +    ctx->thread_pool_min = 0;
 +    ctx->thread_pool_max = THREAD_POOL_MAX_THREADS_DEFAULT;
 +
      return ctx;
  fail:
      g_source_destroy(&ctx->source);
@@ -XXX,XX +XXX,XX @@ void qemu_set_current_aio_context(AioContext *ctx)
      assert(!get_my_aiocontext());
      set_my_aiocontext(ctx);
  }
 +
 +void aio_context_set_thread_pool_params(AioContext *ctx, int64_t min,
 +                                        int64_t max, Error **errp)
 +{
 +
 +    if (min > max || !max || min > INT_MAX || max > INT_MAX) {
 +        error_setg(errp, "bad thread-pool-min/thread-pool-max values");
 +        return;
 +    }
 +
 +    ctx->thread_pool_min = min;
 +    ctx->thread_pool_max = max;
 +
 +    if (ctx->thread_pool) {
 +        thread_pool_update_params(ctx->thread_pool, ctx);
 +    }
 +}
 diff --git a/util/main-loop.c b/util/main-loop.c
 index XXXXXXX..XXXXXXX 100644
 --- a/util/main-loop.c
 +++ b/util/main-loop.c
@@ -XXX,XX +XXX,XX @@
  #include "sysemu/replay.h"
  #include "qemu/main-loop.h"
  #include "block/aio.h"
 +#include "block/thread-pool.h"
  #include "qemu/error-report.h"
  #include "qemu/queue.h"
  #include "qemu/compiler.h"
@@ -XXX,XX +XXX,XX @@ int qemu_init_main_loop(Error **errp)
  static void main_loop_update_params(EventLoopBase *base, Error **errp)
  {
 +    ERRP_GUARD();
 +
      if (!qemu_aio_context) {
          error_setg(errp, "qemu aio context not ready");
          return;
      }
      aio_context_set_aio_params(qemu_aio_context, base->aio_max_batch, errp);
 +    if (*errp) {
 +        return;
 +    }
 +
 +    aio_context_set_thread_pool_params(qemu_aio_context, base->thread_pool_min,
 +                                       base->thread_pool_max, errp);
  }
  MainLoop *mloop;
 diff --git a/util/thread-pool.c b/util/thread-pool.c
 index XXXXXXX..XXXXXXX 100644
 --- a/util/thread-pool.c
 +++ b/util/thread-pool.c
@@ -XXX,XX +XXX,XX @@ struct ThreadPool {
      QemuMutex lock;
      QemuCond worker_stopped;
      QemuSemaphore sem;
 -    int max_threads;
      QEMUBH *new_thread_bh;
      /* The following variables are only accessed from one AioContext. */
@@ -XXX,XX +XXX,XX @@ struct ThreadPool {
      int new_threads;     /* backlog of threads we need to create */
      int pending_threads; /* threads created but not running yet */
      bool stopping;
 +    int min_threads;
 +    int max_threads;
  };
 +static inline bool back_to_sleep(ThreadPool *pool, int ret)
 +{
 +    /*
 +     * The semaphore timed out, we should exit the loop except when:
 +     *  - There is work to do, we raced with the signal.
 +     *  - The max threads threshold just changed, we raced with the signal.
 +     *  - The thread pool forces a minimum number of readily available threads.
 +     */
 +    if (ret == -1 && (!QTAILQ_EMPTY(&pool->request_list) ||
 +            pool->cur_threads > pool->max_threads ||
 +            pool->cur_threads <= pool->min_threads)) {
 +            return true;
 +    }
 +
 +    return false;
 +}
 +
  static void *worker_thread(void *opaque)
  {
      ThreadPool *pool = opaque;
@@ -XXX,XX +XXX,XX @@ static void *worker_thread(void *opaque)
              ret = qemu_sem_timedwait(&pool->sem, 10000);
              qemu_mutex_lock(&pool->lock);
              pool->idle_threads--;
 -        } while (ret == -1 && !QTAILQ_EMPTY(&pool->request_list));
 -        if (ret == -1 || pool->stopping) {
 +        } while (back_to_sleep(pool, ret));
 +        if (ret == -1 || pool->stopping ||
 +            pool->cur_threads > pool->max_threads) {
              break;
          }
@@ -XXX,XX +XXX,XX @@ void thread_pool_submit(ThreadPool *pool, ThreadPoolFunc *func, void *arg)
      thread_pool_submit_aio(pool, func, arg, NULL, NULL);
  }
 +void thread_pool_update_params(ThreadPool *pool, AioContext *ctx)
 +{
 +    qemu_mutex_lock(&pool->lock);
 +
 +    pool->min_threads = ctx->thread_pool_min;
 +    pool->max_threads = ctx->thread_pool_max;
 +
 +    /*
 +     * We either have to:
 +     *  - Increase the number available of threads until over the min_threads
 +     *    threshold.
 +     *  - Decrease the number of available threads until under the max_threads
 +     *    threshold.
 +     *  - Do nothing. The current number of threads fall in between the min and
 +     *    max thresholds. We'll let the pool manage itself.
 +     */
 +    for (int i = pool->cur_threads; i < pool->min_threads; i++) {
 +        spawn_thread(pool);
 +    }
 +
 +    for (int i = pool->cur_threads; i > pool->max_threads; i--) {
 +        qemu_sem_post(&pool->sem);
 +    }
 +
 +    qemu_mutex_unlock(&pool->lock);
 +}
 +
  static void thread_pool_init_one(ThreadPool *pool, AioContext *ctx)
  {
      if (!ctx) {
@@ -XXX,XX +XXX,XX @@ static void thread_pool_init_one(ThreadPool *pool, AioContext *ctx)
      qemu_mutex_init(&pool->lock);
      qemu_cond_init(&pool->worker_stopped);
      qemu_sem_init(&pool->sem, 0);
 -    pool->max_threads = 64;
      pool->new_thread_bh = aio_bh_new(ctx, spawn_thread_bh_fn, pool);
      QLIST_INIT(&pool->head);
      QTAILQ_INIT(&pool->request_list);
 +
 +    thread_pool_update_params(pool, ctx);
  }
  ThreadPool *thread_pool_new(AioContext *ctx)
 --
-.35.1
+.17.1

-[PULL 2/3] util/main-loop: Introduce the main loop into QOM
+[Qemu-devel] [PULL v2 07/10] jobs: utilize job_exit shim
-From: Nicolas Saenz Julienne <nsaenzju@redhat.com>
+From: John Snow <jsnow@redhat.com>
-'event-loop-base' provides basic property handling for all 'AioContext'
+Utilize the job_exit shim by not calling job_defer_to_main_loop, and
-based event loops. So let's define a new 'MainLoopClass' that inherits
+where applicable, converting the deferred callback into the job_exit
-from it. This will permit tweaking the main loop's properties through
+callback.
-qapi as well as through the command line using the '-object' keyword[1].
-Only one instance of 'MainLoopClass' might be created at any time.
+This converts backup, stream, create, and the unit tests all at once.
+Most of these jobs do not see any changes to the order in which they
-'EventLoopBaseClass' learns a new callback, 'can_be_deleted()' so as to
+clean up their resources, except the test-blockjob-txn test, which
-mark 'MainLoop' as non-deletable.
+now puts down its bs before job_completed is called.
-[1] For example:
+This is safe for the same reason the reordering in the mirror job is
-      -object main-loop,id=main-loop,aio-max-batch=<value>
+safe, because job_completed no longer runs under two locks, making
+the unref safe even if it causes a flush.
-Signed-off-by: Nicolas Saenz Julienne <nsaenzju@redhat.com>
-Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
+Signed-off-by: John Snow <jsnow@redhat.com>
-Acked-by: Markus Armbruster <armbru@redhat.com>
+Reviewed-by: Max Reitz <mreitz@redhat.com>
-Message-id: 20220425075723.20019-3-nsaenzju@redhat.com
+Message-id: 20180830015734.19765-7-jsnow@redhat.com
-Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
+Signed-off-by: Max Reitz <mreitz@redhat.com>
 ---
- qapi/qom.json                    | 13 ++++++++
+ block/backup.c            | 16 ----------------
- meson.build                      |  3 +-
+ block/create.c            | 14 +++-----------
- include/qemu/main-loop.h         | 10 ++++++
+ block/stream.c            | 22 +++++++---------------
- include/sysemu/event-loop-base.h |  1 +
+ tests/test-bdrv-drain.c   |  6 ------
- event-loop-base.c                | 13 ++++++++
+ tests/test-blockjob-txn.c | 11 ++---------
- util/main-loop.c                 | 56 ++++++++++++++++++++++++++++++++
+ tests/test-blockjob.c     | 10 ++++------
-files changed, 95 insertions(+), 1 deletion(-)
+files changed, 16 insertions(+), 63 deletions(-)
-diff --git a/qapi/qom.json b/qapi/qom.json
+diff --git a/block/backup.c b/block/backup.c
 index XXXXXXX..XXXXXXX 100644
---- a/qapi/qom.json
+--- a/block/backup.c
-+++ b/qapi/qom.json
++++ b/block/backup.c
-@@ -XXX,XX +XXX,XX @@
+@@ -XXX,XX +XXX,XX @@ static BlockErrorAction backup_error_action(BackupBlockJob *job,
-             '*poll-grow': 'int',
+     }
-             '*poll-shrink': 'int' } }
+ }
-+##
+-typedef struct {
-+# @MainLoopProperties:
+-    int ret;
-+#
+-} BackupCompleteData;
-+# Properties for the main-loop object.
+-
-+#
+-static void backup_complete(Job *job, void *opaque)
-+# Since: 7.1
+-{
-+##
+-    BackupCompleteData *data = opaque;
-+{ 'struct': 'MainLoopProperties',
+-
-+  'base': 'EventLoopBaseProperties',
+-    job_completed(job, data->ret);
-+  'data': {} }
+-    g_free(data);
-+
+-}
- ##
+-
- # @MemoryBackendProperties:
+ static bool coroutine_fn yield_and_check(BackupBlockJob *job)
- #
+ {
-@@ -XXX,XX +XXX,XX @@
+     uint64_t delay_ns;
-     { 'name': 'input-linux',
+@@ -XXX,XX +XXX,XX @@ static void backup_incremental_init_copy_bitmap(BackupBlockJob *job)
-       'if': 'CONFIG_LINUX' },
+ static int coroutine_fn backup_run(Job *opaque_job, Error **errp)
-     'iothread',
+ {
-+    'main-loop',
+     BackupBlockJob *job = container_of(opaque_job, BackupBlockJob, common.job);
-     { 'name': 'memory-backend-epc',
+-    BackupCompleteData *data;
-       'if': 'CONFIG_LINUX' },
+     BlockDriverState *bs = blk_bs(job->common.blk);
-     'memory-backend-file',
+     int64_t offset, nb_clusters;
-@@ -XXX,XX +XXX,XX @@
+     int ret = 0;
-       'input-linux':                { 'type': 'InputLinuxProperties',
+@@ -XXX,XX +XXX,XX @@ static int coroutine_fn backup_run(Job *opaque_job, Error **errp)
-                                       'if': 'CONFIG_LINUX' },
+     qemu_co_rwlock_unlock(&job->flush_rwlock);
-       'iothread':                   'IothreadProperties',
+     hbitmap_free(job->copy_bitmap);
-+      'main-loop':                  'MainLoopProperties',
-       'memory-backend-epc':         { 'type': 'MemoryBackendEpcProperties',
+-    data = g_malloc(sizeof(*data));
-                                       'if': 'CONFIG_LINUX' },
+-    data->ret = ret;
-       'memory-backend-file':        'MemoryBackendFileProperties',
+-    job_defer_to_main_loop(&job->common.job, backup_complete, data);
-diff --git a/meson.build b/meson.build
+     return ret;
-index XXXXXXX..XXXXXXX 100644
+ }
---- a/meson.build
-+++ b/meson.build
+diff --git a/block/create.c b/block/create.c
-@@ -XXX,XX +XXX,XX @@ libqemuutil = static_library('qemuutil',
+index XXXXXXX..XXXXXXX 100644
-                              sources: util_ss.sources() + stub_ss.sources() + genh,
+--- a/block/create.c
-                              dependencies: [util_ss.dependencies(), libm, threads, glib, socket, malloc, pixman])
++++ b/block/create.c
- qemuutil = declare_dependency(link_with: libqemuutil,
+@@ -XXX,XX +XXX,XX @@ typedef struct BlockdevCreateJob {
--                              sources: genh + version_res)
+     Job common;
-+                              sources: genh + version_res,
+     BlockDriver *drv;
-+                              dependencies: [event_loop_base])
+     BlockdevCreateOptions *opts;
+-    int ret;
- if have_system or have_user
+ } BlockdevCreateJob;
-   decodetree = generator(find_program('scripts/decodetree.py'),
-diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
+-static void blockdev_create_complete(Job *job, void *opaque)
-index XXXXXXX..XXXXXXX 100644
+-{
---- a/include/qemu/main-loop.h
+-    BlockdevCreateJob *s = container_of(job, BlockdevCreateJob, common);
-+++ b/include/qemu/main-loop.h
+-
-@@ -XXX,XX +XXX,XX @@
+-    job_completed(job, s->ret);
- #define QEMU_MAIN_LOOP_H
+-}
+-
- #include "block/aio.h"
+ static int coroutine_fn blockdev_create_run(Job *job, Error **errp)
-+#include "qom/object.h"
+ {
-+#include "sysemu/event-loop-base.h"
+     BlockdevCreateJob *s = container_of(job, BlockdevCreateJob, common);
++    int ret;
- #define SIG_IPI SIGUSR1
+     job_progress_set_remaining(&s->common, 1);
-+#define TYPE_MAIN_LOOP  "main-loop"
+-    s->ret = s->drv->bdrv_co_create(s->opts, errp);
-+OBJECT_DECLARE_TYPE(MainLoop, MainLoopClass, MAIN_LOOP)
++    ret = s->drv->bdrv_co_create(s->opts, errp);
-+
+     job_progress_update(&s->common, 1);
-+struct MainLoop {
-+    EventLoopBase parent_obj;
+     qapi_free_BlockdevCreateOptions(s->opts);
-+};
+-    job_defer_to_main_loop(&s->common, blockdev_create_complete, NULL);
-+typedef struct MainLoop MainLoop;
-+
+-    return s->ret;
- /**
++    return ret;
-  * qemu_init_main_loop: Set up the process so that it can run the main loop.
+ }
-  *
-diff --git a/include/sysemu/event-loop-base.h b/include/sysemu/event-loop-base.h
+ static const JobDriver blockdev_create_job_driver = {
-index XXXXXXX..XXXXXXX 100644
+diff --git a/block/stream.c b/block/stream.c
---- a/include/sysemu/event-loop-base.h
+index XXXXXXX..XXXXXXX 100644
-+++ b/include/sysemu/event-loop-base.h
+--- a/block/stream.c
-@@ -XXX,XX +XXX,XX @@ struct EventLoopBaseClass {
++++ b/block/stream.c
+@@ -XXX,XX +XXX,XX @@ static int coroutine_fn stream_populate(BlockBackend *blk,
-     void (*init)(EventLoopBase *base, Error **errp);
+     return blk_co_preadv(blk, offset, qiov.size, &qiov, BDRV_REQ_COPY_ON_READ);
-     void (*update_params)(EventLoopBase *base, Error **errp);
+ }
-+    bool (*can_be_deleted)(EventLoopBase *base);
 -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;
@@ -XXX,XX +XXX,XX @@ 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;
          }
      }
@@ -XXX,XX +XXX,XX @@ out:
      }
      g_free(s->backing_file_str);
 -    job_completed(job, data->ret);
 -    g_free(data);
 +    job->ret = ret;
  }
  static int coroutine_fn stream_run(Job *job, Error **errp)
  {
      StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
 -    StreamCompleteData *data;
      BlockBackend *blk = s->common.blk;
      BlockDriverState *bs = blk_bs(blk);
      BlockDriverState *base = s->base;
@@ -XXX,XX +XXX,XX @@ static int coroutine_fn stream_run(Job *job, Error **errp)
  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);
      return ret;
  }
@@ -XXX,XX +XXX,XX @@ static const BlockJobDriver stream_job_driver = {
          .job_type      = JOB_TYPE_STREAM,
          .free          = block_job_free,
          .run           = stream_run,
 +        .exit          = stream_exit,
          .user_resume   = block_job_user_resume,
          .drain         = block_job_drain,
      },
 diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
 index XXXXXXX..XXXXXXX 100644
 --- a/tests/test-bdrv-drain.c
 +++ b/tests/test-bdrv-drain.c
@@ -XXX,XX +XXX,XX @@ typedef struct TestBlockJob {
      bool should_complete;
  } TestBlockJob;
 -static void test_job_completed(Job *job, void *opaque)
 -{
 -    job_completed(job, 0);
 -}
 -
  static int coroutine_fn test_job_run(Job *job, Error **errp)
  {
      TestBlockJob *s = container_of(job, TestBlockJob, common.job);
@@ -XXX,XX +XXX,XX @@ static int coroutine_fn test_job_run(Job *job, Error **errp)
          job_pause_point(&s->common.job);
      }
 -    job_defer_to_main_loop(&s->common.job, test_job_completed, NULL);
      return 0;
  }
 diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
 index XXXXXXX..XXXXXXX 100644
 --- a/tests/test-blockjob-txn.c
 +++ b/tests/test-blockjob-txn.c
@@ -XXX,XX +XXX,XX @@ 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);
  }
@@ -XXX,XX +XXX,XX @@ static int coroutine_fn test_block_job_run(Job *job, Error **errp)
          }
      }
 -    job_defer_to_main_loop(job, test_block_job_complete,
 -                           (void *)(intptr_t)s->rc);
      return s->rc;
  }
@@ -XXX,XX +XXX,XX @@ static const BlockJobDriver test_block_job_driver = {
          .user_resume   = block_job_user_resume,
          .drain         = block_job_drain,
          .run           = test_block_job_run,
 +        .exit          = test_block_job_exit,
      },
  };
- struct EventLoopBase {
+diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
-diff --git a/event-loop-base.c b/event-loop-base.c
+index XXXXXXX..XXXXXXX 100644
-index XXXXXXX..XXXXXXX 100644
+--- a/tests/test-blockjob.c
---- a/event-loop-base.c
++++ b/tests/test-blockjob.c
-+++ b/event-loop-base.c
+@@ -XXX,XX +XXX,XX @@ typedef struct CancelJob {
-@@ -XXX,XX +XXX,XX @@ static void event_loop_base_complete(UserCreatable *uc, Error **errp)
+     bool completed;
-     }
+ } CancelJob;
- }
+-static void cancel_job_completed(Job *job, void *opaque)
-+static bool event_loop_base_can_be_deleted(UserCreatable *uc)
++static void cancel_job_exit(Job *job)
-+{
+ {
-+    EventLoopBaseClass *bc = EVENT_LOOP_BASE_GET_CLASS(uc);
+-    CancelJob *s = opaque;
-+    EventLoopBase *backend = EVENT_LOOP_BASE(uc);
++    CancelJob *s = container_of(job, CancelJob, common.job);
-+
+     s->completed = true;
-+    if (bc->can_be_deleted) {
+-    job_completed(job, 0);
-+        return bc->can_be_deleted(backend);
+ }
-+    }
-+
+ static void cancel_job_complete(Job *job, Error **errp)
-+    return true;
+@@ -XXX,XX +XXX,XX @@ static int coroutine_fn cancel_job_run(Job *job, Error **errp)
-+}
-+
+     while (!s->should_complete) {
- static void event_loop_base_class_init(ObjectClass *klass, void *class_data)
+         if (job_is_cancelled(&s->common.job)) {
- {
+-            goto defer;
-     UserCreatableClass *ucc = USER_CREATABLE_CLASS(klass);
++            return 0;
-     ucc->complete = event_loop_base_complete;
+         }
-+    ucc->can_be_deleted = event_loop_base_can_be_deleted;
+         if (!job_is_ready(&s->common.job) && s->should_converge) {
-     object_class_property_add(klass, "aio-max-batch", "int",
+@@ -XXX,XX +XXX,XX @@ static int coroutine_fn cancel_job_run(Job *job, Error **errp)
-                               event_loop_base_get_param,
+         job_sleep_ns(&s->common.job, 100000);
-diff --git a/util/main-loop.c b/util/main-loop.c
+     }
-index XXXXXXX..XXXXXXX 100644
---- a/util/main-loop.c
+- defer:
-+++ b/util/main-loop.c
+-    job_defer_to_main_loop(&s->common.job, cancel_job_completed, s);
@@ -XXX,XX +XXX,XX @@
  #include "qemu/error-report.h"
  #include "qemu/queue.h"
  #include "qemu/compiler.h"
 +#include "qom/object.h"
  #ifndef _WIN32
  #include <sys/wait.h>
@@ -XXX,XX +XXX,XX @@ int qemu_init_main_loop(Error **errp)
      return 0;
  }
-+static void main_loop_update_params(EventLoopBase *base, Error **errp)
+@@ -XXX,XX +XXX,XX @@ static const BlockJobDriver test_cancel_driver = {
-+{
+         .user_resume   = block_job_user_resume,
-+    if (!qemu_aio_context) {
+         .drain         = block_job_drain,
-+        error_setg(errp, "qemu aio context not ready");
+         .run           = cancel_job_run,
-+        return;
++        .exit          = cancel_job_exit,
-+    }
+         .complete      = cancel_job_complete,
-+
+     },
-+    aio_context_set_aio_params(qemu_aio_context, base->aio_max_batch, errp);
+ };
 +}
 +
 +MainLoop *mloop;
 +
 +static void main_loop_init(EventLoopBase *base, Error **errp)
 +{
 +    MainLoop *m = MAIN_LOOP(base);
 +
 +    if (mloop) {
 +        error_setg(errp, "only one main-loop instance allowed");
 +        return;
 +    }
 +
 +    main_loop_update_params(base, errp);
 +
 +    mloop = m;
 +    return;
 +}
 +
 +static bool main_loop_can_be_deleted(EventLoopBase *base)
 +{
 +    return false;
 +}
 +
 +static void main_loop_class_init(ObjectClass *oc, void *class_data)
 +{
 +    EventLoopBaseClass *bc = EVENT_LOOP_BASE_CLASS(oc);
 +
 +    bc->init = main_loop_init;
 +    bc->update_params = main_loop_update_params;
 +    bc->can_be_deleted = main_loop_can_be_deleted;
 +}
 +
 +static const TypeInfo main_loop_info = {
 +    .name = TYPE_MAIN_LOOP,
 +    .parent = TYPE_EVENT_LOOP_BASE,
 +    .class_init = main_loop_class_init,
 +    .instance_size = sizeof(MainLoop),
 +};
 +
 +static void main_loop_register_types(void)
 +{
 +    type_register_static(&main_loop_info);
 +}
 +
 +type_init(main_loop_register_types)
 +
  static int max_priority;
  #ifndef _WIN32
 --
-.35.1
+.17.1

-New patch
+[Qemu-devel] [PULL v2 08/10] block/backup: make function variables consistently named
+From: John Snow <jsnow@redhat.com>
+Rename opaque_job to job to be consistent with other job implementations.
+Rename 'job', the BackupBlockJob object, to 's' to also be consistent.
+Suggested-by: Eric Blake <eblake@redhat.com>
+Signed-off-by: John Snow <jsnow@redhat.com>
+Reviewed-by: Max Reitz <mreitz@redhat.com>
+Message-id: 20180830015734.19765-8-jsnow@redhat.com
+Signed-off-by: Max Reitz <mreitz@redhat.com>
+---
+ block/backup.c | 62 +++++++++++++++++++++++++-------------------------
+file changed, 31 insertions(+), 31 deletions(-)
+diff --git a/block/backup.c b/block/backup.c
+index XXXXXXX..XXXXXXX 100644
+--- a/block/backup.c
++++ b/block/backup.c
+@@ -XXX,XX +XXX,XX @@ static void backup_incremental_init_copy_bitmap(BackupBlockJob *job)
+     bdrv_dirty_iter_free(dbi);
+ }
+-static int coroutine_fn backup_run(Job *opaque_job, Error **errp)
++static int coroutine_fn backup_run(Job *job, Error **errp)
+ {
+-    BackupBlockJob *job = container_of(opaque_job, BackupBlockJob, common.job);
+-    BlockDriverState *bs = blk_bs(job->common.blk);
++    BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
++    BlockDriverState *bs = blk_bs(s->common.blk);
+     int64_t offset, nb_clusters;
+     int ret = 0;
+-    QLIST_INIT(&job->inflight_reqs);
+-    qemu_co_rwlock_init(&job->flush_rwlock);
++    QLIST_INIT(&s->inflight_reqs);
++    qemu_co_rwlock_init(&s->flush_rwlock);
+-    nb_clusters = DIV_ROUND_UP(job->len, job->cluster_size);
+-    job_progress_set_remaining(&job->common.job, job->len);
++    nb_clusters = DIV_ROUND_UP(s->len, s->cluster_size);
++    job_progress_set_remaining(job, s->len);
+-    job->copy_bitmap = hbitmap_alloc(nb_clusters, 0);
+-    if (job->sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) {
+-        backup_incremental_init_copy_bitmap(job);
++    s->copy_bitmap = hbitmap_alloc(nb_clusters, 0);
++    if (s->sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) {
++        backup_incremental_init_copy_bitmap(s);
+     } else {
+-        hbitmap_set(job->copy_bitmap, 0, nb_clusters);
++        hbitmap_set(s->copy_bitmap, 0, nb_clusters);
+     }
+-    job->before_write.notify = backup_before_write_notify;
+-    bdrv_add_before_write_notifier(bs, &job->before_write);
++    s->before_write.notify = backup_before_write_notify;
++    bdrv_add_before_write_notifier(bs, &s->before_write);
+-    if (job->sync_mode == MIRROR_SYNC_MODE_NONE) {
++    if (s->sync_mode == MIRROR_SYNC_MODE_NONE) {
+         /* All bits are set in copy_bitmap to allow any cluster to be copied.
+          * This does not actually require them to be copied. */
+-        while (!job_is_cancelled(&job->common.job)) {
++        while (!job_is_cancelled(job)) {
+             /* Yield until the job is cancelled.  We just let our before_write
+              * notify callback service CoW requests. */
+-            job_yield(&job->common.job);
++            job_yield(job);
+         }
+-    } else if (job->sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) {
+-        ret = backup_run_incremental(job);
++    } else if (s->sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) {
++        ret = backup_run_incremental(s);
+     } else {
+         /* Both FULL and TOP SYNC_MODE's require copying.. */
+-        for (offset = 0; offset < job->len;
+-             offset += job->cluster_size) {
++        for (offset = 0; offset < s->len;
++             offset += s->cluster_size) {
+             bool error_is_read;
+             int alloced = 0;
+-            if (yield_and_check(job)) {
++            if (yield_and_check(s)) {
+                 break;
+             }
+-            if (job->sync_mode == MIRROR_SYNC_MODE_TOP) {
++            if (s->sync_mode == MIRROR_SYNC_MODE_TOP) {
+                 int i;
+                 int64_t n;
+                 /* Check to see if these blocks are already in the
+                  * backing file. */
+-                for (i = 0; i < job->cluster_size;) {
++                for (i = 0; i < s->cluster_size;) {
+                     /* bdrv_is_allocated() only returns true/false based
+                      * on the first set of sectors it comes across that
+                      * are are all in the same state.
+@@ -XXX,XX +XXX,XX @@ static int coroutine_fn backup_run(Job *opaque_job, Error **errp)
+                      * needed but at some point that is always the case. */
+                     alloced =
+                         bdrv_is_allocated(bs, offset + i,
+-                                          job->cluster_size - i, &n);
++                                          s->cluster_size - i, &n);
+                     i += n;
+                     if (alloced || n == 0) {
+@@ -XXX,XX +XXX,XX @@ static int coroutine_fn backup_run(Job *opaque_job, Error **errp)
+             if (alloced < 0) {
+                 ret = alloced;
+             } else {
+-                ret = backup_do_cow(job, offset, job->cluster_size,
++                ret = backup_do_cow(s, offset, s->cluster_size,
+                                     &error_is_read, false);
+             }
+             if (ret < 0) {
+                 /* Depending on error action, fail now or retry cluster */
+                 BlockErrorAction action =
+-                    backup_error_action(job, error_is_read, -ret);
++                    backup_error_action(s, error_is_read, -ret);
+                 if (action == BLOCK_ERROR_ACTION_REPORT) {
+                     break;
+                 } else {
+-                    offset -= job->cluster_size;
++                    offset -= s->cluster_size;
+                     continue;
+                 }
+             }
+         }
+     }
+-    notifier_with_return_remove(&job->before_write);
++    notifier_with_return_remove(&s->before_write);
+     /* wait until pending backup_do_cow() calls have completed */
+-    qemu_co_rwlock_wrlock(&job->flush_rwlock);
+-    qemu_co_rwlock_unlock(&job->flush_rwlock);
+-    hbitmap_free(job->copy_bitmap);
++    qemu_co_rwlock_wrlock(&s->flush_rwlock);
++    qemu_co_rwlock_unlock(&s->flush_rwlock);
++    hbitmap_free(s->copy_bitmap);
+     return ret;
+ }
+--
+.17.1

-[PULL 1/3] Introduce event-loop-base abstract class
+[Qemu-devel] [PULL v2 09/10] jobs: remove ret argument to job_completed; privatize it
-From: Nicolas Saenz Julienne <nsaenzju@redhat.com>
+From: John Snow <jsnow@redhat.com>
-Introduce the 'event-loop-base' abstract class, it'll hold the
+Jobs are now expected to return their retcode on the stack, from the
-properties common to all event loops and provide the necessary hooks for
+.run callback, so we can remove that argument.
 their creation and maintenance. Then have iothread inherit from it.
-EventLoopBaseClass is defined as user creatable and provides a hook for
+job_cancel does not need to set -ECANCELED because job_completed will
-its children to attach themselves to the user creatable class 'complete'
+update the return code itself if the job was canceled.
 function. It also provides an update_params() callback to propagate
 property changes onto its children.
-The new 'event-loop-base' class will live in the root directory. It is
+While we're here, make job_completed static to job.c and remove it from
-built on its own using the 'link_whole' option (there are no direct
+job.h; move the documentation of return code to the .run() callback and
-function dependencies between the class and its children, it all happens
+to the job->ret property, accordingly.
 trough 'constructor' magic). And also imposes new compilation
 dependencies:
-    qom <- event-loop-base <- blockdev (iothread.c)
+Signed-off-by: John Snow <jsnow@redhat.com>
 Message-id: 20180830015734.19765-9-jsnow@redhat.com
 Reviewed-by: Max Reitz <mreitz@redhat.com>
 Signed-off-by: Max Reitz <mreitz@redhat.com>
 ---
  include/qemu/job.h | 28 +++++++++++++++-------------
  job.c              | 11 ++++++-----
  trace-events       |  2 +-
 files changed, 22 insertions(+), 19 deletions(-)
-And in subsequent patches:
+diff --git a/include/qemu/job.h b/include/qemu/job.h
     qom <- event-loop-base <- qemuutil (util/main-loop.c)
 All this forced some amount of reordering in meson.build:
  - Moved qom build definition before qemuutil. Doing it the other way
    around (i.e. moving qemuutil after qom) isn't possible as a lot of
    core libraries that live in between the two depend on it.
  - Process the 'hw' subdir earlier, as it introduces files into the
    'qom' source set.
 No functional changes intended.
 Signed-off-by: Nicolas Saenz Julienne <nsaenzju@redhat.com>
 Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
 Acked-by: Markus Armbruster <armbru@redhat.com>
 Message-id: 20220425075723.20019-2-nsaenzju@redhat.com
 Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
 ---
  qapi/qom.json                    |  22 +++++--
  meson.build                      |  23 ++++---
  include/sysemu/event-loop-base.h |  36 +++++++++++
  include/sysemu/iothread.h        |   6 +-
  event-loop-base.c                | 104 +++++++++++++++++++++++++++++++
  iothread.c                       |  65 ++++++-------------
 files changed, 192 insertions(+), 64 deletions(-)
  create mode 100644 include/sysemu/event-loop-base.h
  create mode 100644 event-loop-base.c
 diff --git a/qapi/qom.json b/qapi/qom.json
 index XXXXXXX..XXXXXXX 100644
---- a/qapi/qom.json
+--- a/include/qemu/job.h
-+++ b/qapi/qom.json
++++ b/include/qemu/job.h
-@@ -XXX,XX +XXX,XX @@
+@@ -XXX,XX +XXX,XX @@ typedef struct Job {
-             '*repeat': 'bool',
+     /** Estimated progress_current value at the completion of the job */
-             '*grab-toggle': 'GrabToggleKeys' } }
+     int64_t progress_total;
-+##
+-    /** ret code passed to job_completed. */
-+# @EventLoopBaseProperties:
++    /**
-+#
++     * Return code from @run and/or @prepare callback(s).
-+# Common properties for event loops
++     * Not final until the job has reached the CONCLUDED status.
-+#
++     * 0 on success, -errno on failure.
-+# @aio-max-batch: maximum number of requests in a batch for the AIO engine,
++     */
-+#                 0 means that the engine will use its default.
+     int ret;
-+#                 (default: 0)
-+#
+     /**
-+# Since: 7.1
+@@ -XXX,XX +XXX,XX @@ struct JobDriver {
-+##
+     /** Enum describing the operation */
-+{ 'struct': 'EventLoopBaseProperties',
+     JobType job_type;
-+  'data': { '*aio-max-batch': 'int' } }
-+
+-    /** Mandatory: Entrypoint for the Coroutine. */
- ##
++    /**
- # @IothreadProperties:
++     * Mandatory: Entrypoint for the Coroutine.
- #
++     *
-@@ -XXX,XX +XXX,XX @@
++     * This callback will be invoked when moving from CREATED to RUNNING.
- #               algorithm detects it is spending too long polling without
++     *
- #               encountering events. 0 selects a default behaviour (default: 0)
++     * If this callback returns nonzero, the job transaction it is part of is
- #
++     * aborted. If it returns zero, the job moves into the WAITING state. If it
--# @aio-max-batch: maximum number of requests in a batch for the AIO engine,
++     * is the last job to complete in its transaction, all jobs in the
--#                 0 means that the engine will use its default
++     * transaction move from WAITING to PENDING.
--#                 (default:0, since 6.1)
++     */
-+# The @aio-max-batch option is available since 6.1.
+     int coroutine_fn (*run)(Job *job, Error **errp);
- #
- # Since: 2.0
+     /**
- ##
+@@ -XXX,XX +XXX,XX @@ void job_early_fail(Job *job);
- { 'struct': 'IothreadProperties',
+ /** Moves the @job from RUNNING to READY */
-+  'base': 'EventLoopBaseProperties',
+ void job_transition_to_ready(Job *job);
-   'data': { '*poll-max-ns': 'int',
-             '*poll-grow': 'int',
+-/**
--            '*poll-shrink': 'int',
+- * @job: The job being completed.
--            '*aio-max-batch': 'int' } }
+- * @ret: The status code.
-+            '*poll-shrink': 'int' } }
+- *
+- * 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
- # @MemoryBackendProperties:
+- * is the last job to complete in its transaction, all jobs in the transaction
-diff --git a/meson.build b/meson.build
+- * move from WAITING to PENDING.
 - */
 -void job_completed(Job *job, int ret);
 -
  /** Asynchronously complete the specified @job. */
  void job_complete(Job *job, Error **errp);
 diff --git a/job.c b/job.c
 index XXXXXXX..XXXXXXX 100644
---- a/meson.build
+--- a/job.c
-+++ b/meson.build
++++ b/job.c
-@@ -XXX,XX +XXX,XX @@ subdir('qom')
+@@ -XXX,XX +XXX,XX @@ void job_drain(Job *job)
  subdir('authz')
  subdir('crypto')
  subdir('ui')
 +subdir('hw')
  if enable_modules
@@ -XXX,XX +XXX,XX @@ if enable_modules
    modulecommon = declare_dependency(link_whole: libmodulecommon, compile_args: '-DBUILD_DSO')
  endif
 +qom_ss = qom_ss.apply(config_host, strict: false)
 +libqom = static_library('qom', qom_ss.sources() + genh,
 +                        dependencies: [qom_ss.dependencies()],
 +                        name_suffix: 'fa')
 +qom = declare_dependency(link_whole: libqom)
 +
 +event_loop_base = files('event-loop-base.c')
 +event_loop_base = static_library('event-loop-base', sources: event_loop_base + genh,
 +                                 build_by_default: true)
 +event_loop_base = declare_dependency(link_whole: event_loop_base,
 +                                     dependencies: [qom])
 +
  stub_ss = stub_ss.apply(config_all, strict: false)
  util_ss.add_all(trace_ss)
@@ -XXX,XX +XXX,XX @@ subdir('monitor')
  subdir('net')
  subdir('replay')
  subdir('semihosting')
 -subdir('hw')
  subdir('tcg')
  subdir('fpu')
  subdir('accel')
@@ -XXX,XX +XXX,XX @@ qemu_syms = custom_target('qemu.syms', output: 'qemu.syms',
                               capture: true,
                               command: [undefsym, nm, '@INPUT@'])
 -qom_ss = qom_ss.apply(config_host, strict: false)
 -libqom = static_library('qom', qom_ss.sources() + genh,
 -                        dependencies: [qom_ss.dependencies()],
 -                        name_suffix: 'fa')
 -
 -qom = declare_dependency(link_whole: libqom)
 -
  authz_ss = authz_ss.apply(config_host, strict: false)
  libauthz = static_library('authz', authz_ss.sources() + genh,
                            dependencies: [authz_ss.dependencies()],
@@ -XXX,XX +XXX,XX @@ libblockdev = static_library('blockdev', blockdev_ss.sources() + genh,
                               build_by_default: false)
  blockdev = declare_dependency(link_whole: [libblockdev],
 -                              dependencies: [block])
 +                              dependencies: [block, event_loop_base])
  qmp_ss = qmp_ss.apply(config_host, strict: false)
  libqmp = static_library('qmp', qmp_ss.sources() + genh,
 diff --git a/include/sysemu/event-loop-base.h b/include/sysemu/event-loop-base.h
 new file mode 100644
 index XXXXXXX..XXXXXXX
 --- /dev/null
 +++ b/include/sysemu/event-loop-base.h
@@ -XXX,XX +XXX,XX @@
 +/*
 + * QEMU event-loop backend
 + *
 + * Copyright (C) 2022 Red Hat Inc
 + *
 + * Authors:
 + *  Nicolas Saenz Julienne <nsaenzju@redhat.com>
 + *
 + * This work is licensed under the terms of the GNU GPL, version 2 or later.
 + * See the COPYING file in the top-level directory.
 + */
 +#ifndef QEMU_EVENT_LOOP_BASE_H
 +#define QEMU_EVENT_LOOP_BASE_H
 +
 +#include "qom/object.h"
 +#include "block/aio.h"
 +#include "qemu/typedefs.h"
 +
 +#define TYPE_EVENT_LOOP_BASE         "event-loop-base"
 +OBJECT_DECLARE_TYPE(EventLoopBase, EventLoopBaseClass,
 +                    EVENT_LOOP_BASE)
 +
 +struct EventLoopBaseClass {
 +    ObjectClass parent_class;
 +
 +    void (*init)(EventLoopBase *base, Error **errp);
 +    void (*update_params)(EventLoopBase *base, Error **errp);
 +};
 +
 +struct EventLoopBase {
 +    Object parent;
 +
 +    /* AioContext AIO engine parameters */
 +    int64_t aio_max_batch;
 +};
 +#endif
 diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h
 index XXXXXXX..XXXXXXX 100644
 --- a/include/sysemu/iothread.h
 +++ b/include/sysemu/iothread.h
@@ -XXX,XX +XXX,XX @@
  #include "block/aio.h"
  #include "qemu/thread.h"
  #include "qom/object.h"
 +#include "sysemu/event-loop-base.h"
  #define TYPE_IOTHREAD "iothread"
  struct IOThread {
 -    Object parent_obj;
 +    EventLoopBase parent_obj;
      QemuThread thread;
      AioContext *ctx;
@@ -XXX,XX +XXX,XX @@ struct IOThread {
      int64_t poll_max_ns;
      int64_t poll_grow;
      int64_t poll_shrink;
 -
 -    /* AioContext AIO engine parameters */
 -    int64_t aio_max_batch;
  };
  typedef struct IOThread IOThread;
 diff --git a/event-loop-base.c b/event-loop-base.c
 new file mode 100644
 index XXXXXXX..XXXXXXX
 --- /dev/null
 +++ b/event-loop-base.c
@@ -XXX,XX +XXX,XX @@
 +/*
 + * QEMU event-loop base
 + *
 + * Copyright (C) 2022 Red Hat Inc
 + *
 + * Authors:
 + *  Stefan Hajnoczi <stefanha@redhat.com>
 + *  Nicolas Saenz Julienne <nsaenzju@redhat.com>
 + *
 + * This work is licensed under the terms of the GNU GPL, version 2 or later.
 + * See the COPYING file in the top-level directory.
 + */
 +
 +#include "qemu/osdep.h"
 +#include "qom/object_interfaces.h"
 +#include "qapi/error.h"
 +#include "sysemu/event-loop-base.h"
 +
 +typedef struct {
 +    const char *name;
 +    ptrdiff_t offset; /* field's byte offset in EventLoopBase struct */
 +} EventLoopBaseParamInfo;
 +
 +static EventLoopBaseParamInfo aio_max_batch_info = {
 +    "aio-max-batch", offsetof(EventLoopBase, aio_max_batch),
 +};
 +
 +static void event_loop_base_get_param(Object *obj, Visitor *v,
 +        const char *name, void *opaque, Error **errp)
 +{
 +    EventLoopBase *event_loop_base = EVENT_LOOP_BASE(obj);
 +    EventLoopBaseParamInfo *info = opaque;
 +    int64_t *field = (void *)event_loop_base + info->offset;
 +
 +    visit_type_int64(v, name, field, errp);
 +}
 +
 +static void event_loop_base_set_param(Object *obj, Visitor *v,
 +        const char *name, void *opaque, Error **errp)
 +{
 +    EventLoopBaseClass *bc = EVENT_LOOP_BASE_GET_CLASS(obj);
 +    EventLoopBase *base = EVENT_LOOP_BASE(obj);
 +    EventLoopBaseParamInfo *info = opaque;
 +    int64_t *field = (void *)base + info->offset;
 +    int64_t value;
 +
 +    if (!visit_type_int64(v, name, &value, errp)) {
 +        return;
 +    }
 +
 +    if (value < 0) {
 +        error_setg(errp, "%s value must be in range [0, %" PRId64 "]",
 +                   info->name, INT64_MAX);
 +        return;
 +    }
 +
 +    *field = value;
 +
 +    if (bc->update_params) {
 +        bc->update_params(base, errp);
 +    }
 +
 +    return;
 +}
 +
 +static void event_loop_base_complete(UserCreatable *uc, Error **errp)
 +{
 +    EventLoopBaseClass *bc = EVENT_LOOP_BASE_GET_CLASS(uc);
 +    EventLoopBase *base = EVENT_LOOP_BASE(uc);
 +
 +    if (bc->init) {
 +        bc->init(base, errp);
 +    }
 +}
 +
 +static void event_loop_base_class_init(ObjectClass *klass, void *class_data)
 +{
 +    UserCreatableClass *ucc = USER_CREATABLE_CLASS(klass);
 +    ucc->complete = event_loop_base_complete;
 +
 +    object_class_property_add(klass, "aio-max-batch", "int",
 +                              event_loop_base_get_param,
 +                              event_loop_base_set_param,
 +                              NULL, &aio_max_batch_info);
 +}
 +
 +static const TypeInfo event_loop_base_info = {
 +    .name = TYPE_EVENT_LOOP_BASE,
 +    .parent = TYPE_OBJECT,
 +    .instance_size = sizeof(EventLoopBase),
 +    .class_size = sizeof(EventLoopBaseClass),
 +    .class_init = event_loop_base_class_init,
 +    .abstract = true,
 +    .interfaces = (InterfaceInfo[]) {
 +        { TYPE_USER_CREATABLE },
 +        { }
 +    }
 +};
 +
 +static void register_types(void)
 +{
 +    type_register_static(&event_loop_base_info);
 +}
 +type_init(register_types);
 diff --git a/iothread.c b/iothread.c
 index XXXXXXX..XXXXXXX 100644
 --- a/iothread.c
 +++ b/iothread.c
@@ -XXX,XX +XXX,XX @@
  #include "qemu/module.h"
  #include "block/aio.h"
  #include "block/block.h"
 +#include "sysemu/event-loop-base.h"
  #include "sysemu/iothread.h"
  #include "qapi/error.h"
  #include "qapi/qapi-commands-misc.h"
@@ -XXX,XX +XXX,XX @@ static void iothread_init_gcontext(IOThread *iothread)
      iothread->main_loop = g_main_loop_new(iothread->worker_context, TRUE);
  }
 -static void iothread_set_aio_context_params(IOThread *iothread, Error **errp)
 +static void iothread_set_aio_context_params(EventLoopBase *base, Error **errp)
  {
 +    IOThread *iothread = IOTHREAD(base);
      ERRP_GUARD();
 +    if (!iothread->ctx) {
 +        return;
 +    }
 +
      aio_context_set_poll_params(iothread->ctx,
                                  iothread->poll_max_ns,
                                  iothread->poll_grow,
@@ -XXX,XX +XXX,XX @@ static void iothread_set_aio_context_params(IOThread *iothread, Error **errp)
      }
      aio_context_set_aio_params(iothread->ctx,
 -                               iothread->aio_max_batch,
 +                               iothread->parent_obj.aio_max_batch,
                                 errp);
  }
 -static void iothread_complete(UserCreatable *obj, Error **errp)
 +
 +static void iothread_init(EventLoopBase *base, Error **errp)
  {
      Error *local_error = NULL;
 -    IOThread *iothread = IOTHREAD(obj);
 +    IOThread *iothread = IOTHREAD(base);
      char *thread_name;
      iothread->stopping = false;
@@ -XXX,XX +XXX,XX @@ static void iothread_complete(UserCreatable *obj, Error **errp)
       */
      iothread_init_gcontext(iothread);
 -    iothread_set_aio_context_params(iothread, &local_error);
 +    iothread_set_aio_context_params(base, &local_error);
      if (local_error) {
          error_propagate(errp, local_error);
          aio_context_unref(iothread->ctx);
@@ -XXX,XX +XXX,XX @@ static void iothread_complete(UserCreatable *obj, Error **errp)
       * to inherit.
       */
      thread_name = g_strdup_printf("IO %s",
 -                        object_get_canonical_path_component(OBJECT(obj)));
 +                        object_get_canonical_path_component(OBJECT(base)));
      qemu_thread_create(&iothread->thread, thread_name, iothread_run,
                         iothread, QEMU_THREAD_JOINABLE);
      g_free(thread_name);
@@ -XXX,XX +XXX,XX @@ static IOThreadParamInfo poll_grow_info = {
  static IOThreadParamInfo poll_shrink_info = {
      "poll-shrink", offsetof(IOThread, poll_shrink),
  };
 -static IOThreadParamInfo aio_max_batch_info = {
 -    "aio-max-batch", offsetof(IOThread, aio_max_batch),
 -};
  static void iothread_get_param(Object *obj, Visitor *v,
          const char *name, IOThreadParamInfo *info, Error **errp)
@@ -XXX,XX +XXX,XX @@ static void iothread_set_poll_param(Object *obj, Visitor *v,
      }
  }
--static void iothread_get_aio_param(Object *obj, Visitor *v,
++static void job_completed(Job *job);
--        const char *name, void *opaque, Error **errp)
++
--{
+ static void job_exit(void *opaque)
 -    IOThreadParamInfo *info = opaque;
 -
 -    iothread_get_param(obj, v, name, info, errp);
 -}
 -
 -static void iothread_set_aio_param(Object *obj, Visitor *v,
 -        const char *name, void *opaque, Error **errp)
 -{
 -    IOThread *iothread = IOTHREAD(obj);
 -    IOThreadParamInfo *info = opaque;
 -
 -    if (!iothread_set_param(obj, v, name, info, errp)) {
 -        return;
 -    }
 -
 -    if (iothread->ctx) {
 -        aio_context_set_aio_params(iothread->ctx,
 -                                   iothread->aio_max_batch,
 -                                   errp);
 -    }
 -}
 -
  static void iothread_class_init(ObjectClass *klass, void *class_data)
  {
--    UserCreatableClass *ucc = USER_CREATABLE_CLASS(klass);
+     Job *job = (Job *)opaque;
--    ucc->complete = iothread_complete;
+@@ -XXX,XX +XXX,XX @@ static void job_exit(void *opaque)
-+    EventLoopBaseClass *bc = EVENT_LOOP_BASE_CLASS(klass);
+         job->driver->exit(job);
-+
+         aio_context_release(aio_context);
-+    bc->init = iothread_init;
+     }
-+    bc->update_params = iothread_set_aio_context_params;
+-    job_completed(job, job->ret);
++    job_completed(job);
      object_class_property_add(klass, "poll-max-ns", "int",
                                iothread_get_poll_param,
@@ -XXX,XX +XXX,XX @@ static void iothread_class_init(ObjectClass *klass, void *class_data)
                                iothread_get_poll_param,
                                iothread_set_poll_param,
                                NULL, &poll_shrink_info);
 -    object_class_property_add(klass, "aio-max-batch", "int",
 -                              iothread_get_aio_param,
 -                              iothread_set_aio_param,
 -                              NULL, &aio_max_batch_info);
  }
- static const TypeInfo iothread_info = {
+ /**
-     .name = TYPE_IOTHREAD,
+@@ -XXX,XX +XXX,XX @@ static void job_completed_txn_success(Job *job)
--    .parent = TYPE_OBJECT,
+     }
-+    .parent = TYPE_EVENT_LOOP_BASE,
+ }
-     .class_init = iothread_class_init,
-     .instance_size = sizeof(IOThread),
+-void job_completed(Job *job, int ret)
-     .instance_init = iothread_instance_init,
++static void job_completed(Job *job)
-     .instance_finalize = iothread_instance_finalize,
+ {
--    .interfaces = (InterfaceInfo[]) {
+     assert(job && job->txn && !job_is_completed(job));
--        {TYPE_USER_CREATABLE},
--        {}
+-    job->ret = ret;
--    },
+     job_update_rc(job);
- };
+-    trace_job_completed(job, ret, job->ret);
++    trace_job_completed(job, job->ret);
- static void iothread_register_types(void)
+     if (job->ret) {
-@@ -XXX,XX +XXX,XX @@ static int query_one_iothread(Object *object, void *opaque)
+         job_completed_txn_abort(job);
-     info->poll_max_ns = iothread->poll_max_ns;
+     } else {
-     info->poll_grow = iothread->poll_grow;
+@@ -XXX,XX +XXX,XX @@ void job_cancel(Job *job, bool force)
-     info->poll_shrink = iothread->poll_shrink;
+     }
--    info->aio_max_batch = iothread->aio_max_batch;
+     job_cancel_async(job, force);
-+    info->aio_max_batch = iothread->parent_obj.aio_max_batch;
+     if (!job_started(job)) {
+-        job_completed(job, -ECANCELED);
-     QAPI_LIST_APPEND(*tail, info);
++        job_completed(job);
-     return 0;
+     } else if (job->deferred_to_main_loop) {
          job_completed_txn_abort(job);
      } else {
 diff --git a/trace-events b/trace-events
 index XXXXXXX..XXXXXXX 100644
 --- a/trace-events
 +++ b/trace-events
@@ -XXX,XX +XXX,XX @@ gdbstub_err_checksum_incorrect(uint8_t expected, uint8_t got) "got command packe
  # job.c
  job_state_transition(void *job,  int ret, const char *legal, const char *s0, const char *s1) "job %p (ret: %d) attempting %s transition (%s-->%s)"
  job_apply_verb(void *job, const char *state, const char *verb, const char *legal) "job %p in state %s; applying verb %s (%s)"
 -job_completed(void *job, int ret, int jret) "job %p ret %d corrected ret %d"
 +job_completed(void *job, int ret) "job %p ret %d"
  # job-qmp.c
  qmp_job_cancel(void *job) "job %p"
 --
-.35.1
+.17.1

-New patch
+[Qemu-devel] [PULL v2 10/10] jobs: remove job_defer_to_main_loop
+From: John Snow <jsnow@redhat.com>
+Now that the job infrastructure is handling the job_completed call for
+all implemented jobs, we can remove the interface that allowed jobs to
+schedule their own completion.
+Signed-off-by: John Snow <jsnow@redhat.com>
+Reviewed-by: Max Reitz <mreitz@redhat.com>
+Message-id: 20180830015734.19765-10-jsnow@redhat.com
+Signed-off-by: Max Reitz <mreitz@redhat.com>
+---
+ include/qemu/job.h | 17 -----------------
+ job.c              | 40 ++--------------------------------------
+files changed, 2 insertions(+), 55 deletions(-)
+diff --git a/include/qemu/job.h b/include/qemu/job.h
+index XXXXXXX..XXXXXXX 100644
+--- a/include/qemu/job.h
++++ b/include/qemu/job.h
+@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644
+--- a/job.c
++++ b/job.c
+@@ -XXX,XX +XXX,XX @@ static void coroutine_fn job_co_entry(void *opaque)
+     assert(job && job->driver && job->driver->run);
+     job_pause_point(job);
+     job->ret = job->driver->run(job, &job->err);
+-    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);
+ }
+@@ -XXX,XX +XXX,XX @@ 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;
+--
+.17.1

The following changes since commit 9cf289af47bcfae5c75de37d8e5d6fd23705322c:

Merge tag 'qga-pull-request' of gitlab.com:marcandre.lureau/qemu into staging (2022-05-04 03:42:49 -0700)

are available in the Git repository at:

https://gitlab.com/stefanha/qemu.git tags/block-pull-request

for you to fetch changes up to bef2e050d6a7feb865854c65570c496ac5a8cf53:

util/event-loop-base: Introduce options to set the thread pool size (2022-05-04 17:02:19 +0100)

----------------------------------------------------------------
Pull request

Add new thread-pool-min/thread-pool-max parameters to control the thread pool
used for async I/O.

----------------------------------------------------------------

Nicolas Saenz Julienne (3):
  Introduce event-loop-base abstract class
  util/main-loop: Introduce the main loop into QOM
  util/event-loop-base: Introduce options to set the thread pool size

-- 
2.35.1

From: Nicolas Saenz Julienne <nsaenzju@redhat.com>

Introduce the 'event-loop-base' abstract class, it'll hold the
properties common to all event loops and provide the necessary hooks for
their creation and maintenance. Then have iothread inherit from it.

EventLoopBaseClass is defined as user creatable and provides a hook for
its children to attach themselves to the user creatable class 'complete'
function. It also provides an update_params() callback to propagate
property changes onto its children.

The new 'event-loop-base' class will live in the root directory. It is
built on its own using the 'link_whole' option (there are no direct
function dependencies between the class and its children, it all happens
trough 'constructor' magic). And also imposes new compilation
dependencies:

qom <- event-loop-base <- blockdev (iothread.c)

And in subsequent patches:

qom <- event-loop-base <- qemuutil (util/main-loop.c)

All this forced some amount of reordering in meson.build:

- Moved qom build definition before qemuutil. Doing it the other way
   around (i.e. moving qemuutil after qom) isn't possible as a lot of
   core libraries that live in between the two depend on it.

- Process the 'hw' subdir earlier, as it introduces files into the
   'qom' source set.

No functional changes intended.

Signed-off-by: Nicolas Saenz Julienne <nsaenzju@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Acked-by: Markus Armbruster <armbru@redhat.com>
Message-id: 20220425075723.20019-2-nsaenzju@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 qapi/qom.json                    |  22 +++++--
 meson.build                      |  23 ++++---
 include/sysemu/event-loop-base.h |  36 +++++++++++
 include/sysemu/iothread.h        |   6 +-
 event-loop-base.c                | 104 +++++++++++++++++++++++++++++++
 iothread.c                       |  65 ++++++-------------
 6 files changed, 192 insertions(+), 64 deletions(-)
 create mode 100644 include/sysemu/event-loop-base.h
 create mode 100644 event-loop-base.c

diff --git a/qapi/qom.json b/qapi/qom.json
index XXXXXXX..XXXXXXX 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -XXX,XX +XXX,XX @@
             '*repeat': 'bool',
             '*grab-toggle': 'GrabToggleKeys' } }
 
+##
+# @EventLoopBaseProperties:
+#
+# Common properties for event loops
+#
+# @aio-max-batch: maximum number of requests in a batch for the AIO engine,
+#                 0 means that the engine will use its default.
+#                 (default: 0)
+#
+# Since: 7.1
+##
+{ 'struct': 'EventLoopBaseProperties',
+  'data': { '*aio-max-batch': 'int' } }
+
 ##
 # @IothreadProperties:
 #
@@ -XXX,XX +XXX,XX @@
 #               algorithm detects it is spending too long polling without
 #               encountering events. 0 selects a default behaviour (default: 0)
 #
-# @aio-max-batch: maximum number of requests in a batch for the AIO engine,
-#                 0 means that the engine will use its default
-#                 (default:0, since 6.1)
+# The @aio-max-batch option is available since 6.1.
 #
 # Since: 2.0
 ##
 { 'struct': 'IothreadProperties',
+  'base': 'EventLoopBaseProperties',
   'data': { '*poll-max-ns': 'int',
             '*poll-grow': 'int',
-            '*poll-shrink': 'int',
-            '*aio-max-batch': 'int' } }
+            '*poll-shrink': 'int' } }
 
 ##
 # @MemoryBackendProperties:
diff --git a/meson.build b/meson.build
index XXXXXXX..XXXXXXX 100644
--- a/meson.build
+++ b/meson.build
@@ -XXX,XX +XXX,XX @@ subdir('qom')
 subdir('authz')
 subdir('crypto')
 subdir('ui')
+subdir('hw')
 
 
 if enable_modules
@@ -XXX,XX +XXX,XX @@ if enable_modules
   modulecommon = declare_dependency(link_whole: libmodulecommon, compile_args: '-DBUILD_DSO')
 endif
 
+qom_ss = qom_ss.apply(config_host, strict: false)
+libqom = static_library('qom', qom_ss.sources() + genh,
+                        dependencies: [qom_ss.dependencies()],
+                        name_suffix: 'fa')
+qom = declare_dependency(link_whole: libqom)
+
+event_loop_base = files('event-loop-base.c')
+event_loop_base = static_library('event-loop-base', sources: event_loop_base + genh,
+                                 build_by_default: true)
+event_loop_base = declare_dependency(link_whole: event_loop_base,
+                                     dependencies: [qom])
+
 stub_ss = stub_ss.apply(config_all, strict: false)
 
 util_ss.add_all(trace_ss)
@@ -XXX,XX +XXX,XX @@ subdir('monitor')
 subdir('net')
 subdir('replay')
 subdir('semihosting')
-subdir('hw')
 subdir('tcg')
 subdir('fpu')
 subdir('accel')
@@ -XXX,XX +XXX,XX @@ qemu_syms = custom_target('qemu.syms', output: 'qemu.syms',
                              capture: true,
                              command: [undefsym, nm, '@INPUT@'])
 
-qom_ss = qom_ss.apply(config_host, strict: false)
-libqom = static_library('qom', qom_ss.sources() + genh,
-                        dependencies: [qom_ss.dependencies()],
-                        name_suffix: 'fa')
-
-qom = declare_dependency(link_whole: libqom)
-
 authz_ss = authz_ss.apply(config_host, strict: false)
 libauthz = static_library('authz', authz_ss.sources() + genh,
                           dependencies: [authz_ss.dependencies()],
@@ -XXX,XX +XXX,XX @@ libblockdev = static_library('blockdev', blockdev_ss.sources() + genh,
                              build_by_default: false)
 
 blockdev = declare_dependency(link_whole: [libblockdev],
-                              dependencies: [block])
+                              dependencies: [block, event_loop_base])
 
 qmp_ss = qmp_ss.apply(config_host, strict: false)
 libqmp = static_library('qmp', qmp_ss.sources() + genh,
diff --git a/include/sysemu/event-loop-base.h b/include/sysemu/event-loop-base.h
new file mode 100644
index XXXXXXX..XXXXXXX
--- /dev/null
+++ b/include/sysemu/event-loop-base.h
@@ -XXX,XX +XXX,XX @@
+/*
+ * QEMU event-loop backend
+ *
+ * Copyright (C) 2022 Red Hat Inc
+ *
+ * Authors:
+ *  Nicolas Saenz Julienne <nsaenzju@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#ifndef QEMU_EVENT_LOOP_BASE_H
+#define QEMU_EVENT_LOOP_BASE_H
+
+#include "qom/object.h"
+#include "block/aio.h"
+#include "qemu/typedefs.h"
+
+#define TYPE_EVENT_LOOP_BASE         "event-loop-base"
+OBJECT_DECLARE_TYPE(EventLoopBase, EventLoopBaseClass,
+                    EVENT_LOOP_BASE)
+
+struct EventLoopBaseClass {
+    ObjectClass parent_class;
+
+    void (*init)(EventLoopBase *base, Error **errp);
+    void (*update_params)(EventLoopBase *base, Error **errp);
+};
+
+struct EventLoopBase {
+    Object parent;
+
+    /* AioContext AIO engine parameters */
+    int64_t aio_max_batch;
+};
+#endif
diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h
index XXXXXXX..XXXXXXX 100644
--- a/include/sysemu/iothread.h
+++ b/include/sysemu/iothread.h
@@ -XXX,XX +XXX,XX @@
 #include "block/aio.h"
 #include "qemu/thread.h"
 #include "qom/object.h"
+#include "sysemu/event-loop-base.h"
 
 #define TYPE_IOTHREAD "iothread"
 
 struct IOThread {
-    Object parent_obj;
+    EventLoopBase parent_obj;
 
     QemuThread thread;
     AioContext *ctx;
@@ -XXX,XX +XXX,XX @@ struct IOThread {
     int64_t poll_max_ns;
     int64_t poll_grow;
     int64_t poll_shrink;
-
-    /* AioContext AIO engine parameters */
-    int64_t aio_max_batch;
 };
 typedef struct IOThread IOThread;
 
diff --git a/event-loop-base.c b/event-loop-base.c
new file mode 100644
index XXXXXXX..XXXXXXX
--- /dev/null
+++ b/event-loop-base.c
@@ -XXX,XX +XXX,XX @@
+/*
+ * QEMU event-loop base
+ *
+ * Copyright (C) 2022 Red Hat Inc
+ *
+ * Authors:
+ *  Stefan Hajnoczi <stefanha@redhat.com>
+ *  Nicolas Saenz Julienne <nsaenzju@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qom/object_interfaces.h"
+#include "qapi/error.h"
+#include "sysemu/event-loop-base.h"
+
+typedef struct {
+    const char *name;
+    ptrdiff_t offset; /* field's byte offset in EventLoopBase struct */
+} EventLoopBaseParamInfo;
+
+static EventLoopBaseParamInfo aio_max_batch_info = {
+    "aio-max-batch", offsetof(EventLoopBase, aio_max_batch),
+};
+
+static void event_loop_base_get_param(Object *obj, Visitor *v,
+        const char *name, void *opaque, Error **errp)
+{
+    EventLoopBase *event_loop_base = EVENT_LOOP_BASE(obj);
+    EventLoopBaseParamInfo *info = opaque;
+    int64_t *field = (void *)event_loop_base + info->offset;
+
+    visit_type_int64(v, name, field, errp);
+}
+
+static void event_loop_base_set_param(Object *obj, Visitor *v,
+        const char *name, void *opaque, Error **errp)
+{
+    EventLoopBaseClass *bc = EVENT_LOOP_BASE_GET_CLASS(obj);
+    EventLoopBase *base = EVENT_LOOP_BASE(obj);
+    EventLoopBaseParamInfo *info = opaque;
+    int64_t *field = (void *)base + info->offset;
+    int64_t value;
+
+    if (!visit_type_int64(v, name, &value, errp)) {
+        return;
+    }
+
+    if (value < 0) {
+        error_setg(errp, "%s value must be in range [0, %" PRId64 "]",
+                   info->name, INT64_MAX);
+        return;
+    }
+
+    *field = value;
+
+    if (bc->update_params) {
+        bc->update_params(base, errp);
+    }
+
+    return;
+}
+
+static void event_loop_base_complete(UserCreatable *uc, Error **errp)
+{
+    EventLoopBaseClass *bc = EVENT_LOOP_BASE_GET_CLASS(uc);
+    EventLoopBase *base = EVENT_LOOP_BASE(uc);
+
+    if (bc->init) {
+        bc->init(base, errp);
+    }
+}
+
+static void event_loop_base_class_init(ObjectClass *klass, void *class_data)
+{
+    UserCreatableClass *ucc = USER_CREATABLE_CLASS(klass);
+    ucc->complete = event_loop_base_complete;
+
+    object_class_property_add(klass, "aio-max-batch", "int",
+                              event_loop_base_get_param,
+                              event_loop_base_set_param,
+                              NULL, &aio_max_batch_info);
+}
+
+static const TypeInfo event_loop_base_info = {
+    .name = TYPE_EVENT_LOOP_BASE,
+    .parent = TYPE_OBJECT,
+    .instance_size = sizeof(EventLoopBase),
+    .class_size = sizeof(EventLoopBaseClass),
+    .class_init = event_loop_base_class_init,
+    .abstract = true,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_USER_CREATABLE },
+        { }
+    }
+};
+
+static void register_types(void)
+{
+    type_register_static(&event_loop_base_info);
+}
+type_init(register_types);
diff --git a/iothread.c b/iothread.c
index XXXXXXX..XXXXXXX 100644
--- a/iothread.c
+++ b/iothread.c
@@ -XXX,XX +XXX,XX @@
 #include "qemu/module.h"
 #include "block/aio.h"
 #include "block/block.h"
+#include "sysemu/event-loop-base.h"
 #include "sysemu/iothread.h"
 #include "qapi/error.h"
 #include "qapi/qapi-commands-misc.h"
@@ -XXX,XX +XXX,XX @@ static void iothread_init_gcontext(IOThread *iothread)
     iothread->main_loop = g_main_loop_new(iothread->worker_context, TRUE);
 }
 
-static void iothread_set_aio_context_params(IOThread *iothread, Error **errp)
+static void iothread_set_aio_context_params(EventLoopBase *base, Error **errp)
 {
+    IOThread *iothread = IOTHREAD(base);
     ERRP_GUARD();
 
+    if (!iothread->ctx) {
+        return;
+    }
+
     aio_context_set_poll_params(iothread->ctx,
                                 iothread->poll_max_ns,
                                 iothread->poll_grow,
@@ -XXX,XX +XXX,XX @@ static void iothread_set_aio_context_params(IOThread *iothread, Error **errp)
     }
 
     aio_context_set_aio_params(iothread->ctx,
-                               iothread->aio_max_batch,
+                               iothread->parent_obj.aio_max_batch,
                                errp);
 }
 
-static void iothread_complete(UserCreatable *obj, Error **errp)
+
+static void iothread_init(EventLoopBase *base, Error **errp)
 {
     Error *local_error = NULL;
-    IOThread *iothread = IOTHREAD(obj);
+    IOThread *iothread = IOTHREAD(base);
     char *thread_name;
 
     iothread->stopping = false;
@@ -XXX,XX +XXX,XX @@ static void iothread_complete(UserCreatable *obj, Error **errp)
      */
     iothread_init_gcontext(iothread);
 
-    iothread_set_aio_context_params(iothread, &local_error);
+    iothread_set_aio_context_params(base, &local_error);
     if (local_error) {
         error_propagate(errp, local_error);
         aio_context_unref(iothread->ctx);
@@ -XXX,XX +XXX,XX @@ static void iothread_complete(UserCreatable *obj, Error **errp)
      * to inherit.
      */
     thread_name = g_strdup_printf("IO %s",
-                        object_get_canonical_path_component(OBJECT(obj)));
+                        object_get_canonical_path_component(OBJECT(base)));
     qemu_thread_create(&iothread->thread, thread_name, iothread_run,
                        iothread, QEMU_THREAD_JOINABLE);
     g_free(thread_name);
@@ -XXX,XX +XXX,XX @@ static IOThreadParamInfo poll_grow_info = {
 static IOThreadParamInfo poll_shrink_info = {
     "poll-shrink", offsetof(IOThread, poll_shrink),
 };
-static IOThreadParamInfo aio_max_batch_info = {
-    "aio-max-batch", offsetof(IOThread, aio_max_batch),
-};
 
 static void iothread_get_param(Object *obj, Visitor *v,
         const char *name, IOThreadParamInfo *info, Error **errp)
@@ -XXX,XX +XXX,XX @@ static void iothread_set_poll_param(Object *obj, Visitor *v,
     }
 }
 
-static void iothread_get_aio_param(Object *obj, Visitor *v,
-        const char *name, void *opaque, Error **errp)
-{
-    IOThreadParamInfo *info = opaque;
-
-    iothread_get_param(obj, v, name, info, errp);
-}
-
-static void iothread_set_aio_param(Object *obj, Visitor *v,
-        const char *name, void *opaque, Error **errp)
-{
-    IOThread *iothread = IOTHREAD(obj);
-    IOThreadParamInfo *info = opaque;
-
-    if (!iothread_set_param(obj, v, name, info, errp)) {
-        return;
-    }
-
-    if (iothread->ctx) {
-        aio_context_set_aio_params(iothread->ctx,
-                                   iothread->aio_max_batch,
-                                   errp);
-    }
-}
-
 static void iothread_class_init(ObjectClass *klass, void *class_data)
 {
-    UserCreatableClass *ucc = USER_CREATABLE_CLASS(klass);
-    ucc->complete = iothread_complete;
+    EventLoopBaseClass *bc = EVENT_LOOP_BASE_CLASS(klass);
+
+    bc->init = iothread_init;
+    bc->update_params = iothread_set_aio_context_params;
 
     object_class_property_add(klass, "poll-max-ns", "int",
                               iothread_get_poll_param,
@@ -XXX,XX +XXX,XX @@ static void iothread_class_init(ObjectClass *klass, void *class_data)
                               iothread_get_poll_param,
                               iothread_set_poll_param,
                               NULL, &poll_shrink_info);
-    object_class_property_add(klass, "aio-max-batch", "int",
-                              iothread_get_aio_param,
-                              iothread_set_aio_param,
-                              NULL, &aio_max_batch_info);
 }
 
 static const TypeInfo iothread_info = {
     .name = TYPE_IOTHREAD,
-    .parent = TYPE_OBJECT,
+    .parent = TYPE_EVENT_LOOP_BASE,
     .class_init = iothread_class_init,
     .instance_size = sizeof(IOThread),
     .instance_init = iothread_instance_init,
     .instance_finalize = iothread_instance_finalize,
-    .interfaces = (InterfaceInfo[]) {
-        {TYPE_USER_CREATABLE},
-        {}
-    },
 };
 
 static void iothread_register_types(void)
@@ -XXX,XX +XXX,XX @@ static int query_one_iothread(Object *object, void *opaque)
     info->poll_max_ns = iothread->poll_max_ns;
     info->poll_grow = iothread->poll_grow;
     info->poll_shrink = iothread->poll_shrink;
-    info->aio_max_batch = iothread->aio_max_batch;
+    info->aio_max_batch = iothread->parent_obj.aio_max_batch;
 
     QAPI_LIST_APPEND(*tail, info);
     return 0;
-- 
2.35.1

From: Nicolas Saenz Julienne <nsaenzju@redhat.com>

'event-loop-base' provides basic property handling for all 'AioContext'
based event loops. So let's define a new 'MainLoopClass' that inherits
from it. This will permit tweaking the main loop's properties through
qapi as well as through the command line using the '-object' keyword[1].
Only one instance of 'MainLoopClass' might be created at any time.

'EventLoopBaseClass' learns a new callback, 'can_be_deleted()' so as to
mark 'MainLoop' as non-deletable.

[1] For example:
      -object main-loop,id=main-loop,aio-max-batch=<value>

Signed-off-by: Nicolas Saenz Julienne <nsaenzju@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Acked-by: Markus Armbruster <armbru@redhat.com>
Message-id: 20220425075723.20019-3-nsaenzju@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 qapi/qom.json                    | 13 ++++++++
 meson.build                      |  3 +-
 include/qemu/main-loop.h         | 10 ++++++
 include/sysemu/event-loop-base.h |  1 +
 event-loop-base.c                | 13 ++++++++
 util/main-loop.c                 | 56 ++++++++++++++++++++++++++++++++
 6 files changed, 95 insertions(+), 1 deletion(-)

diff --git a/qapi/qom.json b/qapi/qom.json
index XXXXXXX..XXXXXXX 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -XXX,XX +XXX,XX @@
             '*poll-grow': 'int',
             '*poll-shrink': 'int' } }
 
+##
+# @MainLoopProperties:
+#
+# Properties for the main-loop object.
+#
+# Since: 7.1
+##
+{ 'struct': 'MainLoopProperties',
+  'base': 'EventLoopBaseProperties',
+  'data': {} }
+
 ##
 # @MemoryBackendProperties:
 #
@@ -XXX,XX +XXX,XX @@
     { 'name': 'input-linux',
       'if': 'CONFIG_LINUX' },
     'iothread',
+    'main-loop',
     { 'name': 'memory-backend-epc',
       'if': 'CONFIG_LINUX' },
     'memory-backend-file',
@@ -XXX,XX +XXX,XX @@
       'input-linux':                { 'type': 'InputLinuxProperties',
                                       'if': 'CONFIG_LINUX' },
       'iothread':                   'IothreadProperties',
+      'main-loop':                  'MainLoopProperties',
       'memory-backend-epc':         { 'type': 'MemoryBackendEpcProperties',
                                       'if': 'CONFIG_LINUX' },
       'memory-backend-file':        'MemoryBackendFileProperties',
diff --git a/meson.build b/meson.build
index XXXXXXX..XXXXXXX 100644
--- a/meson.build
+++ b/meson.build
@@ -XXX,XX +XXX,XX @@ libqemuutil = static_library('qemuutil',
                              sources: util_ss.sources() + stub_ss.sources() + genh,
                              dependencies: [util_ss.dependencies(), libm, threads, glib, socket, malloc, pixman])
 qemuutil = declare_dependency(link_with: libqemuutil,
-                              sources: genh + version_res)
+                              sources: genh + version_res,
+                              dependencies: [event_loop_base])
 
 if have_system or have_user
   decodetree = generator(find_program('scripts/decodetree.py'),
diff --git a/include/qemu/main-loop.h b/include/qemu/main-loop.h
index XXXXXXX..XXXXXXX 100644
--- a/include/qemu/main-loop.h
+++ b/include/qemu/main-loop.h
@@ -XXX,XX +XXX,XX @@
 #define QEMU_MAIN_LOOP_H
 
 #include "block/aio.h"
+#include "qom/object.h"
+#include "sysemu/event-loop-base.h"
 
 #define SIG_IPI SIGUSR1
 
+#define TYPE_MAIN_LOOP  "main-loop"
+OBJECT_DECLARE_TYPE(MainLoop, MainLoopClass, MAIN_LOOP)
+
+struct MainLoop {
+    EventLoopBase parent_obj;
+};
+typedef struct MainLoop MainLoop;
+
 /**
  * qemu_init_main_loop: Set up the process so that it can run the main loop.
  *
diff --git a/include/sysemu/event-loop-base.h b/include/sysemu/event-loop-base.h
index XXXXXXX..XXXXXXX 100644
--- a/include/sysemu/event-loop-base.h
+++ b/include/sysemu/event-loop-base.h
@@ -XXX,XX +XXX,XX @@ struct EventLoopBaseClass {
 
     void (*init)(EventLoopBase *base, Error **errp);
     void (*update_params)(EventLoopBase *base, Error **errp);
+    bool (*can_be_deleted)(EventLoopBase *base);
 };
 
 struct EventLoopBase {
diff --git a/event-loop-base.c b/event-loop-base.c
index XXXXXXX..XXXXXXX 100644
--- a/event-loop-base.c
+++ b/event-loop-base.c
@@ -XXX,XX +XXX,XX @@ static void event_loop_base_complete(UserCreatable *uc, Error **errp)
     }
 }
 
+static bool event_loop_base_can_be_deleted(UserCreatable *uc)
+{
+    EventLoopBaseClass *bc = EVENT_LOOP_BASE_GET_CLASS(uc);
+    EventLoopBase *backend = EVENT_LOOP_BASE(uc);
+
+    if (bc->can_be_deleted) {
+        return bc->can_be_deleted(backend);
+    }
+
+    return true;
+}
+
 static void event_loop_base_class_init(ObjectClass *klass, void *class_data)
 {
     UserCreatableClass *ucc = USER_CREATABLE_CLASS(klass);
     ucc->complete = event_loop_base_complete;
+    ucc->can_be_deleted = event_loop_base_can_be_deleted;
 
     object_class_property_add(klass, "aio-max-batch", "int",
                               event_loop_base_get_param,
diff --git a/util/main-loop.c b/util/main-loop.c
index XXXXXXX..XXXXXXX 100644
--- a/util/main-loop.c
+++ b/util/main-loop.c
@@ -XXX,XX +XXX,XX @@
 #include "qemu/error-report.h"
 #include "qemu/queue.h"
 #include "qemu/compiler.h"
+#include "qom/object.h"
 
 #ifndef _WIN32
 #include <sys/wait.h>
@@ -XXX,XX +XXX,XX @@ int qemu_init_main_loop(Error **errp)
     return 0;
 }
 
+static void main_loop_update_params(EventLoopBase *base, Error **errp)
+{
+    if (!qemu_aio_context) {
+        error_setg(errp, "qemu aio context not ready");
+        return;
+    }
+
+    aio_context_set_aio_params(qemu_aio_context, base->aio_max_batch, errp);
+}
+
+MainLoop *mloop;
+
+static void main_loop_init(EventLoopBase *base, Error **errp)
+{
+    MainLoop *m = MAIN_LOOP(base);
+
+    if (mloop) {
+        error_setg(errp, "only one main-loop instance allowed");
+        return;
+    }
+
+    main_loop_update_params(base, errp);
+
+    mloop = m;
+    return;
+}
+
+static bool main_loop_can_be_deleted(EventLoopBase *base)
+{
+    return false;
+}
+
+static void main_loop_class_init(ObjectClass *oc, void *class_data)
+{
+    EventLoopBaseClass *bc = EVENT_LOOP_BASE_CLASS(oc);
+
+    bc->init = main_loop_init;
+    bc->update_params = main_loop_update_params;
+    bc->can_be_deleted = main_loop_can_be_deleted;
+}
+
+static const TypeInfo main_loop_info = {
+    .name = TYPE_MAIN_LOOP,
+    .parent = TYPE_EVENT_LOOP_BASE,
+    .class_init = main_loop_class_init,
+    .instance_size = sizeof(MainLoop),
+};
+
+static void main_loop_register_types(void)
+{
+    type_register_static(&main_loop_info);
+}
+
+type_init(main_loop_register_types)
+
 static int max_priority;
 
 #ifndef _WIN32
-- 
2.35.1

From: Nicolas Saenz Julienne <nsaenzju@redhat.com>

The thread pool regulates itself: when idle, it kills threads until
empty, when in demand, it creates new threads until full. This behaviour
doesn't play well with latency sensitive workloads where the price of
creating a new thread is too high. For example, when paired with qemu's
'-mlock', or using safety features like SafeStack, creating a new thread
has been measured take multiple milliseconds.

In order to mitigate this let's introduce a new 'EventLoopBase'
property to set the thread pool size. The threads will be created during
the pool's initialization or upon updating the property's value, remain
available during its lifetime regardless of demand, and destroyed upon
freeing it. A properly characterized workload will then be able to
configure the pool to avoid any latency spikes.

Signed-off-by: Nicolas Saenz Julienne <nsaenzju@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Acked-by: Markus Armbruster <armbru@redhat.com>
Message-id: 20220425075723.20019-4-nsaenzju@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 qapi/qom.json                    | 10 +++++-
 include/block/aio.h              | 10 ++++++
 include/block/thread-pool.h      |  3 ++
 include/sysemu/event-loop-base.h |  4 +++
 event-loop-base.c                | 23 +++++++++++++
 iothread.c                       |  3 ++
 util/aio-posix.c                 |  1 +
 util/async.c                     | 20 ++++++++++++
 util/main-loop.c                 |  9 ++++++
 util/thread-pool.c               | 55 +++++++++++++++++++++++++++++---
 10 files changed, 133 insertions(+), 5 deletions(-)

diff --git a/qapi/qom.json b/qapi/qom.json
index XXXXXXX..XXXXXXX 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -XXX,XX +XXX,XX @@
 #                 0 means that the engine will use its default.
 #                 (default: 0)
 #
+# @thread-pool-min: minimum number of threads reserved in the thread pool
+#                   (default:0)
+#
+# @thread-pool-max: maximum number of threads the thread pool can contain
+#                   (default:64)
+#
 # Since: 7.1
 ##
 { 'struct': 'EventLoopBaseProperties',
-  'data': { '*aio-max-batch': 'int' } }
+  'data': { '*aio-max-batch': 'int',
+            '*thread-pool-min': 'int',
+            '*thread-pool-max': 'int' } }
 
 ##
 # @IothreadProperties:
diff --git a/include/block/aio.h b/include/block/aio.h
index XXXXXXX..XXXXXXX 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -XXX,XX +XXX,XX @@ struct AioContext {
     QSLIST_HEAD(, Coroutine) scheduled_coroutines;
     QEMUBH *co_schedule_bh;
 
+    int thread_pool_min;
+    int thread_pool_max;
     /* Thread pool for performing work and receiving completion callbacks.
      * Has its own locking.
      */
@@ -XXX,XX +XXX,XX @@ void aio_context_set_poll_params(AioContext *ctx, int64_t max_ns,
 void aio_context_set_aio_params(AioContext *ctx, int64_t max_batch,
                                 Error **errp);
 
+/**
+ * aio_context_set_thread_pool_params:
+ * @ctx: the aio context
+ * @min: min number of threads to have readily available in the thread pool
+ * @min: max number of threads the thread pool can contain
+ */
+void aio_context_set_thread_pool_params(AioContext *ctx, int64_t min,
+                                        int64_t max, Error **errp);
 #endif
diff --git a/include/block/thread-pool.h b/include/block/thread-pool.h
index XXXXXXX..XXXXXXX 100644
--- a/include/block/thread-pool.h
+++ b/include/block/thread-pool.h
@@ -XXX,XX +XXX,XX @@
 
 #include "block/block.h"
 
+#define THREAD_POOL_MAX_THREADS_DEFAULT         64
+
 typedef int ThreadPoolFunc(void *opaque);
 
 typedef struct ThreadPool ThreadPool;
@@ -XXX,XX +XXX,XX @@ BlockAIOCB *thread_pool_submit_aio(ThreadPool *pool,
 int coroutine_fn thread_pool_submit_co(ThreadPool *pool,
         ThreadPoolFunc *func, void *arg);
 void thread_pool_submit(ThreadPool *pool, ThreadPoolFunc *func, void *arg);
+void thread_pool_update_params(ThreadPool *pool, struct AioContext *ctx);
 
 #endif
diff --git a/include/sysemu/event-loop-base.h b/include/sysemu/event-loop-base.h
index XXXXXXX..XXXXXXX 100644
--- a/include/sysemu/event-loop-base.h
+++ b/include/sysemu/event-loop-base.h
@@ -XXX,XX +XXX,XX @@ struct EventLoopBase {
 
     /* AioContext AIO engine parameters */
     int64_t aio_max_batch;
+
+    /* AioContext thread pool parameters */
+    int64_t thread_pool_min;
+    int64_t thread_pool_max;
 };
 #endif
diff --git a/event-loop-base.c b/event-loop-base.c
index XXXXXXX..XXXXXXX 100644
--- a/event-loop-base.c
+++ b/event-loop-base.c
@@ -XXX,XX +XXX,XX @@
 #include "qemu/osdep.h"
 #include "qom/object_interfaces.h"
 #include "qapi/error.h"
+#include "block/thread-pool.h"
 #include "sysemu/event-loop-base.h"
 
 typedef struct {
@@ -XXX,XX +XXX,XX @@ typedef struct {
     ptrdiff_t offset; /* field's byte offset in EventLoopBase struct */
 } EventLoopBaseParamInfo;
 
+static void event_loop_base_instance_init(Object *obj)
+{
+    EventLoopBase *base = EVENT_LOOP_BASE(obj);
+
+    base->thread_pool_max = THREAD_POOL_MAX_THREADS_DEFAULT;
+}
+
 static EventLoopBaseParamInfo aio_max_batch_info = {
     "aio-max-batch", offsetof(EventLoopBase, aio_max_batch),
 };
+static EventLoopBaseParamInfo thread_pool_min_info = {
+    "thread-pool-min", offsetof(EventLoopBase, thread_pool_min),
+};
+static EventLoopBaseParamInfo thread_pool_max_info = {
+    "thread-pool-max", offsetof(EventLoopBase, thread_pool_max),
+};
 
 static void event_loop_base_get_param(Object *obj, Visitor *v,
         const char *name, void *opaque, Error **errp)
@@ -XXX,XX +XXX,XX @@ static void event_loop_base_class_init(ObjectClass *klass, void *class_data)
                               event_loop_base_get_param,
                               event_loop_base_set_param,
                               NULL, &aio_max_batch_info);
+    object_class_property_add(klass, "thread-pool-min", "int",
+                              event_loop_base_get_param,
+                              event_loop_base_set_param,
+                              NULL, &thread_pool_min_info);
+    object_class_property_add(klass, "thread-pool-max", "int",
+                              event_loop_base_get_param,
+                              event_loop_base_set_param,
+                              NULL, &thread_pool_max_info);
 }
 
 static const TypeInfo event_loop_base_info = {
     .name = TYPE_EVENT_LOOP_BASE,
     .parent = TYPE_OBJECT,
     .instance_size = sizeof(EventLoopBase),
+    .instance_init = event_loop_base_instance_init,
     .class_size = sizeof(EventLoopBaseClass),
     .class_init = event_loop_base_class_init,
     .abstract = true,
diff --git a/iothread.c b/iothread.c
index XXXXXXX..XXXXXXX 100644
--- a/iothread.c
+++ b/iothread.c
@@ -XXX,XX +XXX,XX @@ static void iothread_set_aio_context_params(EventLoopBase *base, Error **errp)
     aio_context_set_aio_params(iothread->ctx,
                                iothread->parent_obj.aio_max_batch,
                                errp);
+
+    aio_context_set_thread_pool_params(iothread->ctx, base->thread_pool_min,
+                                       base->thread_pool_max, errp);
 }
 
 
diff --git a/util/aio-posix.c b/util/aio-posix.c
index XXXXXXX..XXXXXXX 100644
--- a/util/aio-posix.c
+++ b/util/aio-posix.c
@@ -XXX,XX +XXX,XX @@
 
 #include "qemu/osdep.h"
 #include "block/block.h"
+#include "block/thread-pool.h"
 #include "qemu/main-loop.h"
 #include "qemu/rcu.h"
 #include "qemu/rcu_queue.h"
diff --git a/util/async.c b/util/async.c
index XXXXXXX..XXXXXXX 100644
--- a/util/async.c
+++ b/util/async.c
@@ -XXX,XX +XXX,XX @@ AioContext *aio_context_new(Error **errp)
 
     ctx->aio_max_batch = 0;
 
+    ctx->thread_pool_min = 0;
+    ctx->thread_pool_max = THREAD_POOL_MAX_THREADS_DEFAULT;
+
     return ctx;
 fail:
     g_source_destroy(&ctx->source);
@@ -XXX,XX +XXX,XX @@ void qemu_set_current_aio_context(AioContext *ctx)
     assert(!get_my_aiocontext());
     set_my_aiocontext(ctx);
 }
+
+void aio_context_set_thread_pool_params(AioContext *ctx, int64_t min,
+                                        int64_t max, Error **errp)
+{
+
+    if (min > max || !max || min > INT_MAX || max > INT_MAX) {
+        error_setg(errp, "bad thread-pool-min/thread-pool-max values");
+        return;
+    }
+
+    ctx->thread_pool_min = min;
+    ctx->thread_pool_max = max;
+
+    if (ctx->thread_pool) {
+        thread_pool_update_params(ctx->thread_pool, ctx);
+    }
+}
diff --git a/util/main-loop.c b/util/main-loop.c
index XXXXXXX..XXXXXXX 100644
--- a/util/main-loop.c
+++ b/util/main-loop.c
@@ -XXX,XX +XXX,XX @@
 #include "sysemu/replay.h"
 #include "qemu/main-loop.h"
 #include "block/aio.h"
+#include "block/thread-pool.h"
 #include "qemu/error-report.h"
 #include "qemu/queue.h"
 #include "qemu/compiler.h"
@@ -XXX,XX +XXX,XX @@ int qemu_init_main_loop(Error **errp)
 
 static void main_loop_update_params(EventLoopBase *base, Error **errp)
 {
+    ERRP_GUARD();
+
     if (!qemu_aio_context) {
         error_setg(errp, "qemu aio context not ready");
         return;
     }
 
     aio_context_set_aio_params(qemu_aio_context, base->aio_max_batch, errp);
+    if (*errp) {
+        return;
+    }
+
+    aio_context_set_thread_pool_params(qemu_aio_context, base->thread_pool_min,
+                                       base->thread_pool_max, errp);
 }
 
 MainLoop *mloop;
diff --git a/util/thread-pool.c b/util/thread-pool.c
index XXXXXXX..XXXXXXX 100644
--- a/util/thread-pool.c
+++ b/util/thread-pool.c
@@ -XXX,XX +XXX,XX @@ struct ThreadPool {
     QemuMutex lock;
     QemuCond worker_stopped;
     QemuSemaphore sem;
-    int max_threads;
     QEMUBH *new_thread_bh;
 
     /* The following variables are only accessed from one AioContext. */
@@ -XXX,XX +XXX,XX @@ struct ThreadPool {
     int new_threads;     /* backlog of threads we need to create */
     int pending_threads; /* threads created but not running yet */
     bool stopping;
+    int min_threads;
+    int max_threads;
 };
 
+static inline bool back_to_sleep(ThreadPool *pool, int ret)
+{
+    /*
+     * The semaphore timed out, we should exit the loop except when:
+     *  - There is work to do, we raced with the signal.
+     *  - The max threads threshold just changed, we raced with the signal.
+     *  - The thread pool forces a minimum number of readily available threads.
+     */
+    if (ret == -1 && (!QTAILQ_EMPTY(&pool->request_list) ||
+            pool->cur_threads > pool->max_threads ||
+            pool->cur_threads <= pool->min_threads)) {
+            return true;
+    }
+
+    return false;
+}
+
 static void *worker_thread(void *opaque)
 {
     ThreadPool *pool = opaque;
@@ -XXX,XX +XXX,XX @@ static void *worker_thread(void *opaque)
             ret = qemu_sem_timedwait(&pool->sem, 10000);
             qemu_mutex_lock(&pool->lock);
             pool->idle_threads--;
-        } while (ret == -1 && !QTAILQ_EMPTY(&pool->request_list));
-        if (ret == -1 || pool->stopping) {
+        } while (back_to_sleep(pool, ret));
+        if (ret == -1 || pool->stopping ||
+            pool->cur_threads > pool->max_threads) {
             break;
         }
 
@@ -XXX,XX +XXX,XX @@ void thread_pool_submit(ThreadPool *pool, ThreadPoolFunc *func, void *arg)
     thread_pool_submit_aio(pool, func, arg, NULL, NULL);
 }
 
+void thread_pool_update_params(ThreadPool *pool, AioContext *ctx)
+{
+    qemu_mutex_lock(&pool->lock);
+
+    pool->min_threads = ctx->thread_pool_min;
+    pool->max_threads = ctx->thread_pool_max;
+
+    /*
+     * We either have to:
+     *  - Increase the number available of threads until over the min_threads
+     *    threshold.
+     *  - Decrease the number of available threads until under the max_threads
+     *    threshold.
+     *  - Do nothing. The current number of threads fall in between the min and
+     *    max thresholds. We'll let the pool manage itself.
+     */
+    for (int i = pool->cur_threads; i < pool->min_threads; i++) {
+        spawn_thread(pool);
+    }
+
+    for (int i = pool->cur_threads; i > pool->max_threads; i--) {
+        qemu_sem_post(&pool->sem);
+    }
+
+    qemu_mutex_unlock(&pool->lock);
+}
+
 static void thread_pool_init_one(ThreadPool *pool, AioContext *ctx)
 {
     if (!ctx) {
@@ -XXX,XX +XXX,XX @@ static void thread_pool_init_one(ThreadPool *pool, AioContext *ctx)
     qemu_mutex_init(&pool->lock);
     qemu_cond_init(&pool->worker_stopped);
     qemu_sem_init(&pool->sem, 0);
-    pool->max_threads = 64;
     pool->new_thread_bh = aio_bh_new(ctx, spawn_thread_bh_fn, pool);
 
     QLIST_INIT(&pool->head);
     QTAILQ_INIT(&pool->request_list);
+
+    thread_pool_update_params(pool, ctx);
 }
 
 ThreadPool *thread_pool_new(AioContext *ctx)
-- 
2.35.1

The following changes since commit 19b599f7664b2ebfd0f405fb79c14dd241557452:

Merge remote-tracking branch 'remotes/armbru/tags/pull-error-2018-08-27-v2' into staging (2018-08-27 16:44:20 +0100)

are available in the Git repository at:

https://git.xanclic.moe/XanClic/qemu.git tags/pull-block-2018-08-31-v2

for you to fetch changes up to e21a1c9831fc80ae3f3c1affdfa43350035d8588:

jobs: remove job_defer_to_main_loop (2018-08-31 16:28:33 +0200)

----------------------------------------------------------------
Block patches:
- (Block) job exit refactoring, part 1
  (removing job_defer_to_main_loop())
- test-bdrv-drain leak fix

----------------------------------------------------------------
John Snow (9):
  jobs: change start callback to run callback
  jobs: canonize Error object
  jobs: add exit shim
  block/commit: utilize job_exit shim
  block/mirror: utilize job_exit shim
  jobs: utilize job_exit shim
  block/backup: make function variables consistently named
  jobs: remove ret argument to job_completed; privatize it
  jobs: remove job_defer_to_main_loop

Marc-André Lureau (1):
  tests: fix bdrv-drain leak

-- 
2.17.1

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Spotted by ASAN:

=================================================================
==5378==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 65536 byte(s) in 1 object(s) allocated from:
    #0 0x7f788f83bc48 in malloc (/lib64/libasan.so.5+0xeec48)
    #1 0x7f788c9923c5 in g_malloc (/lib64/libglib-2.0.so.0+0x523c5)
    #2 0x5622a1fe37bc in coroutine_trampoline /home/elmarco/src/qq/util/coroutine-ucontext.c:116
    #3 0x7f788a15d75f in __correctly_grouped_prefixwc (/lib64/libc.so.6+0x4c75f)

(Broken in commit 4c8158e359d.)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Message-id: 20180809114417.28718-3-marcandre.lureau@redhat.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/test-bdrv-drain.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
index XXXXXXX..XXXXXXX 100644
--- a/tests/test-bdrv-drain.c
+++ b/tests/test-bdrv-drain.c
@@ -XXX,XX +XXX,XX @@ static void coroutine_fn test_co_delete_by_drain(void *opaque)
     }
 
     dbdd->done = true;
+    g_free(buffer);
 }
 
 /**
-- 
2.17.1

From: John Snow <jsnow@redhat.com>

Presently we codify the entry point for a job as the "start" callback,
but a more apt name would be "run" to clarify the idea that when this
function returns we consider the job to have "finished," except for
any cleanup which occurs in separate callbacks later.

As part of this clarification, change the signature to include an error
object and a return code. The error ptr is not yet used, and the return
code while captured, will be overwritten by actions in the job_completed
function.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Message-id: 20180830015734.19765-2-jsnow@redhat.com
Reviewed-by: Jeff Cody <jcody@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 include/qemu/job.h        |  2 +-
 block/backup.c            |  7 ++++---
 block/commit.c            |  7 ++++---
 block/create.c            |  8 +++++---
 block/mirror.c            | 10 ++++++----
 block/stream.c            |  7 ++++---
 job.c                     |  6 +++---
 tests/test-bdrv-drain.c   |  7 ++++---
 tests/test-blockjob-txn.c | 16 ++++++++--------
 tests/test-blockjob.c     |  7 ++++---
 10 files changed, 43 insertions(+), 34 deletions(-)

diff --git a/include/qemu/job.h b/include/qemu/job.h
index XXXXXXX..XXXXXXX 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -XXX,XX +XXX,XX @@ struct JobDriver {
     JobType job_type;
 
     /** Mandatory: Entrypoint for the Coroutine. */
-    CoroutineEntry *start;
+    int coroutine_fn (*run)(Job *job, Error **errp);
 
     /**
      * If the callback is not NULL, it will be invoked when the job transitions
diff --git a/block/backup.c b/block/backup.c
index XXXXXXX..XXXXXXX 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -XXX,XX +XXX,XX @@ 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, Error **errp)
 {
-    BackupBlockJob *job = opaque;
+    BackupBlockJob *job = container_of(opaque_job, BackupBlockJob, common.job);
     BackupCompleteData *data;
     BlockDriverState *bs = blk_bs(job->common.blk);
     int64_t offset, nb_clusters;
@@ -XXX,XX +XXX,XX @@ static void coroutine_fn backup_run(void *opaque)
     data = g_malloc(sizeof(*data));
     data->ret = ret;
     job_defer_to_main_loop(&job->common.job, backup_complete, data);
+    return ret;
 }
 
 static const BlockJobDriver backup_job_driver = {
@@ -XXX,XX +XXX,XX @@ static const BlockJobDriver backup_job_driver = {
         .free                   = block_job_free,
         .user_resume            = block_job_user_resume,
         .drain                  = block_job_drain,
-        .start                  = backup_run,
+        .run                    = backup_run,
         .commit                 = backup_commit,
         .abort                  = backup_abort,
         .clean                  = backup_clean,
diff --git a/block/commit.c b/block/commit.c
index XXXXXXX..XXXXXXX 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -XXX,XX +XXX,XX @@ static void commit_complete(Job *job, void *opaque)
     bdrv_unref(top);
 }
 
-static void coroutine_fn commit_run(void *opaque)
+static int coroutine_fn commit_run(Job *job, Error **errp)
 {
-    CommitBlockJob *s = opaque;
+    CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
     CommitCompleteData *data;
     int64_t offset;
     uint64_t delay_ns = 0;
@@ -XXX,XX +XXX,XX @@ out:
     data = g_malloc(sizeof(*data));
     data->ret = ret;
     job_defer_to_main_loop(&s->common.job, commit_complete, data);
+    return ret;
 }
 
 static const BlockJobDriver commit_job_driver = {
@@ -XXX,XX +XXX,XX @@ static const BlockJobDriver commit_job_driver = {
         .free          = block_job_free,
         .user_resume   = block_job_user_resume,
         .drain         = block_job_drain,
-        .start         = commit_run,
+        .run           = commit_run,
     },
 };
 
diff --git a/block/create.c b/block/create.c
index XXXXXXX..XXXXXXX 100644
--- a/block/create.c
+++ b/block/create.c
@@ -XXX,XX +XXX,XX @@ static void blockdev_create_complete(Job *job, void *opaque)
     job_completed(job, s->ret, s->err);
 }
 
-static void coroutine_fn blockdev_create_run(void *opaque)
+static int coroutine_fn blockdev_create_run(Job *job, Error **errp)
 {
-    BlockdevCreateJob *s = opaque;
+    BlockdevCreateJob *s = container_of(job, BlockdevCreateJob, common);
 
     job_progress_set_remaining(&s->common, 1);
     s->ret = s->drv->bdrv_co_create(s->opts, &s->err);
@@ -XXX,XX +XXX,XX @@ static void coroutine_fn blockdev_create_run(void *opaque)
 
     qapi_free_BlockdevCreateOptions(s->opts);
     job_defer_to_main_loop(&s->common, blockdev_create_complete, NULL);
+
+    return s->ret;
 }
 
 static const JobDriver blockdev_create_job_driver = {
     .instance_size = sizeof(BlockdevCreateJob),
     .job_type      = JOB_TYPE_CREATE,
-    .start         = blockdev_create_run,
+    .run           = blockdev_create_run,
 };
 
 void qmp_blockdev_create(const char *job_id, BlockdevCreateOptions *options,
diff --git a/block/mirror.c b/block/mirror.c
index XXXXXXX..XXXXXXX 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -XXX,XX +XXX,XX @@ static int mirror_flush(MirrorBlockJob *s)
     return ret;
 }
 
-static void coroutine_fn mirror_run(void *opaque)
+static int coroutine_fn mirror_run(Job *job, Error **errp)
 {
-    MirrorBlockJob *s = opaque;
+    MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
     MirrorExitData *data;
     BlockDriverState *bs = s->mirror_top_bs->backing->bs;
     BlockDriverState *target_bs = blk_bs(s->target);
@@ -XXX,XX +XXX,XX @@ immediate_exit:
     if (need_drain) {
         bdrv_drained_begin(bs);
     }
+
     job_defer_to_main_loop(&s->common.job, mirror_exit, data);
+    return ret;
 }
 
 static void mirror_complete(Job *job, Error **errp)
@@ -XXX,XX +XXX,XX @@ static const BlockJobDriver mirror_job_driver = {
         .free                   = block_job_free,
         .user_resume            = block_job_user_resume,
         .drain                  = block_job_drain,
-        .start                  = mirror_run,
+        .run                    = mirror_run,
         .pause                  = mirror_pause,
         .complete               = mirror_complete,
     },
@@ -XXX,XX +XXX,XX @@ static const BlockJobDriver commit_active_job_driver = {
         .free                   = block_job_free,
         .user_resume            = block_job_user_resume,
         .drain                  = block_job_drain,
-        .start                  = mirror_run,
+        .run                    = mirror_run,
         .pause                  = mirror_pause,
         .complete               = mirror_complete,
     },
diff --git a/block/stream.c b/block/stream.c
index XXXXXXX..XXXXXXX 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -XXX,XX +XXX,XX @@ out:
     g_free(data);
 }
 
-static void coroutine_fn stream_run(void *opaque)
+static int coroutine_fn stream_run(Job *job, Error **errp)
 {
-    StreamBlockJob *s = opaque;
+    StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
     StreamCompleteData *data;
     BlockBackend *blk = s->common.blk;
     BlockDriverState *bs = blk_bs(blk);
@@ -XXX,XX +XXX,XX @@ out:
     data = g_malloc(sizeof(*data));
     data->ret = ret;
     job_defer_to_main_loop(&s->common.job, stream_complete, data);
+    return ret;
 }
 
 static const BlockJobDriver stream_job_driver = {
@@ -XXX,XX +XXX,XX @@ static const BlockJobDriver stream_job_driver = {
         .instance_size = sizeof(StreamBlockJob),
         .job_type      = JOB_TYPE_STREAM,
         .free          = block_job_free,
-        .start         = stream_run,
+        .run           = stream_run,
         .user_resume   = block_job_user_resume,
         .drain         = block_job_drain,
     },
diff --git a/job.c b/job.c
index XXXXXXX..XXXXXXX 100644
--- a/job.c
+++ b/job.c
@@ -XXX,XX +XXX,XX @@ static void coroutine_fn job_co_entry(void *opaque)
 {
     Job *job = opaque;
 
-    assert(job && job->driver && job->driver->start);
+    assert(job && job->driver && job->driver->run);
     job_pause_point(job);
-    job->driver->start(job);
+    job->ret = job->driver->run(job, NULL);
 }
 
 
 void job_start(Job *job)
 {
     assert(job && !job_started(job) && job->paused &&
-           job->driver && job->driver->start);
+           job->driver && job->driver->run);
     job->co = qemu_coroutine_create(job_co_entry, job);
     job->pause_count--;
     job->busy = true;
diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
index XXXXXXX..XXXXXXX 100644
--- a/tests/test-bdrv-drain.c
+++ b/tests/test-bdrv-drain.c
@@ -XXX,XX +XXX,XX @@ static void test_job_completed(Job *job, void *opaque)
     job_completed(job, 0, NULL);
 }
 
-static void coroutine_fn test_job_start(void *opaque)
+static int coroutine_fn test_job_run(Job *job, Error **errp)
 {
-    TestBlockJob *s = opaque;
+    TestBlockJob *s = container_of(job, TestBlockJob, common.job);
 
     job_transition_to_ready(&s->common.job);
     while (!s->should_complete) {
@@ -XXX,XX +XXX,XX @@ static void coroutine_fn test_job_start(void *opaque)
     }
 
     job_defer_to_main_loop(&s->common.job, test_job_completed, NULL);
+    return 0;
 }
 
 static void test_job_complete(Job *job, Error **errp)
@@ -XXX,XX +XXX,XX @@ BlockJobDriver test_job_driver = {
         .free           = block_job_free,
         .user_resume    = block_job_user_resume,
         .drain          = block_job_drain,
-        .start          = test_job_start,
+        .run            = test_job_run,
         .complete       = test_job_complete,
     },
 };
diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
index XXXXXXX..XXXXXXX 100644
--- a/tests/test-blockjob-txn.c
+++ b/tests/test-blockjob-txn.c
@@ -XXX,XX +XXX,XX @@ static void test_block_job_complete(Job *job, void *opaque)
     bdrv_unref(bs);
 }
 
-static void coroutine_fn test_block_job_run(void *opaque)
+static int coroutine_fn test_block_job_run(Job *job, Error **errp)
 {
-    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;
         }
     }
 
-    job_defer_to_main_loop(&job->job, test_block_job_complete,
+    job_defer_to_main_loop(job, test_block_job_complete,
                            (void *)(intptr_t)s->rc);
+    return s->rc;
 }
 
 typedef struct {
@@ -XXX,XX +XXX,XX @@ static const BlockJobDriver test_block_job_driver = {
         .free          = block_job_free,
         .user_resume   = block_job_user_resume,
         .drain         = block_job_drain,
-        .start         = test_block_job_run,
+        .run           = test_block_job_run,
     },
 };
 
diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
index XXXXXXX..XXXXXXX 100644
--- a/tests/test-blockjob.c
+++ b/tests/test-blockjob.c
@@ -XXX,XX +XXX,XX @@ 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_run(Job *job, Error **errp)
 {
-    CancelJob *s = opaque;
+    CancelJob *s = container_of(job, CancelJob, common.job);
 
     while (!s->should_complete) {
         if (job_is_cancelled(&s->common.job)) {
@@ -XXX,XX +XXX,XX @@ static void coroutine_fn cancel_job_start(void *opaque)
 
  defer:
     job_defer_to_main_loop(&s->common.job, cancel_job_completed, s);
+    return 0;
 }
 
 static const BlockJobDriver test_cancel_driver = {
@@ -XXX,XX +XXX,XX @@ static const BlockJobDriver test_cancel_driver = {
         .free          = block_job_free,
         .user_resume   = block_job_user_resume,
         .drain         = block_job_drain,
-        .start         = cancel_job_start,
+        .run           = cancel_job_run,
         .complete      = cancel_job_complete,
     },
 };
-- 
2.17.1

From: John Snow <jsnow@redhat.com>

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. The integer error code for job_completed is kept for now,
to be removed shortly in a separate patch.

Signed-off-by: John Snow <jsnow@redhat.com>
Message-id: 20180830015734.19765-3-jsnow@redhat.com
[mreitz: Dropped a superfluous g_strdup()]
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 include/qemu/job.h        | 14 ++++++++------
 block/backup.c            |  2 +-
 block/commit.c            |  2 +-
 block/create.c            |  5 ++---
 block/mirror.c            |  2 +-
 block/stream.c            |  2 +-
 job-qmp.c                 |  5 +++--
 job.c                     | 18 ++++++------------
 tests/test-bdrv-drain.c   |  2 +-
 tests/test-blockjob-txn.c |  2 +-
 tests/test-blockjob.c     |  2 +-
 11 files changed, 26 insertions(+), 30 deletions(-)

diff --git a/include/qemu/job.h b/include/qemu/job.h
index XXXXXXX..XXXXXXX 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -XXX,XX +XXX,XX @@ 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.
+     * If job->ret is nonzero and an error object was not set, it will be set
+     * to strerror(-job->ret) during job_completed.
+     */
+    Error *err;
+
     /** The completion function that will be called when the job completes.  */
     BlockCompletionFunc *cb;
 
@@ -XXX,XX +XXX,XX @@ 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/block/backup.c b/block/backup.c
index XXXXXXX..XXXXXXX 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644
--- a/block/create.c
+++ b/block/create.c
@@ -XXX,XX +XXX,XX @@ 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 int coroutine_fn blockdev_create_run(Job *job, Error **errp)
@@ -XXX,XX +XXX,XX @@ static int coroutine_fn blockdev_create_run(Job *job, Error **errp)
     BlockdevCreateJob *s = container_of(job, BlockdevCreateJob, common);
 
     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, errp);
     job_progress_update(&s->common, 1);
 
     qapi_free_BlockdevCreateOptions(s->opts);
diff --git a/block/mirror.c b/block/mirror.c
index XXXXXXX..XXXXXXX 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -XXX,XX +XXX,XX @@ out:
     }
 
     g_free(s->backing_file_str);
-    job_completed(job, data->ret, NULL);
+    job_completed(job, data->ret);
     g_free(data);
 }
 
diff --git a/job-qmp.c b/job-qmp.c
index XXXXXXX..XXXXXXX 100644
--- a/job-qmp.c
+++ b/job-qmp.c
@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644
--- a/job.c
+++ b/job.c
@@ -XXX,XX +XXX,XX @@ 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);
     }
@@ -XXX,XX +XXX,XX @@ static void coroutine_fn job_co_entry(void *opaque)
 
     assert(job && job->driver && job->driver->run);
     job_pause_point(job);
-    job->ret = job->driver->run(job, NULL);
+    job->ret = job->driver->run(job, &job->err);
 }
 
 
@@ -XXX,XX +XXX,XX @@ 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(&job->err, "%s", strerror(-job->ret));
         }
         job_state_transition(job, JOB_STATUS_ABORTING);
     }
@@ -XXX,XX +XXX,XX @@ 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) {
@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644
--- a/tests/test-bdrv-drain.c
+++ b/tests/test-bdrv-drain.c
@@ -XXX,XX +XXX,XX @@ typedef struct TestBlockJob {
 
 static void test_job_completed(Job *job, void *opaque)
 {
-    job_completed(job, 0, NULL);
+    job_completed(job, 0);
 }
 
 static int coroutine_fn test_job_run(Job *job, Error **errp)
diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
index XXXXXXX..XXXXXXX 100644
--- a/tests/test-blockjob-txn.c
+++ b/tests/test-blockjob-txn.c
@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644
--- a/tests/test-blockjob.c
+++ b/tests/test-blockjob.c
@@ -XXX,XX +XXX,XX @@ 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.17.1

From: John Snow <jsnow@redhat.com>

All 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

Few jobs do anything beyond exactly this. Consolidate this exit
logic for a net reduction in SLOC.

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.

You can observe this in practice by looking at mirror_exit's careful
placement of job_completed and bdrv_unref calls.

If we centralize job exiting, we can signal job completion from outside
of the aio_context, which should allow for job cleanup code to run with
only one lock, which makes cleanup callbacks less tricky to write.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Message-id: 20180830015734.19765-4-jsnow@redhat.com
Reviewed-by: Jeff Cody <jcody@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 include/qemu/job.h | 11 +++++++++++
 job.c              | 18 ++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/include/qemu/job.h b/include/qemu/job.h
index XXXXXXX..XXXXXXX 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -XXX,XX +XXX,XX @@ 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.
+     *
+     * FIXME TODO: This callback is only temporary to transition remaining jobs
+     * to prepare/commit/abort/clean callbacks and will be removed before 3.1.
+     * is released.
+     */
+    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 XXXXXXX..XXXXXXX 100644
--- a/job.c
+++ b/job.c
@@ -XXX,XX +XXX,XX @@ 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
@@ -XXX,XX +XXX,XX @@ static void coroutine_fn job_co_entry(void *opaque)
     assert(job && job->driver && job->driver->run);
     job_pause_point(job);
     job->ret = job->driver->run(job, &job->err);
+    if (!job->deferred_to_main_loop) {
+        job->deferred_to_main_loop = true;
+        aio_bh_schedule_oneshot(qemu_get_aio_context(),
+                                job_exit,
+                                job);
+    }
 }
 
 
-- 
2.17.1

From: John Snow <jsnow@redhat.com>

Change the manual deferment to commit_complete into the implicit
callback to job_exit, renaming commit_complete to commit_exit.

This conversion does change the timing of when job_completed is
called to after the bdrv_replace_node and bdrv_unref calls, which
could have implications for bjob->blk which will now be put down
after this cleanup.

Kevin highlights that we did not take any permissions for that backend
at job creation time, so it is safe to reorder these operations.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Message-id: 20180830015734.19765-5-jsnow@redhat.com
Reviewed-by: Jeff Cody <jcody@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/commit.c | 22 +++++-----------------
 1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index XXXXXXX..XXXXXXX 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -XXX,XX +XXX,XX @@ 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;
     bool remove_commit_top_bs = false;
 
     /* Make sure commit_top_bs and top stay around until bdrv_replace_node() */
@@ -XXX,XX +XXX,XX @@ static void commit_complete(Job *job, void *opaque)
      * the normal backing chain can be restored. */
     blk_unref(s->base);
 
-    if (!job_is_cancelled(job) && ret == 0) {
+    if (!job_is_cancelled(job) && job->ret == 0) {
         /* success */
-        ret = bdrv_drop_intermediate(s->commit_top_bs, base,
-                                     s->backing_file_str);
+        job->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
@@ -XXX,XX +XXX,XX @@ 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.  */
@@ -XXX,XX +XXX,XX @@ static void commit_complete(Job *job, void *opaque)
 static int coroutine_fn commit_run(Job *job, Error **errp)
 {
     CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
-    CommitCompleteData *data;
     int64_t offset;
     uint64_t delay_ns = 0;
     int ret = 0;
@@ -XXX,XX +XXX,XX @@ static int coroutine_fn commit_run(Job *job, Error **errp)
 out:
     qemu_vfree(buf);
 
-    data = g_malloc(sizeof(*data));
-    data->ret = ret;
-    job_defer_to_main_loop(&s->common.job, commit_complete, data);
     return ret;
 }
 
@@ -XXX,XX +XXX,XX @@ static const BlockJobDriver commit_job_driver = {
         .user_resume   = block_job_user_resume,
         .drain         = block_job_drain,
         .run           = commit_run,
+        .exit          = commit_exit,
     },
 };
 
-- 
2.17.1

From: John Snow <jsnow@redhat.com>

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

This does change the order of some bdrv_unref calls and job_completed,
but thanks to the new context in which we call .exit, this is safe to
defer the possible flushing of any nodes to the job_finalize_single
cleanup stage.

Signed-off-by: John Snow <jsnow@redhat.com>
Message-id: 20180830015734.19765-6-jsnow@redhat.com
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/mirror.c | 29 +++++++++++------------------
 1 file changed, 11 insertions(+), 18 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index XXXXXXX..XXXXXXX 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -XXX,XX +XXX,XX @@ 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);
 
-    /* Make sure that the source BDS doesn't go away before we called
-     * job_completed(). */
+    /* Make sure that the source BDS doesn't go away during bdrv_replace_node,
+     * before we can call bdrv_drained_end */
     bdrv_ref(src);
     bdrv_ref(mirror_top_bs);
     bdrv_ref(target_bs);
@@ -XXX,XX +XXX,XX @@ 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;
             }
         }
     }
@@ -XXX,XX +XXX,XX @@ 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;
@@ -XXX,XX +XXX,XX @@ 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) {
@@ -XXX,XX +XXX,XX @@ 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)
@@ -XXX,XX +XXX,XX @@ static int mirror_flush(MirrorBlockJob *s)
 static int coroutine_fn mirror_run(Job *job, Error **errp)
 {
     MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
-    MirrorExitData *data;
     BlockDriverState *bs = s->mirror_top_bs->backing->bs;
     BlockDriverState *target_bs = blk_bs(s->target);
     bool need_drain = true;
@@ -XXX,XX +XXX,XX @@ 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);
     return ret;
 }
 
@@ -XXX,XX +XXX,XX @@ static const BlockJobDriver mirror_job_driver = {
         .user_resume            = block_job_user_resume,
         .drain                  = block_job_drain,
         .run                    = mirror_run,
+        .exit                   = mirror_exit,
         .pause                  = mirror_pause,
         .complete               = mirror_complete,
     },
@@ -XXX,XX +XXX,XX @@ static const BlockJobDriver commit_active_job_driver = {
         .user_resume            = block_job_user_resume,
         .drain                  = block_job_drain,
         .run                    = mirror_run,
+        .exit                   = mirror_exit,
         .pause                  = mirror_pause,
         .complete               = mirror_complete,
     },
-- 
2.17.1

From: John Snow <jsnow@redhat.com>

Utilize the job_exit shim by not calling job_defer_to_main_loop, and
where applicable, converting the deferred callback into the job_exit
callback.

This converts backup, stream, create, and the unit tests all at once.
Most of these jobs do not see any changes to the order in which they
clean up their resources, except the test-blockjob-txn test, which
now puts down its bs before job_completed is called.

This is safe for the same reason the reordering in the mirror job is
safe, because job_completed no longer runs under two locks, making
the unref safe even if it causes a flush.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Message-id: 20180830015734.19765-7-jsnow@redhat.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/backup.c            | 16 ----------------
 block/create.c            | 14 +++-----------
 block/stream.c            | 22 +++++++---------------
 tests/test-bdrv-drain.c   |  6 ------
 tests/test-blockjob-txn.c | 11 ++---------
 tests/test-blockjob.c     | 10 ++++------
 6 files changed, 16 insertions(+), 63 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index XXXXXXX..XXXXXXX 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -XXX,XX +XXX,XX @@ 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;
@@ -XXX,XX +XXX,XX @@ static void backup_incremental_init_copy_bitmap(BackupBlockJob *job)
 static int coroutine_fn backup_run(Job *opaque_job, Error **errp)
 {
     BackupBlockJob *job = container_of(opaque_job, BackupBlockJob, common.job);
-    BackupCompleteData *data;
     BlockDriverState *bs = blk_bs(job->common.blk);
     int64_t offset, nb_clusters;
     int ret = 0;
@@ -XXX,XX +XXX,XX @@ static int coroutine_fn backup_run(Job *opaque_job, Error **errp)
     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);
     return ret;
 }
 
diff --git a/block/create.c b/block/create.c
index XXXXXXX..XXXXXXX 100644
--- a/block/create.c
+++ b/block/create.c
@@ -XXX,XX +XXX,XX @@ 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 int coroutine_fn blockdev_create_run(Job *job, Error **errp)
 {
     BlockdevCreateJob *s = container_of(job, BlockdevCreateJob, common);
+    int ret;
 
     job_progress_set_remaining(&s->common, 1);
-    s->ret = s->drv->bdrv_co_create(s->opts, errp);
+    ret = s->drv->bdrv_co_create(s->opts, errp);
     job_progress_update(&s->common, 1);
 
     qapi_free_BlockdevCreateOptions(s->opts);
-    job_defer_to_main_loop(&s->common, blockdev_create_complete, NULL);
 
-    return s->ret;
+    return ret;
 }
 
 static const JobDriver blockdev_create_job_driver = {
diff --git a/block/stream.c b/block/stream.c
index XXXXXXX..XXXXXXX 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -XXX,XX +XXX,XX @@ 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;
@@ -XXX,XX +XXX,XX @@ 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;
         }
     }
@@ -XXX,XX +XXX,XX @@ out:
     }
 
     g_free(s->backing_file_str);
-    job_completed(job, data->ret);
-    g_free(data);
+    job->ret = ret;
 }
 
 static int coroutine_fn stream_run(Job *job, Error **errp)
 {
     StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
-    StreamCompleteData *data;
     BlockBackend *blk = s->common.blk;
     BlockDriverState *bs = blk_bs(blk);
     BlockDriverState *base = s->base;
@@ -XXX,XX +XXX,XX @@ static int coroutine_fn stream_run(Job *job, Error **errp)
 
 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);
     return ret;
 }
 
@@ -XXX,XX +XXX,XX @@ static const BlockJobDriver stream_job_driver = {
         .job_type      = JOB_TYPE_STREAM,
         .free          = block_job_free,
         .run           = stream_run,
+        .exit          = stream_exit,
         .user_resume   = block_job_user_resume,
         .drain         = block_job_drain,
     },
diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
index XXXXXXX..XXXXXXX 100644
--- a/tests/test-bdrv-drain.c
+++ b/tests/test-bdrv-drain.c
@@ -XXX,XX +XXX,XX @@ typedef struct TestBlockJob {
     bool should_complete;
 } TestBlockJob;
 
-static void test_job_completed(Job *job, void *opaque)
-{
-    job_completed(job, 0);
-}
-
 static int coroutine_fn test_job_run(Job *job, Error **errp)
 {
     TestBlockJob *s = container_of(job, TestBlockJob, common.job);
@@ -XXX,XX +XXX,XX @@ static int coroutine_fn test_job_run(Job *job, Error **errp)
         job_pause_point(&s->common.job);
     }
 
-    job_defer_to_main_loop(&s->common.job, test_job_completed, NULL);
     return 0;
 }
 
diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
index XXXXXXX..XXXXXXX 100644
--- a/tests/test-blockjob-txn.c
+++ b/tests/test-blockjob-txn.c
@@ -XXX,XX +XXX,XX @@ 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);
 }
 
@@ -XXX,XX +XXX,XX @@ static int coroutine_fn test_block_job_run(Job *job, Error **errp)
         }
     }
 
-    job_defer_to_main_loop(job, test_block_job_complete,
-                           (void *)(intptr_t)s->rc);
     return s->rc;
 }
 
@@ -XXX,XX +XXX,XX @@ static const BlockJobDriver test_block_job_driver = {
         .user_resume   = block_job_user_resume,
         .drain         = block_job_drain,
         .run           = test_block_job_run,
+        .exit          = test_block_job_exit,
     },
 };
 
diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
index XXXXXXX..XXXXXXX 100644
--- a/tests/test-blockjob.c
+++ b/tests/test-blockjob.c
@@ -XXX,XX +XXX,XX @@ 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)
@@ -XXX,XX +XXX,XX @@ static int coroutine_fn cancel_job_run(Job *job, Error **errp)
 
     while (!s->should_complete) {
         if (job_is_cancelled(&s->common.job)) {
-            goto defer;
+            return 0;
         }
 
         if (!job_is_ready(&s->common.job) && s->should_converge) {
@@ -XXX,XX +XXX,XX @@ static int coroutine_fn cancel_job_run(Job *job, Error **errp)
         job_sleep_ns(&s->common.job, 100000);
     }
 
- defer:
-    job_defer_to_main_loop(&s->common.job, cancel_job_completed, s);
     return 0;
 }
 
@@ -XXX,XX +XXX,XX @@ static const BlockJobDriver test_cancel_driver = {
         .user_resume   = block_job_user_resume,
         .drain         = block_job_drain,
         .run           = cancel_job_run,
+        .exit          = cancel_job_exit,
         .complete      = cancel_job_complete,
     },
 };
-- 
2.17.1

From: John Snow <jsnow@redhat.com>

Rename opaque_job to job to be consistent with other job implementations.
Rename 'job', the BackupBlockJob object, to 's' to also be consistent.

Suggested-by: Eric Blake <eblake@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Message-id: 20180830015734.19765-8-jsnow@redhat.com
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/backup.c | 62 +++++++++++++++++++++++++-------------------------
 1 file changed, 31 insertions(+), 31 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index XXXXXXX..XXXXXXX 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -XXX,XX +XXX,XX @@ static void backup_incremental_init_copy_bitmap(BackupBlockJob *job)
     bdrv_dirty_iter_free(dbi);
 }
 
-static int coroutine_fn backup_run(Job *opaque_job, Error **errp)
+static int coroutine_fn backup_run(Job *job, Error **errp)
 {
-    BackupBlockJob *job = container_of(opaque_job, BackupBlockJob, common.job);
-    BlockDriverState *bs = blk_bs(job->common.blk);
+    BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
+    BlockDriverState *bs = blk_bs(s->common.blk);
     int64_t offset, nb_clusters;
     int ret = 0;
 
-    QLIST_INIT(&job->inflight_reqs);
-    qemu_co_rwlock_init(&job->flush_rwlock);
+    QLIST_INIT(&s->inflight_reqs);
+    qemu_co_rwlock_init(&s->flush_rwlock);
 
-    nb_clusters = DIV_ROUND_UP(job->len, job->cluster_size);
-    job_progress_set_remaining(&job->common.job, job->len);
+    nb_clusters = DIV_ROUND_UP(s->len, s->cluster_size);
+    job_progress_set_remaining(job, s->len);
 
-    job->copy_bitmap = hbitmap_alloc(nb_clusters, 0);
-    if (job->sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) {
-        backup_incremental_init_copy_bitmap(job);
+    s->copy_bitmap = hbitmap_alloc(nb_clusters, 0);
+    if (s->sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) {
+        backup_incremental_init_copy_bitmap(s);
     } else {
-        hbitmap_set(job->copy_bitmap, 0, nb_clusters);
+        hbitmap_set(s->copy_bitmap, 0, nb_clusters);
     }
 
 
-    job->before_write.notify = backup_before_write_notify;
-    bdrv_add_before_write_notifier(bs, &job->before_write);
+    s->before_write.notify = backup_before_write_notify;
+    bdrv_add_before_write_notifier(bs, &s->before_write);
 
-    if (job->sync_mode == MIRROR_SYNC_MODE_NONE) {
+    if (s->sync_mode == MIRROR_SYNC_MODE_NONE) {
         /* All bits are set in copy_bitmap to allow any cluster to be copied.
          * This does not actually require them to be copied. */
-        while (!job_is_cancelled(&job->common.job)) {
+        while (!job_is_cancelled(job)) {
             /* Yield until the job is cancelled.  We just let our before_write
              * notify callback service CoW requests. */
-            job_yield(&job->common.job);
+            job_yield(job);
         }
-    } else if (job->sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) {
-        ret = backup_run_incremental(job);
+    } else if (s->sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) {
+        ret = backup_run_incremental(s);
     } else {
         /* Both FULL and TOP SYNC_MODE's require copying.. */
-        for (offset = 0; offset < job->len;
-             offset += job->cluster_size) {
+        for (offset = 0; offset < s->len;
+             offset += s->cluster_size) {
             bool error_is_read;
             int alloced = 0;
 
-            if (yield_and_check(job)) {
+            if (yield_and_check(s)) {
                 break;
             }
 
-            if (job->sync_mode == MIRROR_SYNC_MODE_TOP) {
+            if (s->sync_mode == MIRROR_SYNC_MODE_TOP) {
                 int i;
                 int64_t n;
 
                 /* Check to see if these blocks are already in the
                  * backing file. */
 
-                for (i = 0; i < job->cluster_size;) {
+                for (i = 0; i < s->cluster_size;) {
                     /* bdrv_is_allocated() only returns true/false based
                      * on the first set of sectors it comes across that
                      * are are all in the same state.
@@ -XXX,XX +XXX,XX @@ static int coroutine_fn backup_run(Job *opaque_job, Error **errp)
                      * needed but at some point that is always the case. */
                     alloced =
                         bdrv_is_allocated(bs, offset + i,
-                                          job->cluster_size - i, &n);
+                                          s->cluster_size - i, &n);
                     i += n;
 
                     if (alloced || n == 0) {
@@ -XXX,XX +XXX,XX @@ static int coroutine_fn backup_run(Job *opaque_job, Error **errp)
             if (alloced < 0) {
                 ret = alloced;
             } else {
-                ret = backup_do_cow(job, offset, job->cluster_size,
+                ret = backup_do_cow(s, offset, s->cluster_size,
                                     &error_is_read, false);
             }
             if (ret < 0) {
                 /* Depending on error action, fail now or retry cluster */
                 BlockErrorAction action =
-                    backup_error_action(job, error_is_read, -ret);
+                    backup_error_action(s, error_is_read, -ret);
                 if (action == BLOCK_ERROR_ACTION_REPORT) {
                     break;
                 } else {
-                    offset -= job->cluster_size;
+                    offset -= s->cluster_size;
                     continue;
                 }
             }
         }
     }
 
-    notifier_with_return_remove(&job->before_write);
+    notifier_with_return_remove(&s->before_write);
 
     /* wait until pending backup_do_cow() calls have completed */
-    qemu_co_rwlock_wrlock(&job->flush_rwlock);
-    qemu_co_rwlock_unlock(&job->flush_rwlock);
-    hbitmap_free(job->copy_bitmap);
+    qemu_co_rwlock_wrlock(&s->flush_rwlock);
+    qemu_co_rwlock_unlock(&s->flush_rwlock);
+    hbitmap_free(s->copy_bitmap);
 
     return ret;
 }
-- 
2.17.1

From: John Snow <jsnow@redhat.com>

Jobs are now expected to return their retcode on the stack, from the
.run callback, so we can remove that argument.

job_cancel does not need to set -ECANCELED because job_completed will
update the return code itself if the job was canceled.

While we're here, make job_completed static to job.c and remove it from
job.h; move the documentation of return code to the .run() callback and
to the job->ret property, accordingly.

Signed-off-by: John Snow <jsnow@redhat.com>
Message-id: 20180830015734.19765-9-jsnow@redhat.com
Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 include/qemu/job.h | 28 +++++++++++++++-------------
 job.c              | 11 ++++++-----
 trace-events       |  2 +-
 3 files changed, 22 insertions(+), 19 deletions(-)

From: John Snow <jsnow@redhat.com>

Now that the job infrastructure is handling the job_completed call for
all implemented jobs, we can remove the interface that allowed jobs to
schedule their own completion.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Message-id: 20180830015734.19765-10-jsnow@redhat.com
Signed-off-by: Max Reitz <mreitz@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 XXXXXXX..XXXXXXX 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644
--- a/job.c
+++ b/job.c
@@ -XXX,XX +XXX,XX @@ static void coroutine_fn job_co_entry(void *opaque)
     assert(job && job->driver && job->driver->run);
     job_pause_point(job);
     job->ret = job->driver->run(job, &job->err);
-    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);
 }
 
 
@@ -XXX,XX +XXX,XX @@ 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.17.1