[PATCH 1/3] job: introduce dump guest memory job

[PATCH 1/3] job: introduce dump guest memory job

[Hogan Wang via]

There's no way to cancel the current executing dump process, lead to the
virtual machine manager daemon((e.g. libvirtd) cannot restore the dump
job after daemon restart.

Introduce dump guest memory job type, and add an optional 'job-id'
argument for dump-guest-memory QMP to make use of jobs framework.

Signed-off-by: Hogan Wang <hogan.wang@huawei.com>
---
 dump/dump-hmp-cmds.c | 12 ++++++++++--
 dump/dump.c          |  1 +
 qapi/dump.json       |  6 +++++-
 qapi/job.json        |  5 ++++-
 4 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/dump/dump-hmp-cmds.c b/dump/dump-hmp-cmds.c
index e5053b04cd..ba28a5e631 100644
--- a/dump/dump-hmp-cmds.c
+++ b/dump/dump-hmp-cmds.c
@@ -24,9 +24,11 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
     bool has_begin = qdict_haskey(qdict, "begin");
     bool has_length = qdict_haskey(qdict, "length");
     bool has_detach = qdict_haskey(qdict, "detach");
+    bool has_job_id = qdict_haskey(qdict, "job-id");
     int64_t begin = 0;
     int64_t length = 0;
     bool detach = false;
+    const char *job_id = NULL;
     enum DumpGuestMemoryFormat dump_format = DUMP_GUEST_MEMORY_FORMAT_ELF;
     char *prot;
 
@@ -62,10 +64,16 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
         detach = qdict_get_bool(qdict, "detach");
     }
 
+    if (has_job_id) {
+        job_id = qdict_get_str(qdict, "job-id");
+    }
+
     prot = g_strconcat("file:", file, NULL);
 
-    qmp_dump_guest_memory(paging, prot, true, detach, has_begin, begin,
-                          has_length, length, true, dump_format, &err);
+    qmp_dump_guest_memory(paging, prot, has_job_id, job_id,
+                          true, detach, has_begin, begin,
+                          has_length, length, true, dump_format,
+                          &err);
     hmp_handle_error(mon, err);
     g_free(prot);
 }
diff --git a/dump/dump.c b/dump/dump.c
index 4d9658ffa2..6aa946b9ac 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -1872,6 +1872,7 @@ DumpQueryResult *qmp_query_dump(Error **errp)
 }
 
 void qmp_dump_guest_memory(bool paging, const char *file,
+                           bool has_job_id, const char *job_id,
                            bool has_detach, bool detach,
                            bool has_begin, int64_t begin, bool has_length,
                            int64_t length, bool has_format,
diff --git a/qapi/dump.json b/qapi/dump.json
index 90859c5483..5209d0b74f 100644
--- a/qapi/dump.json
+++ b/qapi/dump.json
@@ -59,6 +59,9 @@
 #            2. fd: the protocol starts with "fd:", and the following string
 #               is the fd's name.
 #
+# @job-id: identifier for the newly-created memory dump job. If
+#          omitted, use 'memory-guest-dump' by default. (Since 7.2)
+#
 # @detach: if true, QMP will return immediately rather than
 #          waiting for the dump to finish. The user can track progress
 #          using "query-dump". (since 2.6).
@@ -88,7 +91,8 @@
 #
 ##
 { 'command': 'dump-guest-memory',
-  'data': { 'paging': 'bool', 'protocol': 'str', '*detach': 'bool',
+  'data': { 'paging': 'bool', 'protocol': 'str',
+            '*job-id': 'str', '*detach': 'bool',
             '*begin': 'int', '*length': 'int',
             '*format': 'DumpGuestMemoryFormat'} }
 
diff --git a/qapi/job.json b/qapi/job.json
index d5f84e9615..e14d2290a5 100644
--- a/qapi/job.json
+++ b/qapi/job.json
@@ -28,11 +28,14 @@
 #
 # @snapshot-delete: snapshot delete job type, see "snapshot-delete" (since 6.0)
 #
+# @dump-guest-memory: dump guest memory job type, see "dump-guest-memory" (since 7.2)
+#
 # Since: 1.7
 ##
 { 'enum': 'JobType',
   'data': ['commit', 'stream', 'mirror', 'backup', 'create', 'amend',
-           'snapshot-load', 'snapshot-save', 'snapshot-delete'] }
+           'snapshot-load', 'snapshot-save', 'snapshot-delete',
+           'dump-guest-memory'] }
 
 ##
 # @JobStatus:
-- 
2.33.0

[PATCH 2/3] dump: use jobs framework for dump guest memory

[Hogan Wang via]

There's no way to cancel the current executing dump process, lead to the
virtual machine manager daemon((e.g. libvirtd) cannot restore the dump
job after daemon restart.

Use default job id 'dump-guest-memory' to create a job object for dump
process.

Signed-off-by: Hogan Wang <hogan.wang@huawei.com>
---
 dump/dump.c | 68 +++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 58 insertions(+), 10 deletions(-)

diff --git a/dump/dump.c b/dump/dump.c
index 6aa946b9ac..51dc933c7c 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -1528,6 +1528,14 @@ static void get_max_mapnr(DumpState *s)
 
 static DumpState dump_state_global = { .status = DUMP_STATUS_NONE };
 
+typedef struct DumpJob {
+    Job common;
+    DumpState *state;
+    Coroutine *co;
+    Error **errp;
+} DumpJob;
+
+
 static void dump_state_prepare(DumpState *s)
 {
     /* zero the struct, setting status to active */
@@ -1854,8 +1862,11 @@ static void dump_process(DumpState *s, Error **errp)
 
 static void *dump_thread(void *data)
 {
-    DumpState *s = (DumpState *)data;
-    dump_process(s, NULL);
+    DumpJob *job = (DumpJob *)data;
+    job_progress_set_remaining(&job->common, 1);
+    dump_process(job->state, job->errp);
+    job_progress_update(&job->common, 1);
+    aio_co_wake(job->co);
     return NULL;
 }
 
@@ -1871,6 +1882,37 @@ DumpQueryResult *qmp_query_dump(Error **errp)
     return result;
 }
 
+static void dump_sync_job_bh(void *opaque)
+{
+    dump_thread(opaque);
+}
+
+static int coroutine_fn dump_guest_memory_job_run(Job *job, Error **errp)
+{
+    DumpJob *s = container_of(job, DumpJob, common);
+    DumpState *state = &dump_state_global;
+
+    s->errp = errp;
+    s->co = qemu_coroutine_self();
+
+    if (state->detached) {
+        /* detached dump */
+        qemu_thread_create(&s->state->dump_thread, "dump_thread", dump_thread,
+                           s, QEMU_THREAD_DETACHED);
+    } else {
+        aio_bh_schedule_oneshot(qemu_get_aio_context(),
+                                dump_sync_job_bh, job);
+    }
+    qemu_coroutine_yield();
+    return qatomic_read(&state->status) == DUMP_STATUS_COMPLETED ? 0 : -1;
+}
+
+static const JobDriver dump_guest_memory_job_driver = {
+    .instance_size = sizeof(DumpJob),
+    .job_type      = JOB_TYPE_DUMP_GUEST_MEMORY,
+    .run           = dump_guest_memory_job_run,
+};
+
 void qmp_dump_guest_memory(bool paging, const char *file,
                            bool has_job_id, const char *job_id,
                            bool has_detach, bool detach,
@@ -1882,6 +1924,7 @@ void qmp_dump_guest_memory(bool paging, const char *file,
     const char *p;
     int fd = -1;
     DumpState *s;
+    DumpJob *job;
     bool detach_p = false;
 
     if (runstate_check(RUN_STATE_INMIGRATE)) {
@@ -1977,6 +2020,10 @@ void qmp_dump_guest_memory(bool paging, const char *file,
         return;
     }
 
+    if (!has_job_id) {
+        job_id = "dump-guest-memory";
+    }
+
     s = &dump_state_global;
     dump_state_prepare(s);
 
@@ -1987,15 +2034,16 @@ void qmp_dump_guest_memory(bool paging, const char *file,
         return;
     }
 
-    if (detach_p) {
-        /* detached dump */
-        s->detached = true;
-        qemu_thread_create(&s->dump_thread, "dump_thread", dump_thread,
-                           s, QEMU_THREAD_DETACHED);
-    } else {
-        /* sync dump */
-        dump_process(s, errp);
+    s->detached = detach_p;
+    job = job_create(job_id, &dump_guest_memory_job_driver, NULL,
+                    qemu_get_aio_context(), JOB_DEFAULT,
+                    NULL, NULL, errp);
+    if (!job) {
+        qatomic_set(&s->status, DUMP_STATUS_FAILED);
+        return;
     }
+    job->state = s;
+    job_start(&job->common);
 }
 
 DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error **errp)
-- 
2.33.0

Re: [PATCH 2/3] dump: use jobs framework for dump guest memory

[Marc-André Lureau]

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

Hi

On Wed, Jul 27, 2022 at 6:04 PM Hogan Wang via <qemu-devel@nongnu.org>
wrote:

> There's no way to cancel the current executing dump process, lead to the
> virtual machine manager daemon((e.g. libvirtd) cannot restore the dump
> job after daemon restart.
>
> Use default job id 'dump-guest-memory' to create a job object for dump
> process.
>
> Signed-off-by: Hogan Wang <hogan.wang@huawei.com>
> ---
>  dump/dump.c | 68 +++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 58 insertions(+), 10 deletions(-)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index 6aa946b9ac..51dc933c7c 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -1528,6 +1528,14 @@ static void get_max_mapnr(DumpState *s)
>
>  static DumpState dump_state_global = { .status = DUMP_STATUS_NONE };
>
> +typedef struct DumpJob {
> +    Job common;
> +    DumpState *state;
> +    Coroutine *co;
> +    Error **errp;
> +} DumpJob;
> +
> +
>  static void dump_state_prepare(DumpState *s)
>  {
>      /* zero the struct, setting status to active */
> @@ -1854,8 +1862,11 @@ static void dump_process(DumpState *s, Error **errp)
>
>  static void *dump_thread(void *data)
>  {
> -    DumpState *s = (DumpState *)data;
> -    dump_process(s, NULL);
> +    DumpJob *job = (DumpJob *)data;
> +    job_progress_set_remaining(&job->common, 1);
> +    dump_process(job->state, job->errp);
> +    job_progress_update(&job->common, 1);
> +    aio_co_wake(job->co);
>      return NULL;
>  }
>
> @@ -1871,6 +1882,37 @@ DumpQueryResult *qmp_query_dump(Error **errp)
>      return result;
>  }
>
> +static void dump_sync_job_bh(void *opaque)
> +{
> +    dump_thread(opaque);
> +}
> +
> +static int coroutine_fn dump_guest_memory_job_run(Job *job, Error **errp)
> +{
> +    DumpJob *s = container_of(job, DumpJob, common);
> +    DumpState *state = &dump_state_global;
> +
> +    s->errp = errp;
> +    s->co = qemu_coroutine_self();
> +
> +    if (state->detached) {
> +        /* detached dump */
> +        qemu_thread_create(&s->state->dump_thread, "dump_thread",
> dump_thread,
> +                           s, QEMU_THREAD_DETACHED);
> +    } else {
> +        aio_bh_schedule_oneshot(qemu_get_aio_context(),
> +                                dump_sync_job_bh, job);
> +    }
> +    qemu_coroutine_yield();
> +    return qatomic_read(&state->status) == DUMP_STATUS_COMPLETED ? 0 : -1;
> +}
> +
> +static const JobDriver dump_guest_memory_job_driver = {
> +    .instance_size = sizeof(DumpJob),
> +    .job_type      = JOB_TYPE_DUMP_GUEST_MEMORY,
> +    .run           = dump_guest_memory_job_run,
> +};
> +
>  void qmp_dump_guest_memory(bool paging, const char *file,
>                             bool has_job_id, const char *job_id,
>                             bool has_detach, bool detach,
> @@ -1882,6 +1924,7 @@ void qmp_dump_guest_memory(bool paging, const char
> *file,
>      const char *p;
>      int fd = -1;
>      DumpState *s;
> +    DumpJob *job;
>      bool detach_p = false;
>
>      if (runstate_check(RUN_STATE_INMIGRATE)) {
> @@ -1977,6 +2020,10 @@ void qmp_dump_guest_memory(bool paging, const char
> *file,
>          return;
>      }
>
> +    if (!has_job_id) {
> +        job_id = "dump-guest-memory";
> +    }
> +
>      s = &dump_state_global;
>      dump_state_prepare(s);
>
> @@ -1987,15 +2034,16 @@ void qmp_dump_guest_memory(bool paging, const char
> *file,
>          return;
>      }
>
> -    if (detach_p) {
> -        /* detached dump */
> -        s->detached = true;
> -        qemu_thread_create(&s->dump_thread, "dump_thread", dump_thread,
> -                           s, QEMU_THREAD_DETACHED);
> -    } else {
> -        /* sync dump */
> -        dump_process(s, errp);
> +    s->detached = detach_p;
> +    job = job_create(job_id, &dump_guest_memory_job_driver, NULL,
> +                    qemu_get_aio_context(), JOB_DEFAULT,
> +                    NULL, NULL, errp);
> +    if (!job) {
> +        qatomic_set(&s->status, DUMP_STATUS_FAILED);
> +        return;
>      }
> +    job->state = s;
> +    job_start(&job->common);
>

This is a change of behaviour, the command is no longer "sync" after this
change, as it returns immediately (even though further IO will be blocked
until the dump is completed). Maybe only create a job for commands with a
job-id and detached?

 }
>
>  DumpGuestMemoryCapability *qmp_query_dump_guest_memory_capability(Error
> **errp)
> --
> 2.33.0
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 5864 bytes --]

[PATCH 3/3] dump: support cancel dump process

[Hogan Wang via]

Break saving pages or dump iterate when dump job in cancel state,
make sure dump process exits as soon as possible.

Signed-off-by: Hogan Wang <hogan.wang@huawei.com>
---
 dump/dump.c           | 24 ++++++++++++++++++++++++
 include/sysemu/dump.h |  2 ++
 2 files changed, 26 insertions(+)

diff --git a/dump/dump.c b/dump/dump.c
index 51dc933c7c..881895e831 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -54,6 +54,8 @@ static Error *dump_migration_blocker;
       DIV_ROUND_UP((name_size), 4) +                    \
       DIV_ROUND_UP((desc_size), 4)) * 4)
 
+static bool dump_cancelling(void);
+
 static inline bool dump_is_64bit(DumpState *s)
 {
     return s->dump_info.d_class == ELFCLASS64;
@@ -118,6 +120,10 @@ static int fd_write_vmcore(const void *buf, size_t size, void *opaque)
     DumpState *s = opaque;
     size_t written_size;
 
+    if (dump_cancelling()) {
+        return -ECANCELED;
+    }
+
     written_size = qemu_write_full(s->fd, buf, size);
     if (written_size != size) {
         return -errno;
@@ -627,6 +633,10 @@ static void dump_iterate(DumpState *s, Error **errp)
 
     do {
         block = s->next_block;
+        if (dump_cancelling()) {
+            error_setg(errp, "dump: job cancelled");
+            return;
+        }
 
         size = block->target_end - block->target_start;
         if (s->has_filter) {
@@ -1321,6 +1331,10 @@ static void write_dump_pages(DumpState *s, Error **errp)
      * first page of page section
      */
     while (get_next_page(&block_iter, &pfn_iter, &buf, s)) {
+        if (dump_cancelling()) {
+            error_setg(errp, "dump: job cancelled");
+            goto out;
+        }
         /* check zero page */
         if (buffer_is_zero(buf, s->dump_info.page_size)) {
             ret = write_cache(&page_desc, &pd_zero, sizeof(PageDescriptor),
@@ -1548,6 +1562,15 @@ bool qemu_system_dump_in_progress(void)
     return (qatomic_read(&state->status) == DUMP_STATUS_ACTIVE);
 }
 
+static bool dump_cancelling(void)
+{
+    DumpState *state = &dump_state_global;
+    if (state->job && job_is_cancelled(state->job)) {
+        return true;
+    }
+    return false;
+}
+
 /* calculate total size of memory to be dumped (taking filter into
  * acoount.) */
 static int64_t dump_calculate_size(DumpState *s)
@@ -1894,6 +1917,7 @@ static int coroutine_fn dump_guest_memory_job_run(Job *job, Error **errp)
 
     s->errp = errp;
     s->co = qemu_coroutine_self();
+    state->job = job;
 
     if (state->detached) {
         /* detached dump */
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index ffc2ea1072..41bdbe595f 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -15,6 +15,7 @@
 #define DUMP_H
 
 #include "qapi/qapi-types-dump.h"
+#include "qemu/job.h"
 
 #define MAKEDUMPFILE_SIGNATURE      "makedumpfile"
 #define MAX_SIZE_MDF_HEADER         (4096) /* max size of makedumpfile_header */
@@ -154,6 +155,7 @@ typedef struct DumpState {
     GuestPhysBlockList guest_phys_blocks;
     ArchDumpInfo dump_info;
     MemoryMappingList list;
+    Job *job;
     uint32_t phdr_num;
     uint32_t shdr_num;
     bool resume;
-- 
2.33.0

Re: [PATCH 3/3] dump: support cancel dump process

[Marc-André Lureau]

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

Hi

On Wed, Jul 27, 2022 at 6:02 PM Hogan Wang via <qemu-devel@nongnu.org>
wrote:

> Break saving pages or dump iterate when dump job in cancel state,
> make sure dump process exits as soon as possible.
>
> Signed-off-by: Hogan Wang <hogan.wang@huawei.com>
>

Overall, the series looks good to me. Please send it with a top cover
letter, so it can be processed by patchew too.

I am not familiar with the job infrastructure, it would be nice if Kevin
could check the usage or give some advice.

thanks

---
>  dump/dump.c           | 24 ++++++++++++++++++++++++
>  include/sysemu/dump.h |  2 ++
>  2 files changed, 26 insertions(+)
>
> diff --git a/dump/dump.c b/dump/dump.c
> index 51dc933c7c..881895e831 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -54,6 +54,8 @@ static Error *dump_migration_blocker;
>        DIV_ROUND_UP((name_size), 4) +                    \
>        DIV_ROUND_UP((desc_size), 4)) * 4)
>
> +static bool dump_cancelling(void);
> +
>  static inline bool dump_is_64bit(DumpState *s)
>  {
>      return s->dump_info.d_class == ELFCLASS64;
> @@ -118,6 +120,10 @@ static int fd_write_vmcore(const void *buf, size_t
> size, void *opaque)
>      DumpState *s = opaque;
>      size_t written_size;
>
> +    if (dump_cancelling()) {
> +        return -ECANCELED;
> +    }
> +
>      written_size = qemu_write_full(s->fd, buf, size);
>      if (written_size != size) {
>          return -errno;
> @@ -627,6 +633,10 @@ static void dump_iterate(DumpState *s, Error **errp)
>
>      do {
>          block = s->next_block;
> +        if (dump_cancelling()) {
> +            error_setg(errp, "dump: job cancelled");
> +            return;
> +        }
>
>          size = block->target_end - block->target_start;
>          if (s->has_filter) {
> @@ -1321,6 +1331,10 @@ static void write_dump_pages(DumpState *s, Error
> **errp)
>       * first page of page section
>       */
>      while (get_next_page(&block_iter, &pfn_iter, &buf, s)) {
> +        if (dump_cancelling()) {
> +            error_setg(errp, "dump: job cancelled");
> +            goto out;
> +        }
>          /* check zero page */
>          if (buffer_is_zero(buf, s->dump_info.page_size)) {
>              ret = write_cache(&page_desc, &pd_zero,
> sizeof(PageDescriptor),
> @@ -1548,6 +1562,15 @@ bool qemu_system_dump_in_progress(void)
>      return (qatomic_read(&state->status) == DUMP_STATUS_ACTIVE);
>  }
>
> +static bool dump_cancelling(void)
> +{
> +    DumpState *state = &dump_state_global;
> +    if (state->job && job_is_cancelled(state->job)) {
> +        return true;
> +    }
> +    return false;
> +}
> +
>  /* calculate total size of memory to be dumped (taking filter into
>   * acoount.) */
>  static int64_t dump_calculate_size(DumpState *s)
> @@ -1894,6 +1917,7 @@ static int coroutine_fn
> dump_guest_memory_job_run(Job *job, Error **errp)
>
>      s->errp = errp;
>      s->co = qemu_coroutine_self();
> +    state->job = job;
>
>      if (state->detached) {
>          /* detached dump */
> diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
> index ffc2ea1072..41bdbe595f 100644
> --- a/include/sysemu/dump.h
> +++ b/include/sysemu/dump.h
> @@ -15,6 +15,7 @@
>  #define DUMP_H
>
>  #include "qapi/qapi-types-dump.h"
> +#include "qemu/job.h"
>
>  #define MAKEDUMPFILE_SIGNATURE      "makedumpfile"
>  #define MAX_SIZE_MDF_HEADER         (4096) /* max size of
> makedumpfile_header */
> @@ -154,6 +155,7 @@ typedef struct DumpState {
>      GuestPhysBlockList guest_phys_blocks;
>      ArchDumpInfo dump_info;
>      MemoryMappingList list;
> +    Job *job;
>      uint32_t phdr_num;
>      uint32_t shdr_num;
>      bool resume;
> --
> 2.33.0
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 5006 bytes --]

Re: [PATCH 3/3] dump: support cancel dump process

[Kevin Wolf]

Am 28.07.2022 um 14:37 hat Marc-André Lureau geschrieben:
> Hi
> 
> On Wed, Jul 27, 2022 at 6:02 PM Hogan Wang via <qemu-devel@nongnu.org>
> wrote:
> 
> > Break saving pages or dump iterate when dump job in cancel state,
> > make sure dump process exits as soon as possible.
> >
> > Signed-off-by: Hogan Wang <hogan.wang@huawei.com>
> >
> 
> Overall, the series looks good to me. Please send it with a top cover
> letter, so it can be processed by patchew too.
> 
> I am not familiar with the job infrastructure, it would be nice if Kevin
> could check the usage or give some advice.

There is one big point I see with this series, which is that it could be
considered an incompatible change to 'dump-guest-memory'. Clients are
expected to manage the job now. Though in practice with the default
settings, maybe it actually just results in clients receiving additional
QMP events. (Technically, it is still incompatible because the command
will now fail if you have another job called 'memory-guest-dump' - no
good reason to have that, but it's a scenario that worked and breaks
after this series.)

Markus, do you have an opinion on whether job creation must be
explicitly enabled with a new option or if we can just switch existing
callers?

The implementation of a very basic job looks mostly okay to me, though
of course it doesn't support a few things like pausing the job and
proper progress monitoring. But these things are optional, so it's not a
blocker.

The one thing I would strongly recommend to include is an auto-dismiss
option like every other job has. It is required so that management tools
can query the final job status before it goes away. Most of the
information is in QMP events, too, but events can be lost. auto-finalize
isn't required for this job because it doesn't actually do anything in
the finalize phase.

I'm not sure how safe the whole thing is when it runs in the background
and you can send additional QMP commands while it's running, but that is
preexisting with detach=true.

Kevin

Re: [PATCH 3/3] dump: support cancel dump process

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 28.07.2022 um 14:37 hat Marc-André Lureau geschrieben:
>> Hi
>> 
>> On Wed, Jul 27, 2022 at 6:02 PM Hogan Wang via <qemu-devel@nongnu.org>
>> wrote:
>> 
>> > Break saving pages or dump iterate when dump job in cancel state,
>> > make sure dump process exits as soon as possible.
>> >
>> > Signed-off-by: Hogan Wang <hogan.wang@huawei.com>
>> >
>> 
>> Overall, the series looks good to me. Please send it with a top cover
>> letter, so it can be processed by patchew too.
>> 
>> I am not familiar with the job infrastructure, it would be nice if Kevin
>> could check the usage or give some advice.
>
> There is one big point I see with this series, which is that it could be
> considered an incompatible change to 'dump-guest-memory'. Clients are
> expected to manage the job now. Though in practice with the default
> settings, maybe it actually just results in clients receiving additional
> QMP events. (Technically, it is still incompatible because the command
> will now fail if you have another job called 'memory-guest-dump' - no
> good reason to have that, but it's a scenario that worked and breaks
> after this series.)
>
> Markus, do you have an opinion on whether job creation must be
> explicitly enabled with a new option or if we can just switch existing
> callers?

The conservative answer is to create a job only when new optional
argument @job-id is present, else maintain the traditional behavior.
This means we get to maintain two variations of the command: with and
without a job.

I can see the following alternatives:

(1) Keep both variations forever.  This could make sense if we believe
    the additional complexity and maintenance burden is insignificant.

(2) Deprecate "without a job", and remove it after a suitable grace
    period.  @job-id becomes mandatory then.

(3) Create a job even when the user doesn't ask for it (@job-id absent),
    accept the change in behavior.  This could make sense if we're
    confident the change is harmless in practice.  First step would be
    documenting the change in behavior.  Second step would be the
    argument why it's harmless.

    We may want to deprecate absent @job-id then, so we can get rid of
    the special case after a suitable grace period.

Does this answer your question, Kevin?

> The implementation of a very basic job looks mostly okay to me, though
> of course it doesn't support a few things like pausing the job and
> proper progress monitoring. But these things are optional, so it's not a
> blocker.

We can always improve on top.

> The one thing I would strongly recommend to include is an auto-dismiss
> option like every other job has. It is required so that management tools
> can query the final job status before it goes away. Most of the
> information is in QMP events, too, but events can be lost.

Concur.  Transmitting important information in QMP events only is an
interface design flaw.

>                                                            auto-finalize
> isn't required for this job because it doesn't actually do anything in
> the finalize phase.
>
> I'm not sure how safe the whole thing is when it runs in the background
> and you can send additional QMP commands while it's running, but that is
> preexisting with detach=true.
>
> Kevin