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

[PATCH v4 1/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           | 23 +++++++++++++++++++++++
 include/sysemu/dump.h |  2 ++
 2 files changed, 25 insertions(+)

diff --git a/dump/dump.c b/dump/dump.c
index 4d9658ffa2..a57c580b12 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),
@@ -1540,6 +1554,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)
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

[PATCH v4 2/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 a57c580b12..cec9be30b4 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -1895,6 +1895,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..d162a9f028 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. To be compatible
+#          with legacy dump process, @job-id should omitted. (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

Re: [PATCH v4 2/3] job: introduce dump guest memory job

[Markus Armbruster]

Hogan Wang <hogan.wang@huawei.com> writes:

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

Simpler:

       const char *job_id = qdict_get_try_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,

This becomes

       qmp_dump_guest_memory(paging, prot, !!job_id, job_id,

then.

> +                          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 a57c580b12..cec9be30b4 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -1895,6 +1895,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..d162a9f028 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. To be compatible
> +#          with legacy dump process, @job-id should omitted. (Since 7.2)
> +#

I think we need to describe things in more detail.

What are the behavioral differences between dumping with and without 
@job-id?

Why would you want to pass @job-id?  I figure it's to gain the ability
to monitor and control dump task with query-job, job-cancel, ...

>  # @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).

Hmm, does "detach": false make any sense when "job-id" is present?

Preexisting: @detach's default is undocumented.

> @@ -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:

[PATCH v4 3/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.

When caller pass the 'job-id' argument, create a job for dump process.
And then caller can use job-cancel QMP command to cancel the detached
dump process, use job-dismiss QMP command to release job object.

Examples:
Start dump job:
{"execute": "dump-guest-memory", "arguments": { "job-id": "dump-guest-memory",
                                                "protocol": "file:/tmp/vm.dump",
                                                "paging": false,
                                                "format": "elf",
                                                "detach": true
                                              }}

Cancel dump job:
{"execute": "job-cancel", "arguments": { "id": "dump-guest-memory" }}

Dismiss dump job:
{"execute": "job-dismiss", "arguments": { "id": "dump-guest-memory" }}

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

diff --git a/dump/dump.c b/dump/dump.c
index cec9be30b4..3f4ed8e7a7 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -98,6 +98,7 @@ static int dump_cleanup(DumpState *s)
 {
     guest_phys_blocks_free(&s->guest_phys_blocks);
     memory_mapping_list_free(&s->list);
+    s->job = NULL;
     close(s->fd);
     g_free(s->guest_note);
     s->guest_note = NULL;
@@ -1542,6 +1543,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 */
@@ -1894,6 +1903,64 @@ DumpQueryResult *qmp_query_dump(Error **errp)
     return result;
 }
 
+static void *dump_job_thread(void *opaque)
+{
+    DumpJob *job = (DumpJob *)opaque;
+    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;
+}
+
+static void dump_sync_job_bh(void *opaque)
+{
+    dump_job_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_job_thread, job, 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,
+};
+
+static void dump_job_start(DumpState *state, const char *job_id,
+                           bool detach, Error **errp)
+{
+    DumpJob *job;
+
+    job = job_create(job_id, &dump_guest_memory_job_driver, NULL,
+                     qemu_get_aio_context(), JOB_MANUAL_DISMISS,
+                     NULL, NULL, errp);
+    if (!job) {
+        return;
+    }
+    state->detached = detach;
+    state->job = &job->common;
+    job->state = state;
+    job_start(&job->common);
+}
+
 void qmp_dump_guest_memory(bool paging, const char *file,
                            bool has_job_id, const char *job_id,
                            bool has_detach, bool detach,
@@ -2010,6 +2077,15 @@ void qmp_dump_guest_memory(bool paging, const char *file,
         return;
     }
 
+    if (has_job_id) {
+        dump_job_start(s, job_id, detach_p, errp);
+        if (*errp) {
+            qatomic_set(&s->status, DUMP_STATUS_FAILED);
+            dump_cleanup(s);
+        }
+        return;
+    }
+
     if (detach_p) {
         /* detached dump */
         s->detached = true;
-- 
2.33.0

Re: [PATCH v4 2/3] job: introduce dump guest memory job

[Wangjing(Hogan) via]

> Hogan Wang <hogan.wang@huawei.com> writes:
> 
> > 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(-)
> >
> > @@ -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");
> > +    }
> > +
> 
> Simpler:
> 
>        const char *job_id = qdict_get_try_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,
> 
> This becomes
> 
>        qmp_dump_guest_memory(paging, prot, !!job_id, job_id,
> 
> then.
> 
Accept the improvements.

> > --- 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. To be compatible
> > +#          with legacy dump process, @job-id should omitted. (Since 7.2)
> > +#
> 
> I think we need to describe things in more detail.
> 
> What are the behavioral differences between dumping with and without @job-id?
> 
> Why would you want to pass @job-id?  I figure it's to gain the ability to monitor and control dump task with query-job, job-cancel, ...
> 
Thanks for your review comments, I will write the detailed comment for @job-id in patch set for the next version.

> >  # @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).
> 
> Hmm, does "detach": false make any sense when "job-id" is present?
> 
I had tested in my environment,  "detach": false haven't got any sense when "job-id" is present,cann't cancel and query the job.
I will delete the code condition branch for non-detach dump job.

> Preexisting: @detach's default is undocumented.

Hogan Wang