diff mbox series

[v2,17/40] job: Move BlockJobCreateFlags to Job

Message ID 20180518132114.4070-18-kwolf@redhat.com
State New
Headers show
Series Generic background jobs | expand

Commit Message

Kevin Wolf May 18, 2018, 1:20 p.m. UTC
This renames the BlockJobCreateFlags constants, moves a few JOB_INTERNAL
checks to job_create() and the auto_{finalize,dismiss} fields from
BlockJob to Job.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 include/block/blockjob.h     | 17 -----------------
 include/block/blockjob_int.h |  3 +--
 include/qemu/job.h           | 20 +++++++++++++++++++-
 block/commit.c               |  2 +-
 block/mirror.c               |  2 +-
 block/replication.c          |  4 ++--
 block/stream.c               |  2 +-
 blockdev.c                   | 14 +++++++-------
 blockjob.c                   | 27 +++++++--------------------
 job.c                        | 11 ++++++++++-
 qemu-img.c                   |  2 +-
 tests/test-blockjob-txn.c    |  2 +-
 tests/test-blockjob.c        |  4 ++--
 13 files changed, 53 insertions(+), 57 deletions(-)

Comments

John Snow May 23, 2018, 10:24 p.m. UTC | #1
On 05/18/2018 09:20 AM, Kevin Wolf wrote:
> +    job->auto_finalize = !(flags & JOB_MANUAL_FINALIZE);
> +    job->auto_dismiss  = !(flags & JOB_MANUAL_DISMISS);

Job API might be a good chance to say "No, this is the default behavior
for this API."

I don't know how possible this is, but could we remove these behavior
flags for jobs (but keep them for block jobs), and then any legacy block
job creation interfaces we have can enable/disable them as the user
requested,

and the block job layer itself has hooks that persuade the core job
layer to automatically transition without user input, if appropriate.

(Unless that happens later?)
Kevin Wolf May 24, 2018, 8:17 a.m. UTC | #2
Am 24.05.2018 um 00:24 hat John Snow geschrieben:
> 
> 
> On 05/18/2018 09:20 AM, Kevin Wolf wrote:
> > +    job->auto_finalize = !(flags & JOB_MANUAL_FINALIZE);
> > +    job->auto_dismiss  = !(flags & JOB_MANUAL_DISMISS);
> 
> Job API might be a good chance to say "No, this is the default behavior
> for this API."
> 
> I don't know how possible this is, but could we remove these behavior
> flags for jobs (but keep them for block jobs), and then any legacy block
> job creation interfaces we have can enable/disable them as the user
> requested,
> 
> and the block job layer itself has hooks that persuade the core job
> layer to automatically transition without user input, if appropriate.
> 
> (Unless that happens later?)

That's really a question for job-create, which we don't have yet. I'm
not sure whether job-create would expose those options, and if so, what
the defaults would be.

I think auto-dismiss=false is a good default because the reason for
having a manual job-dismiss is shared by all jobs (you want to be able
to query the job result after it finished).

auto-finalize is different. We introduced it to avoid surprise graph
changes, but this implies that every job changes the block graph when it
completes. This is obviously not true for non-block jobs. They might
still commonly change something else that shouldn't be done as a
surprise, but I am working right now on at least a single job type that
doesn't do anything like that on completion (blockdev-create).

For now, I made it unconditionally auto-finalize=true and
auto-dismiss=false without offering the options in QMP, but that's open
for discussion.

Kevin
John Snow May 24, 2018, 5:46 p.m. UTC | #3
On 05/24/2018 04:17 AM, Kevin Wolf wrote:
> Am 24.05.2018 um 00:24 hat John Snow geschrieben:
>>
>>
>> On 05/18/2018 09:20 AM, Kevin Wolf wrote:
>>> +    job->auto_finalize = !(flags & JOB_MANUAL_FINALIZE);
>>> +    job->auto_dismiss  = !(flags & JOB_MANUAL_DISMISS);
>>
>> Job API might be a good chance to say "No, this is the default behavior
>> for this API."
>>
>> I don't know how possible this is, but could we remove these behavior
>> flags for jobs (but keep them for block jobs), and then any legacy block
>> job creation interfaces we have can enable/disable them as the user
>> requested,
>>
>> and the block job layer itself has hooks that persuade the core job
>> layer to automatically transition without user input, if appropriate.
>>
>> (Unless that happens later?)
> 
> That's really a question for job-create, which we don't have yet. I'm
> not sure whether job-create would expose those options, and if so, what
> the defaults would be.
> 
> I think auto-dismiss=false is a good default because the reason for
> having a manual job-dismiss is shared by all jobs (you want to be able
> to query the job result after it finished).
> 

Sounds good to me.

> auto-finalize is different. We introduced it to avoid surprise graph
> changes, but this implies that every job changes the block graph when it
> completes. This is obviously not true for non-block jobs. They might
> still commonly change something else that shouldn't be done as a
> surprise, but I am working right now on at least a single job type that
> doesn't do anything like that on completion (blockdev-create).
> 

I'll have to stay tuned.

> For now, I made it unconditionally auto-finalize=true and
> auto-dismiss=false without offering the options in QMP, but that's open
> for discussion.

Sure. I was wondering if we didn't want to change the internal API's
defaults around, but I guess that can happen whenever based on what the
QMP looks like.

> 
> Kevin
>
diff mbox series

Patch

diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 3e94e18850..f9aaaaa835 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -91,27 +91,10 @@  typedef struct BlockJob {
     /** ret code passed to block_job_completed. */
     int ret;
 
-    /** True if this job should automatically finalize itself */
-    bool auto_finalize;
-
-    /** True if this job should automatically dismiss itself */
-    bool auto_dismiss;
-
     BlockJobTxn *txn;
     QLIST_ENTRY(BlockJob) txn_list;
 } BlockJob;
 
-typedef enum BlockJobCreateFlags {
-    /* Default behavior */
-    BLOCK_JOB_DEFAULT = 0x00,
-    /* BlockJob is not QMP-created and should not send QMP events */
-    BLOCK_JOB_INTERNAL = 0x01,
-    /* BlockJob requires manual finalize step */
-    BLOCK_JOB_MANUAL_FINALIZE = 0x02,
-    /* BlockJob requires manual dismiss step */
-    BLOCK_JOB_MANUAL_DISMISS = 0x04,
-} BlockJobCreateFlags;
-
 /**
  * block_job_next:
  * @job: A block job, or %NULL.
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index 7e705ae0e9..88639f7efc 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -106,8 +106,7 @@  struct BlockJobDriver {
  * @bs: The block
  * @perm, @shared_perm: Permissions to request for @bs
  * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
- * @flags: Creation flags for the Block Job.
- *         See @BlockJobCreateFlags
+ * @flags: Creation flags for the Block Job. See @JobCreateFlags.
  * @cb: Completion function for the job.
  * @opaque: Opaque pointer value passed to @cb.
  * @errp: Error object.
diff --git a/include/qemu/job.h b/include/qemu/job.h
index 858f3beea4..9783e4049b 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -99,6 +99,12 @@  typedef struct Job {
     /** Set to true when the job has deferred work to the main loop. */
     bool deferred_to_main_loop;
 
+    /** True if this job should automatically finalize itself */
+    bool auto_finalize;
+
+    /** True if this job should automatically dismiss itself */
+    bool auto_dismiss;
+
     /** Element of the list of jobs */
     QLIST_ENTRY(Job) job_list;
 } Job;
@@ -140,6 +146,17 @@  struct JobDriver {
     void (*free)(Job *job);
 };
 
+typedef enum JobCreateFlags {
+    /* Default behavior */
+    JOB_DEFAULT = 0x00,
+    /* Job is not QMP-created and should not send QMP events */
+    JOB_INTERNAL = 0x01,
+    /* Job requires manual finalize step */
+    JOB_MANUAL_FINALIZE = 0x02,
+    /* Job requires manual dismiss step */
+    JOB_MANUAL_DISMISS = 0x04,
+} JobCreateFlags;
+
 
 /**
  * Create a new long-running job and return it.
@@ -147,10 +164,11 @@  struct JobDriver {
  * @job_id: The id of the newly-created job, or %NULL for internal jobs
  * @driver: The class object for the newly-created job.
  * @ctx: The AioContext to run the job coroutine in.
+ * @flags: Creation flags for the job. See @JobCreateFlags.
  * @errp: Error object.
  */
 void *job_create(const char *job_id, const JobDriver *driver, AioContext *ctx,
-                 Error **errp);
+                 int flags, Error **errp);
 
 /**
  * Add a reference to Job refcnt, it will be decreased with job_unref, and then
diff --git a/block/commit.c b/block/commit.c
index c4a98e5804..7a6ae59d42 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -282,7 +282,7 @@  void commit_start(const char *job_id, BlockDriverState *bs,
     }
 
     s = block_job_create(job_id, &commit_job_driver, NULL, bs, 0, BLK_PERM_ALL,
-                         speed, BLOCK_JOB_DEFAULT, NULL, NULL, errp);
+                         speed, JOB_DEFAULT, NULL, NULL, errp);
     if (!s) {
         return;
     }
diff --git a/block/mirror.c b/block/mirror.c
index 9a7226f38c..5091e72554 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1284,7 +1284,7 @@  void mirror_start(const char *job_id, BlockDriverState *bs,
     }
     is_none_mode = mode == MIRROR_SYNC_MODE_NONE;
     base = mode == MIRROR_SYNC_MODE_TOP ? backing_bs(bs) : NULL;
-    mirror_start_job(job_id, bs, BLOCK_JOB_DEFAULT, target, replaces,
+    mirror_start_job(job_id, bs, JOB_DEFAULT, target, replaces,
                      speed, granularity, buf_size, backing_mode,
                      on_source_error, on_target_error, unmap, NULL, NULL,
                      &mirror_job_driver, is_none_mode, base, false,
diff --git a/block/replication.c b/block/replication.c
index 9ed6e0fb04..ff2e2028af 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -568,7 +568,7 @@  static void replication_start(ReplicationState *rs, ReplicationMode mode,
         job = backup_job_create(NULL, 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,
+                                BLOCKDEV_ON_ERROR_REPORT, JOB_INTERNAL,
                                 backup_job_completed, bs, NULL, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
@@ -693,7 +693,7 @@  static void replication_stop(ReplicationState *rs, bool failover, Error **errp)
 
         s->stage = BLOCK_REPLICATION_FAILOVER;
         commit_active_start(NULL, s->active_disk->bs, s->secondary_disk->bs,
-                            BLOCK_JOB_INTERNAL, 0, BLOCKDEV_ON_ERROR_REPORT,
+                            JOB_INTERNAL, 0, BLOCKDEV_ON_ERROR_REPORT,
                             NULL, replication_done, bs, true, errp);
         break;
     default:
diff --git a/block/stream.c b/block/stream.c
index e81b488a22..eee02538ed 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -242,7 +242,7 @@  void stream_start(const char *job_id, BlockDriverState *bs,
                          BLK_PERM_GRAPH_MOD,
                          BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED |
                          BLK_PERM_WRITE,
-                         speed, BLOCK_JOB_DEFAULT, NULL, NULL, errp);
+                         speed, JOB_DEFAULT, NULL, NULL, errp);
     if (!s) {
         goto fail;
     }
diff --git a/blockdev.c b/blockdev.c
index 3533c0dd6a..278b92ce03 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3244,7 +3244,7 @@  void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
             goto out;
         }
         commit_active_start(has_job_id ? job_id : NULL, bs, base_bs,
-                            BLOCK_JOB_DEFAULT, speed, on_error,
+                            JOB_DEFAULT, speed, on_error,
                             filter_node_name, NULL, NULL, false, &local_err);
     } else {
         BlockDriverState *overlay_bs = bdrv_find_overlay(bs, top_bs);
@@ -3275,7 +3275,7 @@  static BlockJob *do_drive_backup(DriveBackup *backup, BlockJobTxn *txn,
     AioContext *aio_context;
     QDict *options = NULL;
     Error *local_err = NULL;
-    int flags, job_flags = BLOCK_JOB_DEFAULT;
+    int flags, job_flags = JOB_DEFAULT;
     int64_t size;
     bool set_backing_hd = false;
 
@@ -3398,10 +3398,10 @@  static BlockJob *do_drive_backup(DriveBackup *backup, BlockJobTxn *txn,
         }
     }
     if (!backup->auto_finalize) {
-        job_flags |= BLOCK_JOB_MANUAL_FINALIZE;
+        job_flags |= JOB_MANUAL_FINALIZE;
     }
     if (!backup->auto_dismiss) {
-        job_flags |= BLOCK_JOB_MANUAL_DISMISS;
+        job_flags |= JOB_MANUAL_DISMISS;
     }
 
     job = backup_job_create(backup->job_id, bs, target_bs, backup->speed,
@@ -3442,7 +3442,7 @@  BlockJob *do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn,
     Error *local_err = NULL;
     AioContext *aio_context;
     BlockJob *job = NULL;
-    int job_flags = BLOCK_JOB_DEFAULT;
+    int job_flags = JOB_DEFAULT;
 
     if (!backup->has_speed) {
         backup->speed = 0;
@@ -3491,10 +3491,10 @@  BlockJob *do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn,
         }
     }
     if (!backup->auto_finalize) {
-        job_flags |= BLOCK_JOB_MANUAL_FINALIZE;
+        job_flags |= JOB_MANUAL_FINALIZE;
     }
     if (!backup->auto_dismiss) {
-        job_flags |= BLOCK_JOB_MANUAL_DISMISS;
+        job_flags |= JOB_MANUAL_DISMISS;
     }
     job = backup_job_create(backup->job_id, bs, target_bs, backup->speed,
                             backup->sync, NULL, backup->compress,
diff --git a/blockjob.c b/blockjob.c
index a1d1f488e9..d9d8ff7f5c 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -285,7 +285,7 @@  static void block_job_do_dismiss(BlockJob *job)
 static void block_job_conclude(BlockJob *job)
 {
     job_state_transition(&job->job, JOB_STATUS_CONCLUDED);
-    if (job->auto_dismiss || !job_started(&job->job)) {
+    if (job->job.auto_dismiss || !job_started(&job->job)) {
         block_job_do_dismiss(job);
     }
 }
@@ -483,7 +483,7 @@  static void block_job_completed_txn_abort(BlockJob *job)
 
 static int block_job_needs_finalize(BlockJob *job)
 {
-    return !job->auto_finalize;
+    return !job->job.auto_finalize;
 }
 
 static void block_job_do_finalize(BlockJob *job)
@@ -688,8 +688,8 @@  BlockJobInfo *block_job_query(BlockJob *job, Error **errp)
     info->io_status = job->iostatus;
     info->ready     = job->ready;
     info->status    = job->job.status;
-    info->auto_finalize = job->auto_finalize;
-    info->auto_dismiss  = job->auto_dismiss;
+    info->auto_finalize = job->job.auto_finalize;
+    info->auto_dismiss  = job->job.auto_dismiss;
     info->has_error = job->ret != 0;
     info->error     = job->ret ? g_strdup(strerror(-job->ret)) : NULL;
     return info;
@@ -736,7 +736,7 @@  static void block_job_event_completed(BlockJob *job, const char *msg)
 static int block_job_event_pending(BlockJob *job)
 {
     job_state_transition(&job->job, JOB_STATUS_PENDING);
-    if (!job->auto_finalize && !block_job_is_internal(job)) {
+    if (!job->job.auto_finalize && !block_job_is_internal(job)) {
         qapi_event_send_block_job_pending(job_type(&job->job),
                                           job->job.id,
                                           &error_abort);
@@ -763,19 +763,8 @@  void *block_job_create(const char *job_id, const BlockJobDriver *driver,
         return NULL;
     }
 
-    if (job_id == NULL && !(flags & BLOCK_JOB_INTERNAL)) {
+    if (job_id == NULL && !(flags & JOB_INTERNAL)) {
         job_id = bdrv_get_device_name(bs);
-        if (!*job_id) {
-            error_setg(errp, "An explicit job ID is required for this node");
-            return NULL;
-        }
-    }
-
-    if (job_id) {
-        if (flags & BLOCK_JOB_INTERNAL) {
-            error_setg(errp, "Cannot specify job ID for internal block job");
-            return NULL;
-        }
     }
 
     blk = blk_new(perm, shared_perm);
@@ -786,7 +775,7 @@  void *block_job_create(const char *job_id, const BlockJobDriver *driver,
     }
 
     job = job_create(job_id, &driver->job_driver, blk_get_aio_context(blk),
-                     errp);
+                     flags, errp);
     if (job == NULL) {
         blk_unref(blk);
         return NULL;
@@ -800,8 +789,6 @@  void *block_job_create(const char *job_id, const BlockJobDriver *driver,
     job->blk           = blk;
     job->cb            = cb;
     job->opaque        = opaque;
-    job->auto_finalize = !(flags & BLOCK_JOB_MANUAL_FINALIZE);
-    job->auto_dismiss  = !(flags & BLOCK_JOB_MANUAL_DISMISS);
 
     error_setg(&job->blocker, "block device is in use by block job: %s",
                job_type_str(&job->job));
diff --git a/job.c b/job.c
index 1e432af0d6..743d38d0d9 100644
--- a/job.c
+++ b/job.c
@@ -182,11 +182,15 @@  static void job_sleep_timer_cb(void *opaque)
 }
 
 void *job_create(const char *job_id, const JobDriver *driver, AioContext *ctx,
-                 Error **errp)
+                 int flags, Error **errp)
 {
     Job *job;
 
     if (job_id) {
+        if (flags & JOB_INTERNAL) {
+            error_setg(errp, "Cannot specify job ID for internal job");
+            return NULL;
+        }
         if (!id_wellformed(job_id)) {
             error_setg(errp, "Invalid job ID '%s'", job_id);
             return NULL;
@@ -195,6 +199,9 @@  void *job_create(const char *job_id, const JobDriver *driver, AioContext *ctx,
             error_setg(errp, "Job ID '%s' already in use", job_id);
             return NULL;
         }
+    } else if (!(flags & JOB_INTERNAL)) {
+        error_setg(errp, "An explicit job ID is required");
+        return NULL;
     }
 
     job = g_malloc0(driver->instance_size);
@@ -205,6 +212,8 @@  void *job_create(const char *job_id, const JobDriver *driver, AioContext *ctx,
     job->busy          = false;
     job->paused        = true;
     job->pause_count   = 1;
+    job->auto_finalize = !(flags & JOB_MANUAL_FINALIZE);
+    job->auto_dismiss  = !(flags & JOB_MANUAL_DISMISS);
 
     job_state_transition(job, JOB_STATUS_CREATED);
     aio_timer_init(qemu_get_aio_context(), &job->sleep_timer,
diff --git a/qemu-img.c b/qemu-img.c
index e6a689c794..12b5818641 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1015,7 +1015,7 @@  static int img_commit(int argc, char **argv)
 
     aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
-    commit_active_start("commit", bs, base_bs, BLOCK_JOB_DEFAULT, 0,
+    commit_active_start("commit", bs, base_bs, JOB_DEFAULT, 0,
                         BLOCKDEV_ON_ERROR_REPORT, NULL, common_block_job_cb,
                         &cbi, false, &local_err);
     aio_context_release(aio_context);
diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
index 93d1ff0859..60e9fa2298 100644
--- a/tests/test-blockjob-txn.c
+++ b/tests/test-blockjob-txn.c
@@ -107,7 +107,7 @@  static BlockJob *test_block_job_start(unsigned int iterations,
 
     snprintf(job_id, sizeof(job_id), "job%u", counter++);
     s = block_job_create(job_id, &test_block_job_driver, txn, bs,
-                         0, BLK_PERM_ALL, 0, BLOCK_JOB_DEFAULT,
+                         0, BLK_PERM_ALL, 0, JOB_DEFAULT,
                          test_block_job_cb, data, &error_abort);
     s->iterations = iterations;
     s->use_timer = use_timer;
diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
index ceb59600ed..8bb0aa8f85 100644
--- a/tests/test-blockjob.c
+++ b/tests/test-blockjob.c
@@ -59,7 +59,7 @@  static BlockJob *do_test_id(BlockBackend *blk, const char *id,
                             bool should_succeed)
 {
     return mk_job(blk, id, &test_block_job_driver,
-                  should_succeed, BLOCK_JOB_DEFAULT);
+                  should_succeed, JOB_DEFAULT);
 }
 
 /* This creates a BlockBackend (optionally with a name) with a
@@ -214,7 +214,7 @@  static CancelJob *create_common(BlockJob **pjob)
 
     blk = create_blk(NULL);
     job = mk_job(blk, "Steve", &test_cancel_driver, true,
-                 BLOCK_JOB_MANUAL_FINALIZE | BLOCK_JOB_MANUAL_DISMISS);
+                 JOB_MANUAL_FINALIZE | JOB_MANUAL_DISMISS);
     job_ref(&job->job);
     assert(job->job.status == JOB_STATUS_CREATED);
     s = container_of(job, CancelJob, common);