From 3f7eb910b9ce698ebc33a039e6bbef1df1437072 Mon Sep 17 00:00:00 2001

From: Kevin Wolf <kwolf@redhat.com>

Date: Tue, 26 Jun 2018 09:48:05 +0200

Subject: [PATCH 36/89] job: Add reference counting

RH-Author: Kevin Wolf <kwolf@redhat.com>

Message-id: <20180626094856.6924-23-kwolf@redhat.com>

Patchwork-id: 81073

O-Subject: [RHV-7.6 qemu-kvm-rhev PATCH v2 22/73] job: Add reference counting

Bugzilla: 1513543

RH-Acked-by: Jeffrey Cody <jcody@redhat.com>

RH-Acked-by: Max Reitz <mreitz@redhat.com>

RH-Acked-by: Fam Zheng <famz@redhat.com>

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>

Reviewed-by: Max Reitz <mreitz@redhat.com>

Reviewed-by: John Snow <jsnow@redhat.com>

(cherry picked from commit 80fa2c756b3241f24015a7503a01f7999d4a942d)

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>

---

 block/backup.c               |  1 +

 block/commit.c               |  1 +

 block/mirror.c               |  2 ++

 block/stream.c               |  1 +

 blockjob.c                   | 48 +++++++++++++++++++-------------------------

 include/block/blockjob.h     | 21 -------------------

 include/block/blockjob_int.h |  7 +++++++

 include/qemu/job.h           | 19 ++++++++++++++++--

 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(-)

diff --git a/block/backup.c b/block/backup.c

index baf8d43..cfdb89d 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 32d29c8..925c96a 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 35fcc1f..0df4f70 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 cb723f1..7273d22 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 0fba01e..0bf0a26 100644

--- a/blockjob.c

+++ b/blockjob.c

@@ -190,31 +190,25 @@ static void block_job_resume(BlockJob *job)

     block_job_enter_cond(job, block_job_timer_not_pending);

 }

-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,

@@ -245,7 +239,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);

@@ -253,7 +247,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)

@@ -367,7 +361,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)

@@ -506,14 +500,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

@@ -526,7 +520,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;

 }

@@ -909,6 +903,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;

@@ -917,7 +912,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/include/block/blockjob.h b/include/block/blockjob.h

index 01cdee6..087e782 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 1e62d6d..6f0fe3c 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 0b78778..0751e2a 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(const Job *job);

diff --git a/job.c b/job.c

index b049a32..926f1de 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 e2395b9..f745919 100644

--- a/qemu-img.c

+++ b/qemu-img.c

@@ -873,7 +873,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 ?

@@ -885,7 +885,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 fe9f412..f9e37d4 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 48b12d1..b49b28c 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 6ccd585..e24fc3f 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);

 }

-- 

1.8.3.1