From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45401) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aODS2-0003WN-CB for qemu-devel@nongnu.org; Tue, 26 Jan 2016 18:55:12 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aODS0-0001cq-Mk for qemu-devel@nongnu.org; Tue, 26 Jan 2016 18:55:10 -0500 From: John Snow Date: Tue, 26 Jan 2016 18:54:55 -0500 Message-Id: <1453852499-25800-2-git-send-email-jsnow@redhat.com> In-Reply-To: <1453852499-25800-1-git-send-email-jsnow@redhat.com> References: <1453852499-25800-1-git-send-email-jsnow@redhat.com> Subject: [Qemu-devel] [PATCH v2 1/5] block: Allow mirror_start to return job references List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-block@nongnu.org Cc: kwolf@redhat.com, jcody@redhat.com, John Snow , armbru@redhat.com, qemu-devel@nongnu.org Pick up an extra reference in mirror_start_job to allow callers of mirror_start and commit_active_start to get a reference to the job they have created. Phase out callers from fishing the job out of bs->job -- use the return value instead. Callers of mirror_start_job and commit_active_start are now responsible for putting down their reference to the job. No callers of mirror_start yet seem to need the reference, so that's left as a void return for now. Ultimately, this patch fixes qemu-img's reliance on bs->job. Reviewed-by: Kevin Wolf Signed-off-by: John Snow --- block/mirror.c | 72 ++++++++++++++++++++++++++--------------------- blockdev.c | 8 ++++-- include/block/block_int.h | 10 +++---- qemu-img.c | 12 +++++--- 4 files changed, 59 insertions(+), 43 deletions(-) diff --git a/block/mirror.c b/block/mirror.c index e9e151c..b193e36 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -707,17 +707,18 @@ static const BlockJobDriver commit_active_job_driver = { .complete = mirror_complete, }; -static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target, - const char *replaces, - int64_t speed, uint32_t granularity, - int64_t buf_size, - BlockdevOnError on_source_error, - BlockdevOnError on_target_error, - bool unmap, - BlockCompletionFunc *cb, - void *opaque, Error **errp, - const BlockJobDriver *driver, - bool is_none_mode, BlockDriverState *base) +static BlockJob *mirror_start_job(BlockDriverState *bs, + BlockDriverState *target, + const char *replaces, + int64_t speed, uint32_t granularity, + int64_t buf_size, + BlockdevOnError on_source_error, + BlockdevOnError on_target_error, + bool unmap, + BlockCompletionFunc *cb, + void *opaque, Error **errp, + const BlockJobDriver *driver, + bool is_none_mode, BlockDriverState *base) { MirrorBlockJob *s; BlockDriverState *replaced_bs; @@ -732,12 +733,12 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target, on_source_error == BLOCKDEV_ON_ERROR_ENOSPC) && (!bs->blk || !blk_iostatus_is_enabled(bs->blk))) { error_setg(errp, QERR_INVALID_PARAMETER, "on-source-error"); - return; + return NULL; } if (buf_size < 0) { error_setg(errp, "Invalid parameter 'buf-size'"); - return; + return NULL; } if (buf_size == 0) { @@ -749,19 +750,19 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target, if (replaces) { replaced_bs = bdrv_lookup_bs(replaces, replaces, errp); if (replaced_bs == NULL) { - return; + return NULL; } } else { replaced_bs = bs; } if (replaced_bs->blk && target->blk) { error_setg(errp, "Can't create node with two BlockBackends"); - return; + return NULL; } s = block_job_create(driver, bs, speed, cb, opaque, errp); if (!s) { - return; + return NULL; } s->replaces = g_strdup(replaces); @@ -778,7 +779,7 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target, if (!s->dirty_bitmap) { g_free(s->replaces); block_job_unref(&s->common); - return; + return NULL; } bdrv_op_block_all(s->target, s->common.blocker); @@ -788,9 +789,11 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target, blk_set_on_error(s->target->blk, on_target_error, on_target_error); blk_iostatus_enable(s->target->blk); } + block_job_ref(&s->common); s->common.co = qemu_coroutine_create(mirror_run); trace_mirror_start(bs, s, s->common.co, opaque); qemu_coroutine_enter(s->common.co, s); + return &s->common; } void mirror_start(BlockDriverState *bs, BlockDriverState *target, @@ -804,6 +807,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target, { bool is_none_mode; BlockDriverState *base; + BlockJob *job; if (mode == MIRROR_SYNC_MODE_INCREMENTAL) { error_setg(errp, "Sync mode 'incremental' not supported"); @@ -811,27 +815,31 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target, } is_none_mode = mode == MIRROR_SYNC_MODE_NONE; base = mode == MIRROR_SYNC_MODE_TOP ? backing_bs(bs) : NULL; - mirror_start_job(bs, target, replaces, - speed, granularity, buf_size, - on_source_error, on_target_error, unmap, cb, opaque, errp, - &mirror_job_driver, is_none_mode, base); + job = mirror_start_job(bs, target, replaces, + speed, granularity, buf_size, + on_source_error, on_target_error, unmap, cb, opaque, + errp, &mirror_job_driver, is_none_mode, base); + if (job) { + block_job_unref(job); + } } -void commit_active_start(BlockDriverState *bs, BlockDriverState *base, - int64_t speed, - BlockdevOnError on_error, - BlockCompletionFunc *cb, - void *opaque, Error **errp) +BlockJob *commit_active_start(BlockDriverState *bs, BlockDriverState *base, + int64_t speed, + BlockdevOnError on_error, + BlockCompletionFunc *cb, + void *opaque, Error **errp) { int64_t length, base_length; int orig_base_flags; int ret; Error *local_err = NULL; + BlockJob *job; orig_base_flags = bdrv_get_flags(base); if (bdrv_reopen(base, bs->open_flags, errp)) { - return; + return NULL; } length = bdrv_getlength(bs); @@ -860,19 +868,19 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base, } bdrv_ref(base); - mirror_start_job(bs, base, NULL, speed, 0, 0, - on_error, on_error, false, cb, opaque, &local_err, - &commit_active_job_driver, false, base); + job = mirror_start_job(bs, base, NULL, speed, 0, 0, + on_error, on_error, false, cb, opaque, &local_err, + &commit_active_job_driver, false, base); if (local_err) { error_propagate(errp, local_err); goto error_restore_flags; } - return; + return job; error_restore_flags: /* ignore error and errp for bdrv_reopen, because we want to propagate * the original error */ bdrv_reopen(base, orig_base_flags, NULL); - return; + return NULL; } diff --git a/blockdev.c b/blockdev.c index 07cfe25..c0b9b12 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2955,6 +2955,7 @@ void qmp_block_commit(const char *device, BlockBackend *blk; BlockDriverState *bs; BlockDriverState *base_bs, *top_bs; + BlockJob *job = NULL; AioContext *aio_context; Error *local_err = NULL; /* This will be part of the QMP command, if/when the @@ -3036,8 +3037,8 @@ void qmp_block_commit(const char *device, " but 'top' is the active layer"); goto out; } - commit_active_start(bs, base_bs, speed, on_error, block_job_cb, - bs, &local_err); + job = commit_active_start(bs, base_bs, speed, on_error, block_job_cb, + bs, &local_err); } else { commit_start(bs, base_bs, top_bs, speed, on_error, block_job_cb, bs, has_backing_file ? backing_file : NULL, &local_err); @@ -3048,6 +3049,9 @@ void qmp_block_commit(const char *device, } out: + if (job) { + block_job_unref(job); + } aio_context_release(aio_context); } diff --git a/include/block/block_int.h b/include/block/block_int.h index 428fa33..de4c3c6 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -631,11 +631,11 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base, * @errp: Error object. * */ -void commit_active_start(BlockDriverState *bs, BlockDriverState *base, - int64_t speed, - BlockdevOnError on_error, - BlockCompletionFunc *cb, - void *opaque, Error **errp); +BlockJob *commit_active_start(BlockDriverState *bs, BlockDriverState *base, + int64_t speed, + BlockdevOnError on_error, + BlockCompletionFunc *cb, + void *opaque, Error **errp); /* * mirror_start: * @bs: Block device to operate on. diff --git a/qemu-img.c b/qemu-img.c index 33e451c..8d11708 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -644,8 +644,9 @@ static void common_block_job_cb(void *opaque, int ret) } } -static void run_block_job(BlockJob *job, Error **errp) +static void run_block_job(BlockJob **pjob, Error **errp) { + BlockJob *job = *pjob; AioContext *aio_context = bdrv_get_aio_context(job->bs); do { @@ -659,6 +660,8 @@ static void run_block_job(BlockJob *job, Error **errp) /* A block job may finish instantaneously without publishing any progress, * so just signal completion here */ qemu_progress_print(100.f, 0); + block_job_unref(job); + *pjob = NULL; } static int img_commit(int argc, char **argv) @@ -667,6 +670,7 @@ static int img_commit(int argc, char **argv) const char *filename, *fmt, *cache, *base; BlockBackend *blk; BlockDriverState *bs, *base_bs; + BlockJob *job; bool progress = false, quiet = false, drop = false; Error *local_err = NULL; CommonBlockJobCBInfo cbi; @@ -755,8 +759,8 @@ static int img_commit(int argc, char **argv) .bs = bs, }; - commit_active_start(bs, base_bs, 0, BLOCKDEV_ON_ERROR_REPORT, - common_block_job_cb, &cbi, &local_err); + job = commit_active_start(bs, base_bs, 0, BLOCKDEV_ON_ERROR_REPORT, + common_block_job_cb, &cbi, &local_err); if (local_err) { goto done; } @@ -769,7 +773,7 @@ static int img_commit(int argc, char **argv) bdrv_ref(bs); } - run_block_job(bs->job, &local_err); + run_block_job(&job, &local_err); if (local_err) { goto unref_backing; } -- 2.4.3