diff mbox series

[14/42] job: Add reference counting

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

Commit Message

Kevin Wolf May 9, 2018, 4:26 p.m. UTC 
This moves reference counting from BlockJob to Job.

In order to keep calling the BlockJob cleanup code when the job is
deleted via job_unref(), introduce a new JobDriver.free callback. Every
block job must use block_job_free() for this callback, this is asserted
in block_job_create().

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/blockjob.h     | 21 -------------------
 include/block/blockjob_int.h |  7 +++++++
 include/qemu/job.h           | 19 ++++++++++++++++--
 block/backup.c               |  1 +
 block/commit.c               |  1 +
 block/mirror.c               |  2 ++
 block/stream.c               |  1 +
 blockjob.c                   | 48 +++++++++++++++++++-------------------------
 job.c                        | 22 ++++++++++++++++----
 qemu-img.c                   |  4 ++--
 tests/test-bdrv-drain.c      |  1 +
 tests/test-blockjob-txn.c    |  1 +
 tests/test-blockjob.c        |  6 ++++--
 13 files changed, 76 insertions(+), 58 deletions(-)

Comments

Max Reitz May 14, 2018, 2:29 p.m. UTC | #1 
On 2018-05-09 18:26, Kevin Wolf wrote:
> This moves reference counting from BlockJob to Job.
> 
> In order to keep calling the BlockJob cleanup code when the job is
> deleted via job_unref(), introduce a new JobDriver.free callback. Every
> block job must use block_job_free() for this callback, this is asserted
> in block_job_create().
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/blockjob.h     | 21 -------------------
>  include/block/blockjob_int.h |  7 +++++++
>  include/qemu/job.h           | 19 ++++++++++++++++--
>  block/backup.c               |  1 +
>  block/commit.c               |  1 +
>  block/mirror.c               |  2 ++
>  block/stream.c               |  1 +
>  blockjob.c                   | 48 +++++++++++++++++++-------------------------
>  job.c                        | 22 ++++++++++++++++----
>  qemu-img.c                   |  4 ++--
>  tests/test-bdrv-drain.c      |  1 +
>  tests/test-blockjob-txn.c    |  1 +
>  tests/test-blockjob.c        |  6 ++++--
>  13 files changed, 76 insertions(+), 58 deletions(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
John Snow May 14, 2018, 9:34 p.m. UTC | #2 
On 05/09/2018 12:26 PM, Kevin Wolf wrote:
> This moves reference counting from BlockJob to Job.
> 
> In order to keep calling the BlockJob cleanup code when the job is
> deleted via job_unref(), introduce a new JobDriver.free callback. Every
> block job must use block_job_free() for this callback, this is asserted
> in block_job_create().
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

So far so good, though it does look a little silly that presently every
last job has the exact same free callback.

Also, I forgot to reply to #13 with this, but I suppose that the
approach you're taking -- of a fairly straightforward mechanical
refactor -- means we don't really have the opportunity to change any of
the pretty dumb names or existing peculiarities of design we've evolved.

I had hoped we'd be able to change a few things, but certainly keeping
them as-is makes the whole re-factoring process an awful lot simpler...

...Or maybe I'm getting ahead of myself, there's a lot of series left to go.

Ah, anyway:

Reviewed-by: John Snow <jsnow@redhat.com>
Kevin Wolf May 15, 2018, 9:06 a.m. UTC | #3 
Am 14.05.2018 um 23:34 hat John Snow geschrieben:
> 
> 
> On 05/09/2018 12:26 PM, Kevin Wolf wrote:
> > This moves reference counting from BlockJob to Job.
> > 
> > In order to keep calling the BlockJob cleanup code when the job is
> > deleted via job_unref(), introduce a new JobDriver.free callback. Every
> > block job must use block_job_free() for this callback, this is asserted
> > in block_job_create().
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> 
> So far so good, though it does look a little silly that presently every
> last job has the exact same free callback.
> 
> Also, I forgot to reply to #13 with this, but I suppose that the
> approach you're taking -- of a fairly straightforward mechanical
> refactor -- means we don't really have the opportunity to change any of
> the pretty dumb names or existing peculiarities of design we've evolved.
> 
> I had hoped we'd be able to change a few things, but certainly keeping
> them as-is makes the whole re-factoring process an awful lot simpler...
> 
> ...Or maybe I'm getting ahead of myself, there's a lot of series left
> to go.

No, I think you're right. My approach was that we'd separate the generic
and the block-specific parts in this series without changing much in the
structure or the names, to keep the conversion as obvious as possible
(the series is long enough already).

However, I intentionally kept the QMP interface minimal where I wasn't
quite sure that we want to keep the old one. If you see anything in the
QMP interface that you'd like to see changed, please do comment.
Anything else is an implementation detail and can still be changed on
top of this series.

Kevin
Eric Blake May 16, 2018, 6:17 p.m. UTC | #4 
On 05/09/2018 11:26 AM, Kevin Wolf wrote:
> This moves reference counting from BlockJob to Job.
> 
> In order to keep calling the BlockJob cleanup code when the job is
> deleted via job_unref(), introduce a new JobDriver.free callback. Every
> block job must use block_job_free() for this callback, this is asserted
> in block_job_create().
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---

> +++ b/job.c

> +
> +void job_unref(Job *job)
> +{
> +    if (--job->refcnt == 0) {

Should this be free()-like and allow an incoming job == NULL as a no-op?
Kevin Wolf May 16, 2018, 8:56 p.m. UTC | #5 
Am 16.05.2018 um 20:17 hat Eric Blake geschrieben:
> On 05/09/2018 11:26 AM, Kevin Wolf wrote:
> > This moves reference counting from BlockJob to Job.
> > 
> > In order to keep calling the BlockJob cleanup code when the job is
> > deleted via job_unref(), introduce a new JobDriver.free callback. Every
> > block job must use block_job_free() for this callback, this is asserted
> > in block_job_create().
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> 
> > +++ b/job.c
> 
> > +
> > +void job_unref(Job *job)
> > +{
> > +    if (--job->refcnt == 0) {
> 
> Should this be free()-like and allow an incoming job == NULL as a no-op?

This behaves like block_job_unref() always behavec, and I don't see a
single caller having a NULL check before calling job_unref(), so is it
worth it?

Kevin
Eric Blake May 16, 2018, 9:25 p.m. UTC | #6 
On 05/16/2018 03:56 PM, Kevin Wolf wrote:

>>> +
>>> +void job_unref(Job *job)
>>> +{
>>> +    if (--job->refcnt == 0) {
>>
>> Should this be free()-like and allow an incoming job == NULL as a no-op?
> 
> This behaves like block_job_unref() always behavec, and I don't see a
> single caller having a NULL check before calling job_unref(), so is it
> worth it?

Only if it makes it easier to clean up a partially-constructed object 
(which is the most likely case of wanting to pass in NULL)
diff mbox series

Patch

diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 96e5f67ed7..5e32e719b6 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -132,9 +132,6 @@  typedef struct BlockJob {
     /** The opaque value that is passed to the completion function.  */
     void *opaque;
 
-    /** Reference count of the block job */
-    int refcnt;
-
     /** True when job has reported completion by calling block_job_completed. */
     bool completed;
 
@@ -400,24 +397,6 @@  void block_job_iostatus_reset(BlockJob *job);
 BlockJobTxn *block_job_txn_new(void);
 
 /**
- * block_job_ref:
- *
- * Add a reference to BlockJob refcnt, it will be decreased with
- * block_job_unref, and then be freed if it comes to be the last
- * reference.
- */
-void block_job_ref(BlockJob *job);
-
-/**
- * block_job_unref:
- *
- * Release a reference that was previously acquired with block_job_ref
- * or block_job_create. If it's the last reference to the object, it will be
- * freed.
- */
-void block_job_unref(BlockJob *job);
-
-/**
  * block_job_txn_unref:
  *
  * Release a reference that was previously acquired with block_job_txn_add_job
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index 1e62d6dd30..6f0fe3c48d 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -144,6 +144,13 @@  void *block_job_create(const char *job_id, const BlockJobDriver *driver,
                        BlockCompletionFunc *cb, void *opaque, Error **errp);
 
 /**
+ * block_job_free:
+ * Callback to be used for JobDriver.free in all block jobs. Frees block job
+ * specific resources in @job.
+ */
+void block_job_free(Job *job);
+
+/**
  * block_job_sleep_ns:
  * @job: The job that calls the function.
  * @ns: How many nanoseconds to stop for.
diff --git a/include/qemu/job.h b/include/qemu/job.h
index 48a7dfc431..64e988f305 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -41,6 +41,9 @@  typedef struct Job {
     /** The type of this job. */
     const JobDriver *driver;
 
+    /** Reference count of the block job */
+    int refcnt;
+
     /** Current state; See @JobStatus for details. */
     JobStatus status;
 
@@ -57,6 +60,9 @@  struct JobDriver {
 
     /** Enum describing the operation */
     JobType job_type;
+
+    /** Called when the job is freed */
+    void (*free)(Job *job);
 };
 
 
@@ -69,8 +75,17 @@  struct JobDriver {
  */
 void *job_create(const char *job_id, const JobDriver *driver, Error **errp);
 
-/** Frees the @job object. */
-void job_delete(Job *job);
+/**
+ * Add a reference to Job refcnt, it will be decreased with job_unref, and then
+ * be freed if it comes to be the last reference.
+ */
+void job_ref(Job *job);
+
+/**
+ * Release a reference that was previously acquired with job_ref() or
+ * job_create(). If it's the last reference to the object, it will be freed.
+ */
+void job_unref(Job *job);
 
 /** Returns the JobType of a given Job. */
 JobType job_type(Job *job);
diff --git a/block/backup.c b/block/backup.c
index baf8d432da..cfdb89d977 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -526,6 +526,7 @@  static const BlockJobDriver backup_job_driver = {
     .job_driver = {
         .instance_size          = sizeof(BackupBlockJob),
         .job_type               = JOB_TYPE_BACKUP,
+        .free                   = block_job_free,
     },
     .start                  = backup_run,
     .commit                 = backup_commit,
diff --git a/block/commit.c b/block/commit.c
index 32d29c890e..925c96abe7 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -218,6 +218,7 @@  static const BlockJobDriver commit_job_driver = {
     .job_driver = {
         .instance_size = sizeof(CommitBlockJob),
         .job_type      = JOB_TYPE_COMMIT,
+        .free          = block_job_free,
     },
     .start         = commit_run,
 };
diff --git a/block/mirror.c b/block/mirror.c
index 054972bdc1..9b1e5c0849 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -989,6 +989,7 @@  static const BlockJobDriver mirror_job_driver = {
     .job_driver = {
         .instance_size          = sizeof(MirrorBlockJob),
         .job_type               = JOB_TYPE_MIRROR,
+        .free                   = block_job_free,
     },
     .start                  = mirror_run,
     .complete               = mirror_complete,
@@ -1001,6 +1002,7 @@  static const BlockJobDriver commit_active_job_driver = {
     .job_driver = {
         .instance_size          = sizeof(MirrorBlockJob),
         .job_type               = JOB_TYPE_COMMIT,
+        .free                   = block_job_free,
     },
     .start                  = mirror_run,
     .complete               = mirror_complete,
diff --git a/block/stream.c b/block/stream.c
index cb723f190a..7273d2213c 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -212,6 +212,7 @@  static const BlockJobDriver stream_job_driver = {
     .job_driver = {
         .instance_size = sizeof(StreamBlockJob),
         .job_type      = JOB_TYPE_STREAM,
+        .free          = block_job_free,
     },
     .start         = stream_run,
 };
diff --git a/blockjob.c b/blockjob.c
index 408a7159f6..1fa5173bb9 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -177,31 +177,25 @@  static void block_job_resume(BlockJob *job)
     block_job_enter(job);
 }
 
-void block_job_ref(BlockJob *job)
-{
-    ++job->refcnt;
-}
-
 static void block_job_attached_aio_context(AioContext *new_context,
                                            void *opaque);
 static void block_job_detach_aio_context(void *opaque);
 
-void block_job_unref(BlockJob *job)
+void block_job_free(Job *job)
 {
-    if (--job->refcnt == 0) {
-        assert(job->job.status == JOB_STATUS_NULL);
-        assert(!job->txn);
-        BlockDriverState *bs = blk_bs(job->blk);
-        bs->job = NULL;
-        block_job_remove_all_bdrv(job);
-        blk_remove_aio_context_notifier(job->blk,
-                                        block_job_attached_aio_context,
-                                        block_job_detach_aio_context, job);
-        blk_unref(job->blk);
-        error_free(job->blocker);
-        assert(!timer_pending(&job->sleep_timer));
-        job_delete(&job->job);
-    }
+    BlockJob *bjob = container_of(job, BlockJob, job);
+    BlockDriverState *bs = blk_bs(bjob->blk);
+
+    assert(!bjob->txn);
+
+    bs->job = NULL;
+    block_job_remove_all_bdrv(bjob);
+    blk_remove_aio_context_notifier(bjob->blk,
+                                    block_job_attached_aio_context,
+                                    block_job_detach_aio_context, bjob);
+    blk_unref(bjob->blk);
+    error_free(bjob->blocker);
+    assert(!timer_pending(&bjob->sleep_timer));
 }
 
 static void block_job_attached_aio_context(AioContext *new_context,
@@ -232,7 +226,7 @@  static void block_job_detach_aio_context(void *opaque)
     BlockJob *job = opaque;
 
     /* In case the job terminates during aio_poll()... */
-    block_job_ref(job);
+    job_ref(&job->job);
 
     block_job_pause(job);
 
@@ -240,7 +234,7 @@  static void block_job_detach_aio_context(void *opaque)
         block_job_drain(job);
     }
 
-    block_job_unref(job);
+    job_unref(&job->job);
 }
 
 static char *child_job_get_parent_desc(BdrvChild *c)
@@ -354,7 +348,7 @@  static void block_job_decommission(BlockJob *job)
     job->deferred_to_main_loop = true;
     block_job_txn_del_job(job);
     job_state_transition(&job->job, JOB_STATUS_NULL);
-    block_job_unref(job);
+    job_unref(&job->job);
 }
 
 static void block_job_do_dismiss(BlockJob *job)
@@ -493,14 +487,14 @@  static int block_job_finish_sync(BlockJob *job,
 
     assert(blk_bs(job->blk)->job == job);
 
-    block_job_ref(job);
+    job_ref(&job->job);
 
     if (finish) {
         finish(job, &local_err);
     }
     if (local_err) {
         error_propagate(errp, local_err);
-        block_job_unref(job);
+        job_unref(&job->job);
         return -EBUSY;
     }
     /* block_job_drain calls block_job_enter, and it should be enough to
@@ -513,7 +507,7 @@  static int block_job_finish_sync(BlockJob *job,
         aio_poll(qemu_get_aio_context(), true);
     }
     ret = (job->cancelled && job->ret == 0) ? -ECANCELED : job->ret;
-    block_job_unref(job);
+    job_unref(&job->job);
     return ret;
 }
 
@@ -902,6 +896,7 @@  void *block_job_create(const char *job_id, const BlockJobDriver *driver,
     }
 
     assert(is_block_job(&job->job));
+    assert(job->job.driver->free == &block_job_free);
 
     job->driver        = driver;
     job->blk           = blk;
@@ -910,7 +905,6 @@  void *block_job_create(const char *job_id, const BlockJobDriver *driver,
     job->busy          = false;
     job->paused        = true;
     job->pause_count   = 1;
-    job->refcnt        = 1;
     job->auto_finalize = !(flags & BLOCK_JOB_MANUAL_FINALIZE);
     job->auto_dismiss  = !(flags & BLOCK_JOB_MANUAL_DISMISS);
     aio_timer_init(qemu_get_aio_context(), &job->sleep_timer,
diff --git a/job.c b/job.c
index 93e846f7e2..67b09cb54d 100644
--- a/job.c
+++ b/job.c
@@ -134,6 +134,7 @@  void *job_create(const char *job_id, const JobDriver *driver, Error **errp)
     job = g_malloc0(driver->instance_size);
     job->driver        = driver;
     job->id            = g_strdup(job_id);
+    job->refcnt        = 1;
 
     job_state_transition(job, JOB_STATUS_CREATED);
 
@@ -142,10 +143,23 @@  void *job_create(const char *job_id, const JobDriver *driver, Error **errp)
     return job;
 }
 
-void job_delete(Job *job)
+void job_ref(Job *job)
 {
-    QLIST_REMOVE(job, job_list);
+    ++job->refcnt;
+}
+
+void job_unref(Job *job)
+{
+    if (--job->refcnt == 0) {
+        assert(job->status == JOB_STATUS_NULL);
 
-    g_free(job->id);
-    g_free(job);
+        if (job->driver->free) {
+            job->driver->free(job);
+        }
+
+        QLIST_REMOVE(job, job_list);
+
+        g_free(job->id);
+        g_free(job);
+    }
 }
diff --git a/qemu-img.c b/qemu-img.c
index ea62d2d61e..82f69269ae 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -862,7 +862,7 @@  static void run_block_job(BlockJob *job, Error **errp)
     int ret = 0;
 
     aio_context_acquire(aio_context);
-    block_job_ref(job);
+    job_ref(&job->job);
     do {
         aio_poll(aio_context, true);
         qemu_progress_print(job->len ?
@@ -874,7 +874,7 @@  static void run_block_job(BlockJob *job, Error **errp)
     } else {
         ret = job->ret;
     }
-    block_job_unref(job);
+    job_unref(&job->job);
     aio_context_release(aio_context);
 
     /* publish completion progress only when success */
diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
index fe9f412b39..f9e37d479c 100644
--- a/tests/test-bdrv-drain.c
+++ b/tests/test-bdrv-drain.c
@@ -522,6 +522,7 @@  static void test_job_complete(BlockJob *job, Error **errp)
 BlockJobDriver test_job_driver = {
     .job_driver = {
         .instance_size  = sizeof(TestBlockJob),
+        .free           = block_job_free,
     },
     .start          = test_job_start,
     .complete       = test_job_complete,
diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
index 48b12d1744..b49b28ca27 100644
--- a/tests/test-blockjob-txn.c
+++ b/tests/test-blockjob-txn.c
@@ -76,6 +76,7 @@  static void test_block_job_cb(void *opaque, int ret)
 static const BlockJobDriver test_block_job_driver = {
     .job_driver = {
         .instance_size = sizeof(TestBlockJob),
+        .free          = block_job_free,
     },
     .start = test_block_job_run,
 };
diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
index 6ccd585dee..e24fc3f140 100644
--- a/tests/test-blockjob.c
+++ b/tests/test-blockjob.c
@@ -19,6 +19,7 @@ 
 static const BlockJobDriver test_block_job_driver = {
     .job_driver = {
         .instance_size = sizeof(BlockJob),
+        .free          = block_job_free,
     },
 };
 
@@ -196,6 +197,7 @@  static void coroutine_fn cancel_job_start(void *opaque)
 static const BlockJobDriver test_cancel_driver = {
     .job_driver = {
         .instance_size = sizeof(CancelJob),
+        .free          = block_job_free,
     },
     .start         = cancel_job_start,
     .complete      = cancel_job_complete,
@@ -210,7 +212,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);
-    block_job_ref(job);
+    job_ref(&job->job);
     assert(job->job.status == JOB_STATUS_CREATED);
     s = container_of(job, CancelJob, common);
     s->blk = blk;
@@ -231,7 +233,7 @@  static void cancel_common(CancelJob *s)
         block_job_dismiss(&dummy, &error_abort);
     }
     assert(job->job.status == JOB_STATUS_NULL);
-    block_job_unref(job);
+    job_unref(&job->job);
     destroy_blk(blk);
 }