From 2f7ae3f2886ec34dfb0a51e75d0f62998213928a Mon Sep 17 00:00:00 2001
From: Kevin Wolf <kwolf@redhat.com>
Date: Tue, 26 Jun 2018 09:48:05 +0200
Subject: [PATCH 097/268] 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