[Qemu-devel] [PATCH 0/3] blockjobs: add explicit job culling

[Qemu-devel] [PATCH 0/3] blockjobs: add explicit job culling

[John Snow]

For jobs that complete when a monitor isn't looking, there's no way to
tell what the job's final return code was. We need to allow jobs to
remain in the list until queried for reliable management.

This is an RFC; tests are on the way.
(Tested only manually via qmp-shell for now.)

John Snow (3):
  blockjob: add manual-cull property
  qmp: add block-job-cull command
  blockjob: expose manual-cull property

 block/backup.c               | 20 +++++++++---------
 block/commit.c               |  2 +-
 block/mirror.c               |  2 +-
 block/replication.c          |  5 +++--
 block/stream.c               |  2 +-
 block/trace-events           |  1 +
 blockdev.c                   | 28 +++++++++++++++++++++----
 blockjob.c                   | 46 +++++++++++++++++++++++++++++++++++++++--
 include/block/block_int.h    |  8 +++++---
 include/block/blockjob.h     | 21 +++++++++++++++++++
 include/block/blockjob_int.h |  2 +-
 qapi/block-core.json         | 49 ++++++++++++++++++++++++++++++++++++--------
 tests/test-blockjob-txn.c    |  2 +-
 tests/test-blockjob.c        |  2 +-
 14 files changed, 155 insertions(+), 35 deletions(-)

-- 
2.9.5

[Qemu-devel] [PATCH 1/3] blockjob: add manual-cull property

[John Snow]

Add a "manual cull" property to block jobs that forces them to linger
in the block job list (visible to QMP queries) until the user
explicitly dismisses them via QMP.

The cull command itself is implemented in the next commit, and the
feature is exposed to drive-backup and blockdev-backup in the subsequent
commit.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/backup.c               | 20 +++++++++----------
 block/commit.c               |  2 +-
 block/mirror.c               |  2 +-
 block/replication.c          |  5 +++--
 block/stream.c               |  2 +-
 blockdev.c                   |  8 ++++----
 blockjob.c                   | 46 ++++++++++++++++++++++++++++++++++++++++++--
 include/block/block_int.h    |  8 +++++---
 include/block/blockjob.h     | 21 ++++++++++++++++++++
 include/block/blockjob_int.h |  2 +-
 qapi/block-core.json         |  7 ++++---
 tests/test-blockjob-txn.c    |  2 +-
 tests/test-blockjob.c        |  2 +-
 13 files changed, 97 insertions(+), 30 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 517c300..0b00908 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -532,15 +532,15 @@ static const BlockJobDriver backup_job_driver = {
     .drain                  = backup_drain,
 };
 
-BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
-                  BlockDriverState *target, int64_t speed,
-                  MirrorSyncMode sync_mode, BdrvDirtyBitmap *sync_bitmap,
-                  bool compress,
-                  BlockdevOnError on_source_error,
-                  BlockdevOnError on_target_error,
-                  int creation_flags,
-                  BlockCompletionFunc *cb, void *opaque,
-                  BlockJobTxn *txn, Error **errp)
+BlockJob *backup_job_create(const char *job_id, bool manual_cull,
+                            BlockDriverState *bs, BlockDriverState *target,
+                            int64_t speed, MirrorSyncMode sync_mode,
+                            BdrvDirtyBitmap *sync_bitmap, bool compress,
+                            BlockdevOnError on_source_error,
+                            BlockdevOnError on_target_error,
+                            int creation_flags,
+                            BlockCompletionFunc *cb, void *opaque,
+                            BlockJobTxn *txn, Error **errp)
 {
     int64_t len;
     BlockDriverInfo bdi;
@@ -608,7 +608,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
     }
 
     /* job->common.len is fixed, so we can't allow resize */
-    job = block_job_create(job_id, &backup_job_driver, bs,
+    job = block_job_create(job_id, &backup_job_driver, manual_cull, bs,
                            BLK_PERM_CONSISTENT_READ,
                            BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE |
                            BLK_PERM_WRITE_UNCHANGED | BLK_PERM_GRAPH_MOD,
diff --git a/block/commit.c b/block/commit.c
index 8f0e835..308a5fd 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -304,7 +304,7 @@ void commit_start(const char *job_id, BlockDriverState *bs,
         return;
     }
 
-    s = block_job_create(job_id, &commit_job_driver, bs, 0, BLK_PERM_ALL,
+    s = block_job_create(job_id, &commit_job_driver, false, bs, 0, BLK_PERM_ALL,
                          speed, BLOCK_JOB_DEFAULT, NULL, NULL, errp);
     if (!s) {
         return;
diff --git a/block/mirror.c b/block/mirror.c
index 6f5cb9f..013e73a 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1180,7 +1180,7 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
     }
 
     /* Make sure that the source is not resized while the job is running */
-    s = block_job_create(job_id, driver, mirror_top_bs,
+    s = block_job_create(job_id, driver, false, mirror_top_bs,
                          BLK_PERM_CONSISTENT_READ,
                          BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED |
                          BLK_PERM_WRITE | BLK_PERM_GRAPH_MOD, speed,
diff --git a/block/replication.c b/block/replication.c
index 3a4e682..6c59f00 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -539,8 +539,9 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
         bdrv_op_block_all(top_bs, s->blocker);
         bdrv_op_unblock(top_bs, BLOCK_OP_TYPE_DATAPLANE, s->blocker);
 
-        job = backup_job_create(NULL, s->secondary_disk->bs, s->hidden_disk->bs,
-                                0, MIRROR_SYNC_MODE_NONE, NULL, false,
+        job = backup_job_create(NULL, false, s->secondary_disk->bs,
+                                s->hidden_disk->bs, 0, MIRROR_SYNC_MODE_NONE,
+                                NULL, false,
                                 BLOCKDEV_ON_ERROR_REPORT,
                                 BLOCKDEV_ON_ERROR_REPORT, BLOCK_JOB_INTERNAL,
                                 backup_job_completed, bs, NULL, &local_err);
diff --git a/block/stream.c b/block/stream.c
index e6f7234..c644f34 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -244,7 +244,7 @@ void stream_start(const char *job_id, BlockDriverState *bs,
     /* Prevent concurrent jobs trying to modify the graph structure here, we
      * already have our own plans. Also don't allow resize as the image size is
      * queried only at the job start and then cached. */
-    s = block_job_create(job_id, &stream_job_driver, bs,
+    s = block_job_create(job_id, &stream_job_driver, false, bs,
                          BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED |
                          BLK_PERM_GRAPH_MOD,
                          BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED |
diff --git a/blockdev.c b/blockdev.c
index 56a6b24..eeb4986 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3290,8 +3290,8 @@ static BlockJob *do_drive_backup(DriveBackup *backup, BlockJobTxn *txn,
         }
     }
 
-    job = backup_job_create(backup->job_id, bs, target_bs, backup->speed,
-                            backup->sync, bmap, backup->compress,
+    job = backup_job_create(backup->job_id, false, bs, target_bs,
+                            backup->speed, backup->sync, bmap, backup->compress,
                             backup->on_source_error, backup->on_target_error,
                             BLOCK_JOB_DEFAULT, NULL, NULL, txn, &local_err);
     bdrv_unref(target_bs);
@@ -3369,8 +3369,8 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn,
             goto out;
         }
     }
-    job = backup_job_create(backup->job_id, bs, target_bs, backup->speed,
-                            backup->sync, NULL, backup->compress,
+    job = backup_job_create(backup->job_id, false, bs, target_bs,
+                            backup->speed, backup->sync, NULL, backup->compress,
                             backup->on_source_error, backup->on_target_error,
                             BLOCK_JOB_DEFAULT, NULL, NULL, txn, &local_err);
     if (local_err != NULL) {
diff --git a/blockjob.c b/blockjob.c
index 3a0c491..b4aaeb0 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -336,6 +336,8 @@ static void block_job_completed_single(BlockJob *job)
         QLIST_REMOVE(job, txn_list);
         block_job_txn_unref(job->txn);
     }
+
+    job->finished = true;
     block_job_unref(job);
 }
 
@@ -352,6 +354,13 @@ static void block_job_cancel_async(BlockJob *job)
     job->cancelled = true;
 }
 
+static void block_job_do_cull(BlockJob *job)
+{
+    assert(job && job->manual_cull == true);
+    job->manual_cull = false;
+    block_job_unref(job);
+}
+
 static int block_job_finish_sync(BlockJob *job,
                                  void (*finish)(BlockJob *, Error **errp),
                                  Error **errp)
@@ -381,6 +390,9 @@ static int block_job_finish_sync(BlockJob *job,
         aio_poll(qemu_get_aio_context(), true);
     }
     ret = (job->cancelled && job->ret == 0) ? -ECANCELED : job->ret;
+    if (job->manual_cull) {
+        block_job_do_cull(job);
+    }
     block_job_unref(job);
     return ret;
 }
@@ -483,6 +495,26 @@ void block_job_complete(BlockJob *job, Error **errp)
     job->driver->complete(job, errp);
 }
 
+void block_job_cull(BlockJob **jobptr, Error **errp)
+{
+    BlockJob *job = *jobptr;
+    /* similarly to _complete, this is QMP-interface only. */
+    assert(job->id);
+    if (!job->manual_cull) {
+        error_setg(errp, "The active block job '%s' was not started with "
+                   "\'manual-cull\': true, and so cannot be culled as it will "
+                   "clean up after itself automatically", job->id);
+        return;
+    } else if (!job->finished) {
+        error_setg(errp, "The active block job '%s' has not yet terminated, "
+                   "and cannot be culled yet", job->id);
+        return;
+    }
+
+    block_job_do_cull(job);
+    *jobptr = NULL;
+}
+
 void block_job_user_pause(BlockJob *job)
 {
     job->user_paused = true;
@@ -505,7 +537,9 @@ void block_job_user_resume(BlockJob *job)
 
 void block_job_cancel(BlockJob *job)
 {
-    if (block_job_started(job)) {
+    if (job->finished) {
+        return;
+    } else if (block_job_started(job)) {
         block_job_cancel_async(job);
         block_job_enter(job);
     } else {
@@ -562,6 +596,7 @@ BlockJobInfo *block_job_query(BlockJob *job, Error **errp)
     info->speed     = job->speed;
     info->io_status = job->iostatus;
     info->ready     = job->ready;
+    info->finished  = job->finished;
     return info;
 }
 
@@ -609,7 +644,7 @@ static void block_job_event_completed(BlockJob *job, const char *msg)
  */
 
 void *block_job_create(const char *job_id, const BlockJobDriver *driver,
-                       BlockDriverState *bs, uint64_t perm,
+                       bool manual_cull, BlockDriverState *bs, uint64_t perm,
                        uint64_t shared_perm, int64_t speed, int flags,
                        BlockCompletionFunc *cb, void *opaque, Error **errp)
 {
@@ -664,6 +699,7 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
     job->paused        = true;
     job->pause_count   = 1;
     job->refcnt        = 1;
+    job->manual_cull   = manual_cull;
 
     error_setg(&job->blocker, "block device is in use by block job: %s",
                BlockJobType_str(driver->job_type));
@@ -689,6 +725,12 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
             return NULL;
         }
     }
+
+    /* Hang on to an extra reference on behalf of the QMP monitor */
+    if (job->manual_cull) {
+        block_job_ref(job);
+    }
+
     return job;
 }
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 99abe2c..7d5a5d1 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -944,6 +944,8 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
  * backup_job_create:
  * @job_id: The id of the newly-created job, or %NULL to use the
  * device name of @bs.
+ * @manual_cull: Whether or not this job, when completed, will need to be
+ *               manually culled via block-job-cull or not.
  * @bs: Block device to operate on.
  * @target: Block device to write to.
  * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
@@ -960,9 +962,9 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
  * Create a backup operation on @bs.  Clusters in @bs are written to @target
  * until the job is cancelled or manually completed.
  */
-BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
-                            BlockDriverState *target, int64_t speed,
-                            MirrorSyncMode sync_mode,
+BlockJob *backup_job_create(const char *job_id, bool manual_cull,
+                            BlockDriverState *bs, BlockDriverState *target,
+                            int64_t speed, MirrorSyncMode sync_mode,
                             BdrvDirtyBitmap *sync_bitmap,
                             bool compress,
                             BlockdevOnError on_source_error,
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 67c0968..8ea20ba 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -135,6 +135,15 @@ typedef struct BlockJob {
      */
     int ret;
 
+    /* True if this job must remain present post-completion until
+     * it can be queried and removed via block-job-cull. */
+    bool manual_cull;
+
+    /* True if and only if manual_cull is true and the job has finished
+     * calling either abort or commit (if applicable) and can be reaped
+     * via interactive user command. */
+    bool finished;
+
     /** Non-NULL if this job is part of a transaction */
     BlockJobTxn *txn;
     QLIST_ENTRY(BlockJob) txn_list;
@@ -227,6 +236,18 @@ void block_job_cancel(BlockJob *job);
 void block_job_complete(BlockJob *job, Error **errp);
 
 /**
+ * block_job_cull:
+ * @job: The job to be culled.
+ * @errp: Error object.
+ *
+ * For completed or canceled jobs, remove the job object
+ * from the block_job_query list and perform final cleanup.
+ *
+ * Note that at this stage, the job cannot be rolled back.
+ */
+void block_job_cull(BlockJob **job, Error **errp);
+
+/**
  * block_job_query:
  * @job: The job to get information about.
  *
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index f13ad05..17e2c91 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -132,7 +132,7 @@ struct BlockJobDriver {
  * called from a wrapper that is specific to the job type.
  */
 void *block_job_create(const char *job_id, const BlockJobDriver *driver,
-                       BlockDriverState *bs, uint64_t perm,
+                       bool manual_cull, BlockDriverState *bs, uint64_t perm,
                        uint64_t shared_perm, int64_t speed, int flags,
                        BlockCompletionFunc *cb, void *opaque, Error **errp);
 
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 750bb0c..a4f5e10 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -984,9 +984,10 @@
 # Since: 1.1
 ##
 { 'struct': 'BlockJobInfo',
-  'data': {'type': 'str', 'device': 'str', 'len': 'int',
-           'offset': 'int', 'busy': 'bool', 'paused': 'bool', 'speed': 'int',
-           'io-status': 'BlockDeviceIoStatus', 'ready': 'bool'} }
+  'data': {'type': 'str', 'device': 'str', 'len': 'int', 'offset': 'int',
+           'busy': 'bool', 'paused': 'bool', 'speed': 'int',
+           'io-status': 'BlockDeviceIoStatus', 'ready': 'bool',
+           'finished': 'bool'} }
 
 ##
 # @query-block-jobs:
diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
index c77343f..5ab0e22 100644
--- a/tests/test-blockjob-txn.c
+++ b/tests/test-blockjob-txn.c
@@ -101,7 +101,7 @@ static BlockJob *test_block_job_start(unsigned int iterations,
     g_assert_nonnull(bs);
 
     snprintf(job_id, sizeof(job_id), "job%u", counter++);
-    s = block_job_create(job_id, &test_block_job_driver, bs,
+    s = block_job_create(job_id, &test_block_job_driver, false, bs,
                          0, BLK_PERM_ALL, 0, BLOCK_JOB_DEFAULT,
                          test_block_job_cb, data, &error_abort);
     s->iterations = iterations;
diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
index 23bdf1a..9fc2785 100644
--- a/tests/test-blockjob.c
+++ b/tests/test-blockjob.c
@@ -30,7 +30,7 @@ static BlockJob *do_test_id(BlockBackend *blk, const char *id,
     BlockJob *job;
     Error *errp = NULL;
 
-    job = block_job_create(id, &test_block_job_driver, blk_bs(blk),
+    job = block_job_create(id, &test_block_job_driver, false, blk_bs(blk),
                            0, BLK_PERM_ALL, 0, BLOCK_JOB_DEFAULT, block_job_cb,
                            NULL, &errp);
     if (should_succeed) {
-- 
2.9.5

[Qemu-devel] [PATCH 2/3] qmp: add block-job-cull command

[John Snow]

For jobs that have finished (either completed or canceled), allow the
user to dismiss the job's status reports via block-job-cull.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/trace-events   |  1 +
 blockdev.c           | 14 ++++++++++++++
 qapi/block-core.json | 21 +++++++++++++++++++++
 3 files changed, 36 insertions(+)

diff --git a/block/trace-events b/block/trace-events
index 25dd5a3..cb64e91 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -46,6 +46,7 @@ qmp_block_job_cancel(void *job) "job %p"
 qmp_block_job_pause(void *job) "job %p"
 qmp_block_job_resume(void *job) "job %p"
 qmp_block_job_complete(void *job) "job %p"
+qmp_block_job_cull(void *job) "job %p"
 qmp_block_stream(void *bs, void *job) "bs %p job %p"
 
 # block/file-win32.c
diff --git a/blockdev.c b/blockdev.c
index eeb4986..ee07bca 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3766,6 +3766,20 @@ void qmp_block_job_complete(const char *device, Error **errp)
     aio_context_release(aio_context);
 }
 
+void qmp_block_job_cull(const char *device, Error **errp)
+{
+    AioContext *aio_context;
+    BlockJob *job = find_block_job(device, &aio_context, errp);
+
+    if (!job) {
+        return;
+    }
+
+    trace_qmp_block_job_cull(job);
+    block_job_cull(&job, errp);
+    aio_context_release(aio_context);
+}
+
 void qmp_change_backing_file(const char *device,
                              const char *image_node_name,
                              const char *backing_file,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index a4f5e10..de322d1 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2161,6 +2161,27 @@
 { 'command': 'block-job-complete', 'data': { 'device': 'str' } }
 
 ##
+# @block-job-cull:
+#
+# For jobs that have already completed, remove them from the block-job-query
+# list. This command only needs to be run for jobs which were started with the
+# manual-cull=true option.
+#
+# This command will refuse to operate on any job that has not yet reached
+# its terminal state. "cancel" or "complete" will still need to be used as
+# appropriate.
+#
+# @device: The job identifier. This used to be a device name (hence
+#          the name of the parameter), but since QEMU 2.7 it can have
+#          other values.
+#
+# Returns: Nothing on success
+#
+# Since: 2.11
+##
+{ 'command': 'block-job-cull', 'data': { 'device': 'str' } }
+
+##
 # @BlockdevDiscardOptions:
 #
 # Determines how to handle discard requests.
-- 
2.9.5

[Qemu-devel] [PATCH 3/3] blockjob: expose manual-cull property

[John Snow]

For drive-backup and blockdev-backup, expose the manual-cull
property, having it default to false. There are no universal
creation parameters, so it must be added to each job type that
it makes sense for individually.

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

diff --git a/blockdev.c b/blockdev.c
index ee07bca..ba2ebfb 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3198,6 +3198,9 @@ static BlockJob *do_drive_backup(DriveBackup *backup, BlockJobTxn *txn,
     if (!backup->has_job_id) {
         backup->job_id = NULL;
     }
+    if (!backup->has_manual_cull) {
+        backup->manual_cull = false;
+    }
     if (!backup->has_compress) {
         backup->compress = false;
     }
@@ -3290,7 +3293,7 @@ static BlockJob *do_drive_backup(DriveBackup *backup, BlockJobTxn *txn,
         }
     }
 
-    job = backup_job_create(backup->job_id, false, bs, target_bs,
+    job = backup_job_create(backup->job_id, backup->manual_cull, bs, target_bs,
                             backup->speed, backup->sync, bmap, backup->compress,
                             backup->on_source_error, backup->on_target_error,
                             BLOCK_JOB_DEFAULT, NULL, NULL, txn, &local_err);
@@ -3341,6 +3344,9 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn,
     if (!backup->has_job_id) {
         backup->job_id = NULL;
     }
+    if (!backup->has_manual_cull) {
+        backup->manual_cull = false;
+    }
     if (!backup->has_compress) {
         backup->compress = false;
     }
@@ -3369,7 +3375,7 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn,
             goto out;
         }
     }
-    job = backup_job_create(backup->job_id, false, bs, target_bs,
+    job = backup_job_create(backup->job_id, backup->manual_cull, bs, target_bs,
                             backup->speed, backup->sync, NULL, backup->compress,
                             backup->on_source_error, backup->on_target_error,
                             BLOCK_JOB_DEFAULT, NULL, NULL, txn, &local_err);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index de322d1..c646743 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1104,6 +1104,11 @@
 # @job-id: identifier for the newly-created block job. If
 #          omitted, the device name will be used. (Since 2.7)
 #
+# @manual-cull: Whether or not the job created by this command needs to be
+#               cleaned up manually via block-job-cull or not. The default is
+#               false. When true, the job will remain in a "completed" state
+#               until culled manually with block-job-cull. (Since 2.11)
+#
 # @device: the device name or node-name of a root node which should be copied.
 #
 # @target: the target of the new image. If the file exists, or if it
@@ -1144,9 +1149,10 @@
 # Since: 1.6
 ##
 { 'struct': 'DriveBackup',
-  'data': { '*job-id': 'str', 'device': 'str', 'target': 'str',
-            '*format': 'str', 'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
-            '*speed': 'int', '*bitmap': 'str', '*compress': 'bool',
+  'data': { '*job-id': 'str', '*manual-cull': 'bool', 'device': 'str',
+            'target': 'str', '*format': 'str', 'sync': 'MirrorSyncMode',
+            '*mode': 'NewImageMode', '*speed': 'int', '*bitmap': 'str',
+            '*compress': 'bool',
             '*on-source-error': 'BlockdevOnError',
             '*on-target-error': 'BlockdevOnError' } }
 
@@ -1156,6 +1162,11 @@
 # @job-id: identifier for the newly-created block job. If
 #          omitted, the device name will be used. (Since 2.7)
 #
+# @manual-cull: Whether or not the job created by this command needs to be
+#               cleaned up manually via block-job-cull or not. The default is
+#               false. When true, the job will remain in a "completed" state
+#               until culled manually with block-job-cull. (Since 2.11)
+#
 # @device: the device name or node-name of a root node which should be copied.
 #
 # @target: the device name or node-name of the backup target node.
@@ -1185,8 +1196,8 @@
 # Since: 2.3
 ##
 { 'struct': 'BlockdevBackup',
-  'data': { '*job-id': 'str', 'device': 'str', 'target': 'str',
-            'sync': 'MirrorSyncMode',
+  'data': { '*job-id': 'str', '*manual-cull': 'bool', 'device': 'str',
+            'target': 'str', 'sync': 'MirrorSyncMode',
             '*speed': 'int',
             '*compress': 'bool',
             '*on-source-error': 'BlockdevOnError',
-- 
2.9.5

Re: [Qemu-devel] [PATCH 3/3] blockjob: expose manual-cull property

[Paolo Bonzini]

On 03/10/2017 05:15, John Snow wrote:
> For drive-backup and blockdev-backup, expose the manual-cull
> property, having it default to false. There are no universal
> creation parameters, so it must be added to each job type that
> it makes sense for individually.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  blockdev.c           | 10 ++++++++--
>  qapi/block-core.json | 21 ++++++++++++++++-----
>  2 files changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index ee07bca..ba2ebfb 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3198,6 +3198,9 @@ static BlockJob *do_drive_backup(DriveBackup *backup, BlockJobTxn *txn,
>      if (!backup->has_job_id) {
>          backup->job_id = NULL;
>      }
> +    if (!backup->has_manual_cull) {
> +        backup->manual_cull = false;
> +    }
>      if (!backup->has_compress) {
>          backup->compress = false;
>      }
> @@ -3290,7 +3293,7 @@ static BlockJob *do_drive_backup(DriveBackup *backup, BlockJobTxn *txn,
>          }
>      }
>  
> -    job = backup_job_create(backup->job_id, false, bs, target_bs,
> +    job = backup_job_create(backup->job_id, backup->manual_cull, bs, target_bs,
>                              backup->speed, backup->sync, bmap, backup->compress,
>                              backup->on_source_error, backup->on_target_error,
>                              BLOCK_JOB_DEFAULT, NULL, NULL, txn, &local_err);
> @@ -3341,6 +3344,9 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn,
>      if (!backup->has_job_id) {
>          backup->job_id = NULL;
>      }
> +    if (!backup->has_manual_cull) {
> +        backup->manual_cull = false;
> +    }
>      if (!backup->has_compress) {
>          backup->compress = false;
>      }
> @@ -3369,7 +3375,7 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn,
>              goto out;
>          }
>      }
> -    job = backup_job_create(backup->job_id, false, bs, target_bs,
> +    job = backup_job_create(backup->job_id, backup->manual_cull, bs, target_bs,
>                              backup->speed, backup->sync, NULL, backup->compress,
>                              backup->on_source_error, backup->on_target_error,
>                              BLOCK_JOB_DEFAULT, NULL, NULL, txn, &local_err);
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index de322d1..c646743 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1104,6 +1104,11 @@
>  # @job-id: identifier for the newly-created block job. If
>  #          omitted, the device name will be used. (Since 2.7)
>  #
> +# @manual-cull: Whether or not the job created by this command needs to be
> +#               cleaned up manually via block-job-cull or not. The default is
> +#               false. When true, the job will remain in a "completed" state
> +#               until culled manually with block-job-cull. (Since 2.11)
> +#
>  # @device: the device name or node-name of a root node which should be copied.
>  #
>  # @target: the target of the new image. If the file exists, or if it
> @@ -1144,9 +1149,10 @@
>  # Since: 1.6
>  ##
>  { 'struct': 'DriveBackup',
> -  'data': { '*job-id': 'str', 'device': 'str', 'target': 'str',
> -            '*format': 'str', 'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
> -            '*speed': 'int', '*bitmap': 'str', '*compress': 'bool',
> +  'data': { '*job-id': 'str', '*manual-cull': 'bool', 'device': 'str',
> +            'target': 'str', '*format': 'str', 'sync': 'MirrorSyncMode',
> +            '*mode': 'NewImageMode', '*speed': 'int', '*bitmap': 'str',
> +            '*compress': 'bool',
>              '*on-source-error': 'BlockdevOnError',
>              '*on-target-error': 'BlockdevOnError' } }
>  
> @@ -1156,6 +1162,11 @@
>  # @job-id: identifier for the newly-created block job. If
>  #          omitted, the device name will be used. (Since 2.7)
>  #
> +# @manual-cull: Whether or not the job created by this command needs to be
> +#               cleaned up manually via block-job-cull or not. The default is
> +#               false. When true, the job will remain in a "completed" state
> +#               until culled manually with block-job-cull. (Since 2.11)

The verb "cull" is a bit weird.  The only alternative that comes to mind
though are "reap" (like processes). There's also "join" (like threads),
but would imply waiting if the jobs hasn't completed yet, and we
probably don't want it.

Paolo

>  # @device: the device name or node-name of a root node which should be copied.
>  #
>  # @target: the device name or node-name of the backup target node.
> @@ -1185,8 +1196,8 @@
>  # Since: 2.3
>  ##
>  { 'struct': 'BlockdevBackup',
> -  'data': { '*job-id': 'str', 'device': 'str', 'target': 'str',
> -            'sync': 'MirrorSyncMode',
> +  'data': { '*job-id': 'str', '*manual-cull': 'bool', 'device': 'str',
> +            'target': 'str', 'sync': 'MirrorSyncMode',
>              '*speed': 'int',
>              '*compress': 'bool',
>              '*on-source-error': 'BlockdevOnError',
> 

Re: [Qemu-devel] [PATCH 3/3] blockjob: expose manual-cull property

[John Snow]

On 10/03/2017 11:57 AM, Paolo Bonzini wrote:
> On 03/10/2017 05:15, John Snow wrote:
>> For drive-backup and blockdev-backup, expose the manual-cull
>> property, having it default to false. There are no universal
>> creation parameters, so it must be added to each job type that
>> it makes sense for individually.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>

[...]

> 
> The verb "cull" is a bit weird.  The only alternative that comes to mind
> though are "reap" (like processes). There's also "join" (like threads),
> but would imply waiting if the jobs hasn't completed yet, and we
> probably don't want it.
> 
> Paolo
> 

Sure, open to suggestions. I think Kevin suggested "delete" which I have
reservations about because of people potentially confusing it with
"cancel" or "complete" -- it does not have the capacity to
end/terminate/finish/complete/cancel a job.

"reap" might be fine. I don't really have any strong preference.

--js

Re: [Qemu-devel] [PATCH 3/3] blockjob: expose manual-cull property

[Jeff Cody]

On Tue, Oct 03, 2017 at 11:59:28AM -0400, John Snow wrote:
> 
> 
> On 10/03/2017 11:57 AM, Paolo Bonzini wrote:
> > On 03/10/2017 05:15, John Snow wrote:
> >> For drive-backup and blockdev-backup, expose the manual-cull
> >> property, having it default to false. There are no universal
> >> creation parameters, so it must be added to each job type that
> >> it makes sense for individually.
> >>
> >> Signed-off-by: John Snow <jsnow@redhat.com>
> 
> [...]
> 
> > 
> > The verb "cull" is a bit weird.  The only alternative that comes to mind
> > though are "reap" (like processes). There's also "join" (like threads),
> > but would imply waiting if the jobs hasn't completed yet, and we
> > probably don't want it.
> > 
> > Paolo
> > 
> 
> Sure, open to suggestions. I think Kevin suggested "delete" which I have
> reservations about because of people potentially confusing it with
> "cancel" or "complete" -- it does not have the capacity to
> end/terminate/finish/complete/cancel a job.
> 
> "reap" might be fine. I don't really have any strong preference.
>

As far as verbs go, I like both 'reap' and 'delete'.  As far as the
property, naming it 'manual_verb' is a bit odd, too. Maybe a clearer term
for the property would just be 'persistent', with the QMP command being
'block_job_reap' or 'block_job_delete'?

-Jeff

Re: [Qemu-devel] [PATCH 3/3] blockjob: expose manual-cull property

[John Snow]

On 10/03/2017 01:43 PM, Jeff Cody wrote:
> On Tue, Oct 03, 2017 at 11:59:28AM -0400, John Snow wrote:
>>
>>
>> On 10/03/2017 11:57 AM, Paolo Bonzini wrote:
>>> On 03/10/2017 05:15, John Snow wrote:
>>>> For drive-backup and blockdev-backup, expose the manual-cull
>>>> property, having it default to false. There are no universal
>>>> creation parameters, so it must be added to each job type that
>>>> it makes sense for individually.
>>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>
>> [...]
>>
>>>
>>> The verb "cull" is a bit weird.  The only alternative that comes to mind
>>> though are "reap" (like processes). There's also "join" (like threads),
>>> but would imply waiting if the jobs hasn't completed yet, and we
>>> probably don't want it.
>>>
>>> Paolo
>>>
>>
>> Sure, open to suggestions. I think Kevin suggested "delete" which I have
>> reservations about because of people potentially confusing it with
>> "cancel" or "complete" -- it does not have the capacity to
>> end/terminate/finish/complete/cancel a job.
>>
>> "reap" might be fine. I don't really have any strong preference.
>>
> 
> As far as verbs go, I like both 'reap' and 'delete'.  As far as the
> property, naming it 'manual_verb' is a bit odd, too. Maybe a clearer term
> for the property would just be 'persistent', with the QMP command being
> 'block_job_reap' or 'block_job_delete'?
> 
> -Jeff
> 

As they say, two hard problems in Computer Science ...

Re: [Qemu-devel] [PATCH 0/3] blockjobs: add explicit job culling

[Vladimir Sementsov-Ogievskiy]

03.10.2017 06:15, John Snow wrote:
> For jobs that complete when a monitor isn't looking, there's no way to
> tell what the job's final return code was. We need to allow jobs to
> remain in the list until queried for reliable management.
>
> This is an RFC; tests are on the way.
> (Tested only manually via qmp-shell for now.)

That's a cool feature!
What about transactions support?

>
> John Snow (3):
>    blockjob: add manual-cull property
>    qmp: add block-job-cull command
>    blockjob: expose manual-cull property
>
>   block/backup.c               | 20 +++++++++---------
>   block/commit.c               |  2 +-
>   block/mirror.c               |  2 +-
>   block/replication.c          |  5 +++--
>   block/stream.c               |  2 +-
>   block/trace-events           |  1 +
>   blockdev.c                   | 28 +++++++++++++++++++++----
>   blockjob.c                   | 46 +++++++++++++++++++++++++++++++++++++++--
>   include/block/block_int.h    |  8 +++++---
>   include/block/blockjob.h     | 21 +++++++++++++++++++
>   include/block/blockjob_int.h |  2 +-
>   qapi/block-core.json         | 49 ++++++++++++++++++++++++++++++++++++--------
>   tests/test-blockjob-txn.c    |  2 +-
>   tests/test-blockjob.c        |  2 +-
>   14 files changed, 155 insertions(+), 35 deletions(-)
>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 0/3] blockjobs: add explicit job culling

[John Snow]

On 10/03/2017 05:20 AM, Vladimir Sementsov-Ogievskiy wrote:
> 03.10.2017 06:15, John Snow wrote:
>> For jobs that complete when a monitor isn't looking, there's no way to
>> tell what the job's final return code was. We need to allow jobs to
>> remain in the list until queried for reliable management.
>>
>> This is an RFC; tests are on the way.
>> (Tested only manually via qmp-shell for now.)
> 
> That's a cool feature!
> What about transactions support?
> 

Didn't test that yet (!) but the intent is that it will be compatible.
The jobs in the transaction, whether using grouped-completion mode or
not, will simply hang around in the list after completion.

For grouped-completion=false:

The jobs will complete individually and then remain in the list.

For grouped-completion=true:

The jobs will remain in their ready-to-commit-or-abort state until all
jobs in the transaction are ready to commit-or-abort, then all jobs will
either commit or abort. After commit-or-abort, all jobs (that were
created with manual-cull=true !) will remain in the query list.

The intended effect here is that this property changes NOTHING except
that the job will remain in the query list until it is dismissed, and
should not change anything about how it behaves during its lifetime.

One downside here is that since we have no universal "job creation
argument list" that I can't add it to all jobs universally. In the case
of transactions, though, I could at least add a property that *forces*
all jobs below to become manual-cull style jobs, and that way you'd only
have to specify it once instead of for each action.

--js

>>
>> John Snow (3):
>>    blockjob: add manual-cull property
>>    qmp: add block-job-cull command
>>    blockjob: expose manual-cull property
>>
>>   block/backup.c               | 20 +++++++++---------
>>   block/commit.c               |  2 +-
>>   block/mirror.c               |  2 +-
>>   block/replication.c          |  5 +++--
>>   block/stream.c               |  2 +-
>>   block/trace-events           |  1 +
>>   blockdev.c                   | 28 +++++++++++++++++++++----
>>   blockjob.c                   | 46
>> +++++++++++++++++++++++++++++++++++++++--
>>   include/block/block_int.h    |  8 +++++---
>>   include/block/blockjob.h     | 21 +++++++++++++++++++
>>   include/block/blockjob_int.h |  2 +-
>>   qapi/block-core.json         | 49
>> ++++++++++++++++++++++++++++++++++++--------
>>   tests/test-blockjob-txn.c    |  2 +-
>>   tests/test-blockjob.c        |  2 +-
>>   14 files changed, 155 insertions(+), 35 deletions(-)
>>
> 
> 

Oh, while I'm here, I should point out that another downside of this
patch is that it doesn't prevent "cancel" from attempting to re-enter
the job. Or rather, I had to patch that out specifically. The job
remains in a list of jobs that some portions of the code consider to be
"active" jobs. (look for any code that checks to see if the job has
started.)

A (perhaps) more provably cleaner approach would be to simply move any
completed job onto a different list upon completion, and patch
query-jobs to query both lists, and allow the cull command to remove any
jobs on that list. A downside of that approach, however, is that without
multiple job support, you may launch a second job that perhaps
overwrites the first job unless you're careful about how you manage that
data.

There are pros and cons to either way, but I'd rather not get in the
business of overhauling the blockjobs API unless it's for universal jobs.

--js