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