* [Qemu-devel] [PATCH v2 1/5] block: Allow mirror_start to return job references
2016-01-26 23:54 [Qemu-devel] [PATCH v2 0/5] block: reduce reliance on bs->job pointer John Snow
@ 2016-01-26 23:54 ` John Snow
2016-01-26 23:54 ` [Qemu-devel] [PATCH v2 2/5] block: Allow stream_start " John Snow
` (4 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: John Snow @ 2016-01-26 23:54 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, jcody, John Snow, armbru, qemu-devel
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 <kwolf@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [Qemu-devel] [PATCH v2 2/5] block: Allow stream_start to return job references
2016-01-26 23:54 [Qemu-devel] [PATCH v2 0/5] block: reduce reliance on bs->job pointer John Snow
2016-01-26 23:54 ` [Qemu-devel] [PATCH v2 1/5] block: Allow mirror_start to return job references John Snow
@ 2016-01-26 23:54 ` John Snow
2016-01-26 23:54 ` [Qemu-devel] [PATCH v2 3/5] block: allow backup_start " John Snow
` (3 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: John Snow @ 2016-01-26 23:54 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, jcody, John Snow, armbru, qemu-devel
stream_start now picks up a reference for its return value, a copy of
the job started. callers are responsible for putting it down when they
are done with it.
This removes a minor reference to bs->job in qmp_block_stream, for
a simple tracing function.
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
block/stream.c | 8 +++++---
blockdev.c | 8 +++++---
include/block/block_int.h | 9 +++++----
3 files changed, 15 insertions(+), 10 deletions(-)
diff --git a/block/stream.c b/block/stream.c
index cafaa07..26a2990 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -214,7 +214,7 @@ static const BlockJobDriver stream_job_driver = {
.set_speed = stream_set_speed,
};
-void stream_start(BlockDriverState *bs, BlockDriverState *base,
+BlockJob *stream_start(BlockDriverState *bs, BlockDriverState *base,
const char *backing_file_str, int64_t speed,
BlockdevOnError on_error,
BlockCompletionFunc *cb,
@@ -226,19 +226,21 @@ void stream_start(BlockDriverState *bs, BlockDriverState *base,
on_error == BLOCKDEV_ON_ERROR_ENOSPC) &&
(!bs->blk || !blk_iostatus_is_enabled(bs->blk))) {
error_setg(errp, QERR_INVALID_PARAMETER, "on-error");
- return;
+ return NULL;
}
s = block_job_create(&stream_job_driver, bs, speed, cb, opaque, errp);
if (!s) {
- return;
+ return NULL;
}
s->base = base;
s->backing_file_str = g_strdup(backing_file_str);
s->on_error = on_error;
+ block_job_ref(&s->common);
s->common.co = qemu_coroutine_create(stream_run);
trace_stream_start(bs, base, s, s->common.co, opaque);
qemu_coroutine_enter(s->common.co, s);
+ return &s->common;
}
diff --git a/blockdev.c b/blockdev.c
index c0b9b12..5a74c81 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2883,6 +2883,7 @@ void qmp_block_stream(const char *device,
BlockBackend *blk;
BlockDriverState *bs;
BlockDriverState *base_bs = NULL;
+ BlockJob *job;
AioContext *aio_context;
Error *local_err = NULL;
const char *base_name = NULL;
@@ -2932,14 +2933,15 @@ void qmp_block_stream(const char *device,
/* backing_file string overrides base bs filename */
base_name = has_backing_file ? backing_file : base_name;
- stream_start(bs, base_bs, base_name, has_speed ? speed : 0,
- on_error, block_job_cb, bs, &local_err);
+ job = stream_start(bs, base_bs, base_name, has_speed ? speed : 0,
+ on_error, block_job_cb, bs, &local_err);
if (local_err) {
error_propagate(errp, local_err);
goto out;
}
- trace_qmp_block_stream(bs, bs->job);
+ trace_qmp_block_stream(bs, job);
+ block_job_unref(job);
out:
aio_context_release(aio_context);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index de4c3c6..7a86d7f 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -598,10 +598,11 @@ int is_windows_drive(const char *filename);
* streaming job, the backing file of @bs will be changed to
* @base_id in the written image and to @base in the live BlockDriverState.
*/
-void stream_start(BlockDriverState *bs, BlockDriverState *base,
- const char *base_id, int64_t speed, BlockdevOnError on_error,
- BlockCompletionFunc *cb,
- void *opaque, Error **errp);
+BlockJob *stream_start(BlockDriverState *bs, BlockDriverState *base,
+ const char *base_id, int64_t speed,
+ BlockdevOnError on_error,
+ BlockCompletionFunc *cb,
+ void *opaque, Error **errp);
/**
* commit_start:
--
2.4.3
^ permalink raw reply related [flat|nested] 9+ messages in thread* [Qemu-devel] [PATCH v2 3/5] block: allow backup_start to return job references
2016-01-26 23:54 [Qemu-devel] [PATCH v2 0/5] block: reduce reliance on bs->job pointer John Snow
2016-01-26 23:54 ` [Qemu-devel] [PATCH v2 1/5] block: Allow mirror_start to return job references John Snow
2016-01-26 23:54 ` [Qemu-devel] [PATCH v2 2/5] block: Allow stream_start " John Snow
@ 2016-01-26 23:54 ` John Snow
2016-01-26 23:54 ` [Qemu-devel] [PATCH v2 4/5] block/backup: Pack Notifier within BackupBlockJob John Snow
` (2 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: John Snow @ 2016-01-26 23:54 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, jcody, John Snow, armbru, qemu-devel
backup_start picks up a reference to return the job it created to a
caller. callers are updated to put down the reference when they are
finished.
This is particularly interesting for transactions where backup jobs
pick up an implicit reference to the job. Previously, we check to
see if the job still exists by seeing if (bs->job == state->job),
but now we can be assured that our job object is still valid.
The job of course may have been canceled already, though.
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
block/backup.c | 38 +++++-----
blockdev.c | 180 +++++++++++++++++++++++++---------------------
include/block/block_int.h | 2 +-
3 files changed, 120 insertions(+), 100 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 00cafdb..ce96b45 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -485,13 +485,13 @@ static void coroutine_fn backup_run(void *opaque)
block_job_defer_to_main_loop(&job->common, backup_complete, data);
}
-void backup_start(BlockDriverState *bs, BlockDriverState *target,
- int64_t speed, MirrorSyncMode sync_mode,
- BdrvDirtyBitmap *sync_bitmap,
- BlockdevOnError on_source_error,
- BlockdevOnError on_target_error,
- BlockCompletionFunc *cb, void *opaque,
- BlockJobTxn *txn, Error **errp)
+BlockJob *backup_start(BlockDriverState *bs, BlockDriverState *target,
+ int64_t speed, MirrorSyncMode sync_mode,
+ BdrvDirtyBitmap *sync_bitmap,
+ BlockdevOnError on_source_error,
+ BlockdevOnError on_target_error,
+ BlockCompletionFunc *cb, void *opaque,
+ BlockJobTxn *txn, Error **errp)
{
int64_t len;
@@ -501,53 +501,53 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
if (bs == target) {
error_setg(errp, "Source and target cannot be the same");
- return;
+ return NULL;
}
if ((on_source_error == BLOCKDEV_ON_ERROR_STOP ||
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 (!bdrv_is_inserted(bs)) {
error_setg(errp, "Device is not inserted: %s",
bdrv_get_device_name(bs));
- return;
+ return NULL;
}
if (!bdrv_is_inserted(target)) {
error_setg(errp, "Device is not inserted: %s",
bdrv_get_device_name(target));
- return;
+ return NULL;
}
if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) {
- return;
+ return NULL;
}
if (bdrv_op_is_blocked(target, BLOCK_OP_TYPE_BACKUP_TARGET, errp)) {
- return;
+ return NULL;
}
if (sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) {
if (!sync_bitmap) {
error_setg(errp, "must provide a valid bitmap name for "
"\"incremental\" sync mode");
- return;
+ return NULL;
}
/* Create a new bitmap, and freeze/disable this one. */
if (bdrv_dirty_bitmap_create_successor(bs, sync_bitmap, errp) < 0) {
- return;
+ return NULL;
}
} else if (sync_bitmap) {
error_setg(errp,
"a sync_bitmap was provided to backup_run, "
"but received an incompatible sync_mode (%s)",
MirrorSyncMode_lookup[sync_mode]);
- return;
+ return NULL;
}
len = bdrv_getlength(bs);
@@ -572,13 +572,17 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
job->sync_bitmap = sync_mode == MIRROR_SYNC_MODE_INCREMENTAL ?
sync_bitmap : NULL;
job->common.len = len;
+
+ block_job_ref(&job->common);
job->common.co = qemu_coroutine_create(backup_run);
block_job_txn_add_job(txn, &job->common);
qemu_coroutine_enter(job->common.co, job);
- return;
+ return &job->common;
error:
if (sync_bitmap) {
bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL);
}
+
+ return NULL;
}
diff --git a/blockdev.c b/blockdev.c
index 5a74c81..ad46aa8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1805,17 +1805,17 @@ typedef struct DriveBackupState {
BlockJob *job;
} DriveBackupState;
-static void do_drive_backup(const char *device, const char *target,
- bool has_format, const char *format,
- enum MirrorSyncMode sync,
- bool has_mode, enum NewImageMode mode,
- bool has_speed, int64_t speed,
- bool has_bitmap, const char *bitmap,
- bool has_on_source_error,
- BlockdevOnError on_source_error,
- bool has_on_target_error,
- BlockdevOnError on_target_error,
- BlockJobTxn *txn, Error **errp);
+static BlockJob *do_drive_backup(const char *device, const char *target,
+ bool has_format, const char *format,
+ enum MirrorSyncMode sync,
+ bool has_mode, enum NewImageMode mode,
+ bool has_speed, int64_t speed,
+ bool has_bitmap, const char *bitmap,
+ bool has_on_source_error,
+ BlockdevOnError on_source_error,
+ bool has_on_target_error,
+ BlockdevOnError on_target_error,
+ BlockJobTxn *txn, Error **errp);
static void drive_backup_prepare(BlkActionState *common, Error **errp)
{
@@ -1845,31 +1845,29 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
bdrv_drained_begin(blk_bs(blk));
state->bs = blk_bs(blk);
- do_drive_backup(backup->device, backup->target,
- backup->has_format, backup->format,
- backup->sync,
- backup->has_mode, backup->mode,
- backup->has_speed, backup->speed,
- backup->has_bitmap, backup->bitmap,
- backup->has_on_source_error, backup->on_source_error,
- backup->has_on_target_error, backup->on_target_error,
- common->block_job_txn, &local_err);
+ state->job = do_drive_backup(backup->device, backup->target,
+ backup->has_format, backup->format,
+ backup->sync,
+ backup->has_mode, backup->mode,
+ backup->has_speed, backup->speed,
+ backup->has_bitmap, backup->bitmap,
+ backup->has_on_source_error,
+ backup->on_source_error,
+ backup->has_on_target_error,
+ backup->on_target_error,
+ common->block_job_txn, &local_err);
if (local_err) {
error_propagate(errp, local_err);
return;
}
-
- state->job = state->bs->job;
}
static void drive_backup_abort(BlkActionState *common)
{
DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
- BlockDriverState *bs = state->bs;
- /* Only cancel if it's the job we started */
- if (bs && bs->job && bs->job == state->job) {
- block_job_cancel_sync(bs->job);
+ if (state->job) {
+ block_job_cancel_sync(state->job);
}
}
@@ -1881,6 +1879,11 @@ static void drive_backup_clean(BlkActionState *common)
bdrv_drained_end(state->bs);
aio_context_release(state->aio_context);
}
+
+ if (state->job) {
+ block_job_unref(state->job);
+ state->job = NULL;
+ }
}
typedef struct BlockdevBackupState {
@@ -1890,14 +1893,14 @@ typedef struct BlockdevBackupState {
AioContext *aio_context;
} BlockdevBackupState;
-static void do_blockdev_backup(const char *device, const char *target,
- enum MirrorSyncMode sync,
- bool has_speed, int64_t speed,
- bool has_on_source_error,
- BlockdevOnError on_source_error,
- bool has_on_target_error,
- BlockdevOnError on_target_error,
- BlockJobTxn *txn, Error **errp);
+static BlockJob *do_blockdev_backup(const char *device, const char *target,
+ enum MirrorSyncMode sync,
+ bool has_speed, int64_t speed,
+ bool has_on_source_error,
+ BlockdevOnError on_source_error,
+ bool has_on_target_error,
+ BlockdevOnError on_target_error,
+ BlockJobTxn *txn, Error **errp);
static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
{
@@ -1937,28 +1940,26 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
state->bs = blk_bs(blk);
bdrv_drained_begin(state->bs);
- do_blockdev_backup(backup->device, backup->target,
- backup->sync,
- backup->has_speed, backup->speed,
- backup->has_on_source_error, backup->on_source_error,
- backup->has_on_target_error, backup->on_target_error,
- common->block_job_txn, &local_err);
+ state->job = do_blockdev_backup(backup->device, backup->target,
+ backup->sync,
+ backup->has_speed, backup->speed,
+ backup->has_on_source_error,
+ backup->on_source_error,
+ backup->has_on_target_error,
+ backup->on_target_error,
+ common->block_job_txn, &local_err);
if (local_err) {
error_propagate(errp, local_err);
return;
}
-
- state->job = state->bs->job;
}
static void blockdev_backup_abort(BlkActionState *common)
{
BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common);
- BlockDriverState *bs = state->bs;
- /* Only cancel if it's the job we started */
- if (bs && bs->job && bs->job == state->job) {
- block_job_cancel_sync(bs->job);
+ if (state->job) {
+ block_job_cancel_sync(state->job);
}
}
@@ -1970,6 +1971,11 @@ static void blockdev_backup_clean(BlkActionState *common)
bdrv_drained_end(state->bs);
aio_context_release(state->aio_context);
}
+
+ if (state->job) {
+ block_job_unref(state->job);
+ state->job = NULL;
+ }
}
typedef struct BlockDirtyBitmapState {
@@ -3057,22 +3063,23 @@ out:
aio_context_release(aio_context);
}
-static void do_drive_backup(const char *device, const char *target,
- bool has_format, const char *format,
- enum MirrorSyncMode sync,
- bool has_mode, enum NewImageMode mode,
- bool has_speed, int64_t speed,
- bool has_bitmap, const char *bitmap,
- bool has_on_source_error,
- BlockdevOnError on_source_error,
- bool has_on_target_error,
- BlockdevOnError on_target_error,
- BlockJobTxn *txn, Error **errp)
+static BlockJob *do_drive_backup(const char *device, const char *target,
+ bool has_format, const char *format,
+ enum MirrorSyncMode sync,
+ bool has_mode, enum NewImageMode mode,
+ bool has_speed, int64_t speed,
+ bool has_bitmap, const char *bitmap,
+ bool has_on_source_error,
+ BlockdevOnError on_source_error,
+ bool has_on_target_error,
+ BlockdevOnError on_target_error,
+ BlockJobTxn *txn, Error **errp)
{
BlockBackend *blk;
BlockDriverState *bs;
BlockDriverState *target_bs;
BlockDriverState *source = NULL;
+ BlockJob *job = NULL;
BdrvDirtyBitmap *bmap = NULL;
AioContext *aio_context;
QDict *options = NULL;
@@ -3098,7 +3105,7 @@ static void do_drive_backup(const char *device, const char *target,
if (!blk) {
error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
"Device '%s' not found", device);
- return;
+ return NULL;
}
aio_context = blk_get_aio_context(blk);
@@ -3181,9 +3188,9 @@ static void do_drive_backup(const char *device, const char *target,
}
}
- backup_start(bs, target_bs, speed, sync, bmap,
- on_source_error, on_target_error,
- block_job_cb, bs, txn, &local_err);
+ job = backup_start(bs, target_bs, speed, sync, bmap,
+ on_source_error, on_target_error,
+ block_job_cb, bs, txn, &local_err);
if (local_err != NULL) {
bdrv_unref(target_bs);
error_propagate(errp, local_err);
@@ -3192,6 +3199,7 @@ static void do_drive_backup(const char *device, const char *target,
out:
aio_context_release(aio_context);
+ return job;
}
void qmp_drive_backup(const char *device, const char *target,
@@ -3204,12 +3212,15 @@ void qmp_drive_backup(const char *device, const char *target,
bool has_on_target_error, BlockdevOnError on_target_error,
Error **errp)
{
- return do_drive_backup(device, target, has_format, format, sync,
- has_mode, mode, has_speed, speed,
- has_bitmap, bitmap,
- has_on_source_error, on_source_error,
- has_on_target_error, on_target_error,
- NULL, errp);
+ BlockJob *job = do_drive_backup(device, target, has_format, format, sync,
+ has_mode, mode, has_speed, speed,
+ has_bitmap, bitmap,
+ has_on_source_error, on_source_error,
+ has_on_target_error, on_target_error,
+ NULL, errp);
+ if (job) {
+ block_job_unref(job);
+ }
}
BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp)
@@ -3217,18 +3228,19 @@ BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp)
return bdrv_named_nodes_list(errp);
}
-void do_blockdev_backup(const char *device, const char *target,
- enum MirrorSyncMode sync,
- bool has_speed, int64_t speed,
- bool has_on_source_error,
- BlockdevOnError on_source_error,
- bool has_on_target_error,
- BlockdevOnError on_target_error,
- BlockJobTxn *txn, Error **errp)
+BlockJob *do_blockdev_backup(const char *device, const char *target,
+ enum MirrorSyncMode sync,
+ bool has_speed, int64_t speed,
+ bool has_on_source_error,
+ BlockdevOnError on_source_error,
+ bool has_on_target_error,
+ BlockdevOnError on_target_error,
+ BlockJobTxn *txn, Error **errp)
{
BlockBackend *blk, *target_blk;
BlockDriverState *bs;
BlockDriverState *target_bs;
+ BlockJob *job = NULL;
Error *local_err = NULL;
AioContext *aio_context;
@@ -3245,7 +3257,7 @@ void do_blockdev_backup(const char *device, const char *target,
blk = blk_by_name(device);
if (!blk) {
error_setg(errp, "Device '%s' not found", device);
- return;
+ return NULL;
}
aio_context = blk_get_aio_context(blk);
@@ -3271,14 +3283,15 @@ void do_blockdev_backup(const char *device, const char *target,
bdrv_ref(target_bs);
bdrv_set_aio_context(target_bs, aio_context);
- backup_start(bs, target_bs, speed, sync, NULL, on_source_error,
- on_target_error, block_job_cb, bs, txn, &local_err);
+ job = backup_start(bs, target_bs, speed, sync, NULL, on_source_error,
+ on_target_error, block_job_cb, bs, txn, &local_err);
if (local_err != NULL) {
bdrv_unref(target_bs);
error_propagate(errp, local_err);
}
out:
aio_context_release(aio_context);
+ return job;
}
void qmp_blockdev_backup(const char *device, const char *target,
@@ -3290,10 +3303,13 @@ void qmp_blockdev_backup(const char *device, const char *target,
BlockdevOnError on_target_error,
Error **errp)
{
- do_blockdev_backup(device, target, sync, has_speed, speed,
- has_on_source_error, on_source_error,
- has_on_target_error, on_target_error,
- NULL, errp);
+ BlockJob *job = do_blockdev_backup(device, target, sync, has_speed, speed,
+ has_on_source_error, on_source_error,
+ has_on_target_error, on_target_error,
+ NULL, errp);
+ if (job) {
+ block_job_unref(job);
+ }
}
/* Parameter check and block job starting for drive mirroring.
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 7a86d7f..9a5cc39 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -684,7 +684,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
* Start a backup operation on @bs. Clusters in @bs are written to @target
* until the job is cancelled or manually completed.
*/
-void backup_start(BlockDriverState *bs, BlockDriverState *target,
+BlockJob *backup_start(BlockDriverState *bs, BlockDriverState *target,
int64_t speed, MirrorSyncMode sync_mode,
BdrvDirtyBitmap *sync_bitmap,
BlockdevOnError on_source_error,
--
2.4.3
^ permalink raw reply related [flat|nested] 9+ messages in thread* [Qemu-devel] [PATCH v2 4/5] block/backup: Pack Notifier within BackupBlockJob
2016-01-26 23:54 [Qemu-devel] [PATCH v2 0/5] block: reduce reliance on bs->job pointer John Snow
` (2 preceding siblings ...)
2016-01-26 23:54 ` [Qemu-devel] [PATCH v2 3/5] block: allow backup_start " John Snow
@ 2016-01-26 23:54 ` John Snow
2016-01-27 2:21 ` Fam Zheng
2016-01-26 23:54 ` [Qemu-devel] [PATCH v2 5/5] blockjob: add Job parameter to BlockCompletionFunc John Snow
2016-02-02 20:20 ` [Qemu-devel] [PATCH v2 0/5] block: reduce reliance on bs->job pointer John Snow
5 siblings, 1 reply; 9+ messages in thread
From: John Snow @ 2016-01-26 23:54 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, jcody, John Snow, armbru, qemu-devel
Instead of relying on peeking at bs->job, we want to explicitly get
a reference to the job that was involved in this notifier callback.
Pack the Notifier inside of the BackupBlockJob so we can use
container_of to get a reference back to the BackupBlockJob object.
This cuts out one more case where we rely unnecessarily on bs->job.
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
block/backup.c | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index ce96b45..735cbe6 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -46,6 +46,7 @@ typedef struct BackupBlockJob {
CoRwlock flush_rwlock;
uint64_t sectors_read;
HBitmap *bitmap;
+ NotifierWithReturn before_write;
QLIST_HEAD(, CowRequest) inflight_reqs;
} BackupBlockJob;
@@ -87,11 +88,11 @@ static void cow_request_end(CowRequest *req)
}
static int coroutine_fn backup_do_cow(BlockDriverState *bs,
+ BackupBlockJob *job,
int64_t sector_num, int nb_sectors,
bool *error_is_read,
bool is_write_notifier)
{
- BackupBlockJob *job = (BackupBlockJob *)bs->job;
CowRequest cow_request;
struct iovec iov;
QEMUIOVector bounce_qiov;
@@ -189,6 +190,7 @@ static int coroutine_fn backup_before_write_notify(
NotifierWithReturn *notifier,
void *opaque)
{
+ BackupBlockJob *job = container_of(notifier, BackupBlockJob, before_write);
BdrvTrackedRequest *req = opaque;
int64_t sector_num = req->offset >> BDRV_SECTOR_BITS;
int nb_sectors = req->bytes >> BDRV_SECTOR_BITS;
@@ -196,7 +198,8 @@ static int coroutine_fn backup_before_write_notify(
assert((req->offset & (BDRV_SECTOR_SIZE - 1)) == 0);
assert((req->bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
- return backup_do_cow(req->bs, sector_num, nb_sectors, NULL, true);
+ return backup_do_cow(req->bs, job, sector_num,
+ nb_sectors, NULL, true);
}
static void backup_set_speed(BlockJob *job, int64_t speed, Error **errp)
@@ -344,7 +347,8 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
if (yield_and_check(job)) {
return ret;
}
- ret = backup_do_cow(bs, cluster * BACKUP_SECTORS_PER_CLUSTER,
+ ret = backup_do_cow(bs, job,
+ cluster * BACKUP_SECTORS_PER_CLUSTER,
BACKUP_SECTORS_PER_CLUSTER, &error_is_read,
false);
if ((ret < 0) &&
@@ -380,9 +384,6 @@ static void coroutine_fn backup_run(void *opaque)
BlockDriverState *bs = job->common.bs;
BlockDriverState *target = job->target;
BlockdevOnError on_target_error = job->on_target_error;
- NotifierWithReturn before_write = {
- .notify = backup_before_write_notify,
- };
int64_t start, end;
int ret = 0;
@@ -400,7 +401,8 @@ static void coroutine_fn backup_run(void *opaque)
blk_iostatus_enable(target->blk);
}
- bdrv_add_before_write_notifier(bs, &before_write);
+ job->before_write.notify = backup_before_write_notify;
+ bdrv_add_before_write_notifier(bs, &job->before_write);
if (job->sync_mode == MIRROR_SYNC_MODE_NONE) {
while (!block_job_is_cancelled(&job->common)) {
@@ -452,7 +454,7 @@ static void coroutine_fn backup_run(void *opaque)
}
}
/* FULL sync mode we copy the whole drive. */
- ret = backup_do_cow(bs, start * BACKUP_SECTORS_PER_CLUSTER,
+ ret = backup_do_cow(bs, job, start * BACKUP_SECTORS_PER_CLUSTER,
BACKUP_SECTORS_PER_CLUSTER, &error_is_read, false);
if (ret < 0) {
/* Depending on error action, fail now or retry cluster */
@@ -468,7 +470,7 @@ static void coroutine_fn backup_run(void *opaque)
}
}
- notifier_with_return_remove(&before_write);
+ notifier_with_return_remove(&job->before_write);
/* wait until pending backup_do_cow() calls have completed */
qemu_co_rwlock_wrlock(&job->flush_rwlock);
--
2.4.3
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH v2 4/5] block/backup: Pack Notifier within BackupBlockJob
2016-01-26 23:54 ` [Qemu-devel] [PATCH v2 4/5] block/backup: Pack Notifier within BackupBlockJob John Snow
@ 2016-01-27 2:21 ` Fam Zheng
0 siblings, 0 replies; 9+ messages in thread
From: Fam Zheng @ 2016-01-27 2:21 UTC (permalink / raw)
To: John Snow; +Cc: kwolf, jcody, armbru, qemu-block, qemu-devel
On Tue, 01/26 18:54, John Snow wrote:
> Instead of relying on peeking at bs->job, we want to explicitly get
> a reference to the job that was involved in this notifier callback.
>
> Pack the Notifier inside of the BackupBlockJob so we can use
> container_of to get a reference back to the BackupBlockJob object.
>
> This cuts out one more case where we rely unnecessarily on bs->job.
>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block/backup.c | 20 +++++++++++---------
> 1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/block/backup.c b/block/backup.c
> index ce96b45..735cbe6 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -46,6 +46,7 @@ typedef struct BackupBlockJob {
> CoRwlock flush_rwlock;
> uint64_t sectors_read;
> HBitmap *bitmap;
> + NotifierWithReturn before_write;
> QLIST_HEAD(, CowRequest) inflight_reqs;
> } BackupBlockJob;
>
> @@ -87,11 +88,11 @@ static void cow_request_end(CowRequest *req)
> }
>
> static int coroutine_fn backup_do_cow(BlockDriverState *bs,
> + BackupBlockJob *job,
> int64_t sector_num, int nb_sectors,
> bool *error_is_read,
> bool is_write_notifier)
> {
> - BackupBlockJob *job = (BackupBlockJob *)bs->job;
> CowRequest cow_request;
> struct iovec iov;
> QEMUIOVector bounce_qiov;
> @@ -189,6 +190,7 @@ static int coroutine_fn backup_before_write_notify(
> NotifierWithReturn *notifier,
> void *opaque)
> {
> + BackupBlockJob *job = container_of(notifier, BackupBlockJob, before_write);
> BdrvTrackedRequest *req = opaque;
> int64_t sector_num = req->offset >> BDRV_SECTOR_BITS;
> int nb_sectors = req->bytes >> BDRV_SECTOR_BITS;
> @@ -196,7 +198,8 @@ static int coroutine_fn backup_before_write_notify(
> assert((req->offset & (BDRV_SECTOR_SIZE - 1)) == 0);
> assert((req->bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
>
> - return backup_do_cow(req->bs, sector_num, nb_sectors, NULL, true);
> + return backup_do_cow(req->bs, job, sector_num,
> + nb_sectors, NULL, true);
> }
>
> static void backup_set_speed(BlockJob *job, int64_t speed, Error **errp)
> @@ -344,7 +347,8 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
> if (yield_and_check(job)) {
> return ret;
> }
> - ret = backup_do_cow(bs, cluster * BACKUP_SECTORS_PER_CLUSTER,
> + ret = backup_do_cow(bs, job,
> + cluster * BACKUP_SECTORS_PER_CLUSTER,
> BACKUP_SECTORS_PER_CLUSTER, &error_is_read,
> false);
> if ((ret < 0) &&
> @@ -380,9 +384,6 @@ static void coroutine_fn backup_run(void *opaque)
> BlockDriverState *bs = job->common.bs;
> BlockDriverState *target = job->target;
> BlockdevOnError on_target_error = job->on_target_error;
> - NotifierWithReturn before_write = {
> - .notify = backup_before_write_notify,
> - };
> int64_t start, end;
> int ret = 0;
>
> @@ -400,7 +401,8 @@ static void coroutine_fn backup_run(void *opaque)
> blk_iostatus_enable(target->blk);
> }
>
> - bdrv_add_before_write_notifier(bs, &before_write);
> + job->before_write.notify = backup_before_write_notify;
> + bdrv_add_before_write_notifier(bs, &job->before_write);
>
> if (job->sync_mode == MIRROR_SYNC_MODE_NONE) {
> while (!block_job_is_cancelled(&job->common)) {
> @@ -452,7 +454,7 @@ static void coroutine_fn backup_run(void *opaque)
> }
> }
> /* FULL sync mode we copy the whole drive. */
> - ret = backup_do_cow(bs, start * BACKUP_SECTORS_PER_CLUSTER,
> + ret = backup_do_cow(bs, job, start * BACKUP_SECTORS_PER_CLUSTER,
> BACKUP_SECTORS_PER_CLUSTER, &error_is_read, false);
> if (ret < 0) {
> /* Depending on error action, fail now or retry cluster */
> @@ -468,7 +470,7 @@ static void coroutine_fn backup_run(void *opaque)
> }
> }
>
> - notifier_with_return_remove(&before_write);
> + notifier_with_return_remove(&job->before_write);
>
> /* wait until pending backup_do_cow() calls have completed */
> qemu_co_rwlock_wrlock(&job->flush_rwlock);
> --
> 2.4.3
>
>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH v2 5/5] blockjob: add Job parameter to BlockCompletionFunc
2016-01-26 23:54 [Qemu-devel] [PATCH v2 0/5] block: reduce reliance on bs->job pointer John Snow
` (3 preceding siblings ...)
2016-01-26 23:54 ` [Qemu-devel] [PATCH v2 4/5] block/backup: Pack Notifier within BackupBlockJob John Snow
@ 2016-01-26 23:54 ` John Snow
2016-01-27 2:25 ` Fam Zheng
2016-02-02 20:20 ` [Qemu-devel] [PATCH v2 0/5] block: reduce reliance on bs->job pointer John Snow
5 siblings, 1 reply; 9+ messages in thread
From: John Snow @ 2016-01-26 23:54 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, jcody, John Snow, armbru, qemu-devel
It will no longer be sufficient to rely on the opaque parameter
containing a BDS, and there's no way to reliably include a
self-reference to the job we're creating, so always pass the Job
object forward to any callbacks.
Signed-off-by: John Snow <jsnow@redhat.com>
---
block/backup.c | 2 +-
block/commit.c | 2 +-
block/mirror.c | 6 +++---
block/stream.c | 2 +-
blockdev.c | 14 +++++---------
blockjob.c | 13 +++++++++++--
include/block/block.h | 2 ++
include/block/block_int.h | 10 +++++-----
include/block/blockjob.h | 6 ++++--
qemu-img.c | 4 ++--
tests/test-blockjob-txn.c | 4 ++--
11 files changed, 37 insertions(+), 28 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 735cbe6..dd7e532 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -492,7 +492,7 @@ BlockJob *backup_start(BlockDriverState *bs, BlockDriverState *target,
BdrvDirtyBitmap *sync_bitmap,
BlockdevOnError on_source_error,
BlockdevOnError on_target_error,
- BlockCompletionFunc *cb, void *opaque,
+ BlockJobCompletionFunc *cb, void *opaque,
BlockJobTxn *txn, Error **errp)
{
int64_t len;
diff --git a/block/commit.c b/block/commit.c
index 446a3ae..aca4b84 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -203,7 +203,7 @@ static const BlockJobDriver commit_job_driver = {
void commit_start(BlockDriverState *bs, BlockDriverState *base,
BlockDriverState *top, int64_t speed,
- BlockdevOnError on_error, BlockCompletionFunc *cb,
+ BlockdevOnError on_error, BlockJobCompletionFunc *cb,
void *opaque, const char *backing_file_str, Error **errp)
{
CommitBlockJob *s;
diff --git a/block/mirror.c b/block/mirror.c
index b193e36..8016834 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -715,7 +715,7 @@ static BlockJob *mirror_start_job(BlockDriverState *bs,
BlockdevOnError on_source_error,
BlockdevOnError on_target_error,
bool unmap,
- BlockCompletionFunc *cb,
+ BlockJobCompletionFunc *cb,
void *opaque, Error **errp,
const BlockJobDriver *driver,
bool is_none_mode, BlockDriverState *base)
@@ -802,7 +802,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
MirrorSyncMode mode, BlockdevOnError on_source_error,
BlockdevOnError on_target_error,
bool unmap,
- BlockCompletionFunc *cb,
+ BlockJobCompletionFunc *cb,
void *opaque, Error **errp)
{
bool is_none_mode;
@@ -827,7 +827,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
BlockJob *commit_active_start(BlockDriverState *bs, BlockDriverState *base,
int64_t speed,
BlockdevOnError on_error,
- BlockCompletionFunc *cb,
+ BlockJobCompletionFunc *cb,
void *opaque, Error **errp)
{
int64_t length, base_length;
diff --git a/block/stream.c b/block/stream.c
index 26a2990..0c8ae20 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -217,7 +217,7 @@ static const BlockJobDriver stream_job_driver = {
BlockJob *stream_start(BlockDriverState *bs, BlockDriverState *base,
const char *backing_file_str, int64_t speed,
BlockdevOnError on_error,
- BlockCompletionFunc *cb,
+ BlockJobCompletionFunc *cb,
void *opaque, Error **errp)
{
StreamBlockJob *s;
diff --git a/blockdev.c b/blockdev.c
index ad46aa8..9851405 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2854,28 +2854,24 @@ out:
aio_context_release(aio_context);
}
-static void block_job_cb(void *opaque, int ret)
+static void block_job_cb(BlockJob *job, int ret)
{
/* Note that this function may be executed from another AioContext besides
* the QEMU main loop. If you need to access anything that assumes the
* QEMU global mutex, use a BH or introduce a mutex.
*/
-
- BlockDriverState *bs = opaque;
const char *msg = NULL;
- trace_block_job_cb(bs, bs->job, ret);
-
- assert(bs->job);
+ trace_block_job_cb(job->bs, job, ret);
if (ret < 0) {
msg = strerror(-ret);
}
- if (block_job_is_cancelled(bs->job)) {
- block_job_event_cancelled(bs->job);
+ if (block_job_is_cancelled(job)) {
+ block_job_event_cancelled(job);
} else {
- block_job_event_completed(bs->job, msg);
+ block_job_event_completed(job, msg);
}
}
diff --git a/blockjob.c b/blockjob.c
index 80adb9d..fe6873c 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -51,7 +51,7 @@ struct BlockJobTxn {
};
void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
- int64_t speed, BlockCompletionFunc *cb,
+ int64_t speed, BlockJobCompletionFunc *cb,
void *opaque, Error **errp)
{
BlockJob *job;
@@ -90,6 +90,15 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
return job;
}
+/**
+ * For generic callbacks to retrieve the data they submitted
+ * during callback registration.
+ */
+void *block_job_data(BlockJob *job)
+{
+ return job->opaque;
+}
+
void block_job_ref(BlockJob *job)
{
++job->refcnt;
@@ -118,7 +127,7 @@ static void block_job_completed_single(BlockJob *job)
job->driver->abort(job);
}
}
- job->cb(job->opaque, job->ret);
+ job->cb(job, job->ret);
if (job->txn) {
block_job_txn_unref(job->txn);
}
diff --git a/include/block/block.h b/include/block/block.h
index 25f36dc..ed6f2ee 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -16,6 +16,8 @@ typedef struct BdrvChild BdrvChild;
typedef struct BdrvChildRole BdrvChildRole;
typedef struct BlockJobTxn BlockJobTxn;
+typedef void BlockJobCompletionFunc(BlockJob *job, int ret);
+
typedef struct BlockDriverInfo {
/* in bytes, 0 if irrelevant */
int cluster_size;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 9a5cc39..271e922 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -601,7 +601,7 @@ int is_windows_drive(const char *filename);
BlockJob *stream_start(BlockDriverState *bs, BlockDriverState *base,
const char *base_id, int64_t speed,
BlockdevOnError on_error,
- BlockCompletionFunc *cb,
+ BlockJobCompletionFunc *cb,
void *opaque, Error **errp);
/**
@@ -619,7 +619,7 @@ BlockJob *stream_start(BlockDriverState *bs, BlockDriverState *base,
*/
void commit_start(BlockDriverState *bs, BlockDriverState *base,
BlockDriverState *top, int64_t speed,
- BlockdevOnError on_error, BlockCompletionFunc *cb,
+ BlockdevOnError on_error, BlockJobCompletionFunc *cb,
void *opaque, const char *backing_file_str, Error **errp);
/**
* commit_active_start:
@@ -635,7 +635,7 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,
BlockJob *commit_active_start(BlockDriverState *bs, BlockDriverState *base,
int64_t speed,
BlockdevOnError on_error,
- BlockCompletionFunc *cb,
+ BlockJobCompletionFunc *cb,
void *opaque, Error **errp);
/*
* mirror_start:
@@ -665,7 +665,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
MirrorSyncMode mode, BlockdevOnError on_source_error,
BlockdevOnError on_target_error,
bool unmap,
- BlockCompletionFunc *cb,
+ BlockJobCompletionFunc *cb,
void *opaque, Error **errp);
/*
@@ -689,7 +689,7 @@ BlockJob *backup_start(BlockDriverState *bs, BlockDriverState *target,
BdrvDirtyBitmap *sync_bitmap,
BlockdevOnError on_source_error,
BlockdevOnError on_target_error,
- BlockCompletionFunc *cb, void *opaque,
+ BlockJobCompletionFunc *cb, void *opaque,
BlockJobTxn *txn, Error **errp);
void blk_set_bs(BlockBackend *blk, BlockDriverState *bs);
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index d84ccd8..b233d27 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -143,7 +143,7 @@ struct BlockJob {
int64_t speed;
/** The completion function that will be called when the job completes. */
- BlockCompletionFunc *cb;
+ BlockJobCompletionFunc *cb;
/** Block other operations when block job is running */
Error *blocker;
@@ -186,9 +186,11 @@ struct BlockJob {
* called from a wrapper that is specific to the job type.
*/
void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
- int64_t speed, BlockCompletionFunc *cb,
+ int64_t speed, BlockJobCompletionFunc *cb,
void *opaque, Error **errp);
+void *block_job_data(BlockJob *job);
+
/**
* block_job_sleep_ns:
* @job: The job that calls the function.
diff --git a/qemu-img.c b/qemu-img.c
index 8d11708..cc96cc8 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -635,9 +635,9 @@ typedef struct CommonBlockJobCBInfo {
Error **errp;
} CommonBlockJobCBInfo;
-static void common_block_job_cb(void *opaque, int ret)
+static void common_block_job_cb(BlockJob *job, int ret)
{
- CommonBlockJobCBInfo *cbi = opaque;
+ CommonBlockJobCBInfo *cbi = block_job_data(job);
if (ret < 0) {
error_setg_errno(cbi->errp, -ret, "Block job failed");
diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
index 34747e9..3e8d17d 100644
--- a/tests/test-blockjob-txn.c
+++ b/tests/test-blockjob-txn.c
@@ -66,9 +66,9 @@ typedef struct {
int *result;
} TestBlockJobCBData;
-static void test_block_job_cb(void *opaque, int ret)
+static void test_block_job_cb(BlockJob *job, int ret)
{
- TestBlockJobCBData *data = opaque;
+ TestBlockJobCBData *data = block_job_data(job);
if (!ret && block_job_is_cancelled(&data->job->common)) {
ret = -ECANCELED;
}
--
2.4.3
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH v2 5/5] blockjob: add Job parameter to BlockCompletionFunc
2016-01-26 23:54 ` [Qemu-devel] [PATCH v2 5/5] blockjob: add Job parameter to BlockCompletionFunc John Snow
@ 2016-01-27 2:25 ` Fam Zheng
0 siblings, 0 replies; 9+ messages in thread
From: Fam Zheng @ 2016-01-27 2:25 UTC (permalink / raw)
To: John Snow; +Cc: kwolf, jcody, armbru, qemu-block, qemu-devel
On Tue, 01/26 18:54, John Snow wrote:
> It will no longer be sufficient to rely on the opaque parameter
> containing a BDS, and there's no way to reliably include a
> self-reference to the job we're creating, so always pass the Job
> object forward to any callbacks.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block/backup.c | 2 +-
> block/commit.c | 2 +-
> block/mirror.c | 6 +++---
> block/stream.c | 2 +-
> blockdev.c | 14 +++++---------
> blockjob.c | 13 +++++++++++--
> include/block/block.h | 2 ++
> include/block/block_int.h | 10 +++++-----
> include/block/blockjob.h | 6 ++++--
> qemu-img.c | 4 ++--
> tests/test-blockjob-txn.c | 4 ++--
> 11 files changed, 37 insertions(+), 28 deletions(-)
>
> diff --git a/block/backup.c b/block/backup.c
> index 735cbe6..dd7e532 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -492,7 +492,7 @@ BlockJob *backup_start(BlockDriverState *bs, BlockDriverState *target,
> BdrvDirtyBitmap *sync_bitmap,
> BlockdevOnError on_source_error,
> BlockdevOnError on_target_error,
> - BlockCompletionFunc *cb, void *opaque,
> + BlockJobCompletionFunc *cb, void *opaque,
> BlockJobTxn *txn, Error **errp)
> {
> int64_t len;
> diff --git a/block/commit.c b/block/commit.c
> index 446a3ae..aca4b84 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -203,7 +203,7 @@ static const BlockJobDriver commit_job_driver = {
>
> void commit_start(BlockDriverState *bs, BlockDriverState *base,
> BlockDriverState *top, int64_t speed,
> - BlockdevOnError on_error, BlockCompletionFunc *cb,
> + BlockdevOnError on_error, BlockJobCompletionFunc *cb,
> void *opaque, const char *backing_file_str, Error **errp)
> {
> CommitBlockJob *s;
> diff --git a/block/mirror.c b/block/mirror.c
> index b193e36..8016834 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -715,7 +715,7 @@ static BlockJob *mirror_start_job(BlockDriverState *bs,
> BlockdevOnError on_source_error,
> BlockdevOnError on_target_error,
> bool unmap,
> - BlockCompletionFunc *cb,
> + BlockJobCompletionFunc *cb,
> void *opaque, Error **errp,
> const BlockJobDriver *driver,
> bool is_none_mode, BlockDriverState *base)
> @@ -802,7 +802,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
> MirrorSyncMode mode, BlockdevOnError on_source_error,
> BlockdevOnError on_target_error,
> bool unmap,
> - BlockCompletionFunc *cb,
> + BlockJobCompletionFunc *cb,
> void *opaque, Error **errp)
> {
> bool is_none_mode;
> @@ -827,7 +827,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
> BlockJob *commit_active_start(BlockDriverState *bs, BlockDriverState *base,
> int64_t speed,
> BlockdevOnError on_error,
> - BlockCompletionFunc *cb,
> + BlockJobCompletionFunc *cb,
> void *opaque, Error **errp)
> {
> int64_t length, base_length;
> diff --git a/block/stream.c b/block/stream.c
> index 26a2990..0c8ae20 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -217,7 +217,7 @@ static const BlockJobDriver stream_job_driver = {
> BlockJob *stream_start(BlockDriverState *bs, BlockDriverState *base,
> const char *backing_file_str, int64_t speed,
> BlockdevOnError on_error,
> - BlockCompletionFunc *cb,
> + BlockJobCompletionFunc *cb,
> void *opaque, Error **errp)
> {
> StreamBlockJob *s;
> diff --git a/blockdev.c b/blockdev.c
> index ad46aa8..9851405 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2854,28 +2854,24 @@ out:
> aio_context_release(aio_context);
> }
>
> -static void block_job_cb(void *opaque, int ret)
> +static void block_job_cb(BlockJob *job, int ret)
> {
> /* Note that this function may be executed from another AioContext besides
> * the QEMU main loop. If you need to access anything that assumes the
> * QEMU global mutex, use a BH or introduce a mutex.
> */
> -
> - BlockDriverState *bs = opaque;
> const char *msg = NULL;
>
> - trace_block_job_cb(bs, bs->job, ret);
> -
> - assert(bs->job);
> + trace_block_job_cb(job->bs, job, ret);
>
> if (ret < 0) {
> msg = strerror(-ret);
> }
>
> - if (block_job_is_cancelled(bs->job)) {
> - block_job_event_cancelled(bs->job);
> + if (block_job_is_cancelled(job)) {
> + block_job_event_cancelled(job);
> } else {
> - block_job_event_completed(bs->job, msg);
> + block_job_event_completed(job, msg);
> }
> }
>
> diff --git a/blockjob.c b/blockjob.c
> index 80adb9d..fe6873c 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -51,7 +51,7 @@ struct BlockJobTxn {
> };
>
> void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
> - int64_t speed, BlockCompletionFunc *cb,
> + int64_t speed, BlockJobCompletionFunc *cb,
> void *opaque, Error **errp)
> {
> BlockJob *job;
> @@ -90,6 +90,15 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
> return job;
> }
>
> +/**
> + * For generic callbacks to retrieve the data they submitted
> + * during callback registration.
> + */
> +void *block_job_data(BlockJob *job)
> +{
> + return job->opaque;
> +}
> +
> void block_job_ref(BlockJob *job)
> {
> ++job->refcnt;
> @@ -118,7 +127,7 @@ static void block_job_completed_single(BlockJob *job)
> job->driver->abort(job);
> }
> }
> - job->cb(job->opaque, job->ret);
> + job->cb(job, job->ret);
> if (job->txn) {
> block_job_txn_unref(job->txn);
> }
> diff --git a/include/block/block.h b/include/block/block.h
> index 25f36dc..ed6f2ee 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -16,6 +16,8 @@ typedef struct BdrvChild BdrvChild;
> typedef struct BdrvChildRole BdrvChildRole;
> typedef struct BlockJobTxn BlockJobTxn;
>
> +typedef void BlockJobCompletionFunc(BlockJob *job, int ret);
> +
> typedef struct BlockDriverInfo {
> /* in bytes, 0 if irrelevant */
> int cluster_size;
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 9a5cc39..271e922 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -601,7 +601,7 @@ int is_windows_drive(const char *filename);
> BlockJob *stream_start(BlockDriverState *bs, BlockDriverState *base,
> const char *base_id, int64_t speed,
> BlockdevOnError on_error,
> - BlockCompletionFunc *cb,
> + BlockJobCompletionFunc *cb,
> void *opaque, Error **errp);
>
> /**
> @@ -619,7 +619,7 @@ BlockJob *stream_start(BlockDriverState *bs, BlockDriverState *base,
> */
> void commit_start(BlockDriverState *bs, BlockDriverState *base,
> BlockDriverState *top, int64_t speed,
> - BlockdevOnError on_error, BlockCompletionFunc *cb,
> + BlockdevOnError on_error, BlockJobCompletionFunc *cb,
> void *opaque, const char *backing_file_str, Error **errp);
> /**
> * commit_active_start:
> @@ -635,7 +635,7 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,
> BlockJob *commit_active_start(BlockDriverState *bs, BlockDriverState *base,
> int64_t speed,
> BlockdevOnError on_error,
> - BlockCompletionFunc *cb,
> + BlockJobCompletionFunc *cb,
> void *opaque, Error **errp);
> /*
> * mirror_start:
> @@ -665,7 +665,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
> MirrorSyncMode mode, BlockdevOnError on_source_error,
> BlockdevOnError on_target_error,
> bool unmap,
> - BlockCompletionFunc *cb,
> + BlockJobCompletionFunc *cb,
> void *opaque, Error **errp);
>
> /*
> @@ -689,7 +689,7 @@ BlockJob *backup_start(BlockDriverState *bs, BlockDriverState *target,
> BdrvDirtyBitmap *sync_bitmap,
> BlockdevOnError on_source_error,
> BlockdevOnError on_target_error,
> - BlockCompletionFunc *cb, void *opaque,
> + BlockJobCompletionFunc *cb, void *opaque,
> BlockJobTxn *txn, Error **errp);
>
> void blk_set_bs(BlockBackend *blk, BlockDriverState *bs);
> diff --git a/include/block/blockjob.h b/include/block/blockjob.h
> index d84ccd8..b233d27 100644
> --- a/include/block/blockjob.h
> +++ b/include/block/blockjob.h
> @@ -143,7 +143,7 @@ struct BlockJob {
> int64_t speed;
>
> /** The completion function that will be called when the job completes. */
> - BlockCompletionFunc *cb;
> + BlockJobCompletionFunc *cb;
>
> /** Block other operations when block job is running */
> Error *blocker;
> @@ -186,9 +186,11 @@ struct BlockJob {
> * called from a wrapper that is specific to the job type.
> */
> void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
> - int64_t speed, BlockCompletionFunc *cb,
> + int64_t speed, BlockJobCompletionFunc *cb,
> void *opaque, Error **errp);
>
> +void *block_job_data(BlockJob *job);
> +
> /**
> * block_job_sleep_ns:
> * @job: The job that calls the function.
> diff --git a/qemu-img.c b/qemu-img.c
> index 8d11708..cc96cc8 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -635,9 +635,9 @@ typedef struct CommonBlockJobCBInfo {
> Error **errp;
> } CommonBlockJobCBInfo;
>
> -static void common_block_job_cb(void *opaque, int ret)
> +static void common_block_job_cb(BlockJob *job, int ret)
> {
> - CommonBlockJobCBInfo *cbi = opaque;
> + CommonBlockJobCBInfo *cbi = block_job_data(job);
>
> if (ret < 0) {
> error_setg_errno(cbi->errp, -ret, "Block job failed");
> diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
> index 34747e9..3e8d17d 100644
> --- a/tests/test-blockjob-txn.c
> +++ b/tests/test-blockjob-txn.c
> @@ -66,9 +66,9 @@ typedef struct {
> int *result;
> } TestBlockJobCBData;
>
> -static void test_block_job_cb(void *opaque, int ret)
> +static void test_block_job_cb(BlockJob *job, int ret)
> {
> - TestBlockJobCBData *data = opaque;
> + TestBlockJobCBData *data = block_job_data(job);
> if (!ret && block_job_is_cancelled(&data->job->common)) {
> ret = -ECANCELED;
> }
> --
> 2.4.3
>
>
Reviewed-by: Fam Zheng <famz@redhat.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/5] block: reduce reliance on bs->job pointer
2016-01-26 23:54 [Qemu-devel] [PATCH v2 0/5] block: reduce reliance on bs->job pointer John Snow
` (4 preceding siblings ...)
2016-01-26 23:54 ` [Qemu-devel] [PATCH v2 5/5] blockjob: add Job parameter to BlockCompletionFunc John Snow
@ 2016-02-02 20:20 ` John Snow
5 siblings, 0 replies; 9+ messages in thread
From: John Snow @ 2016-02-02 20:20 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, jcody, armbru, qemu-devel
ping, I think this one is up to you, jtc?
On 01/26/2016 06:54 PM, John Snow wrote:
> This is a small collection of patches to reduce our use of the bs->job
> pointer where possible. There are still more usages in the code, but
> this cuts down on a few.
>
> The goal is to eventually eliminate all of them and allow multiple block
> jobs to run concurrently, but design on what that will look like is
> on-going.
>
> In the meantime, eliminate a few obviously needless references to
> bs->job by allowing more systems to carry pointers to jobs directly
> instead of trying to fish the pointer out of the BDS all the time.
>
> ===
> v2:
> ===
>
> Key:
> [----] : patches are identical
> [####] : number of functional differences between upstream/downstream patch
> [down] : patch is downstream-only
> The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
>
> 001/5:[----] [--] 'block: Allow mirror_start to return job references'
> 002/5:[----] [--] 'block: Allow stream_start to return job references'
> 003/5:[----] [--] 'block: allow backup_start to return job references'
> 004/5:[down] 'block/backup: Pack Notifier within BackupBlockJob'
> 005/5:[0006] [FC] 'blockjob: add Job parameter to BlockCompletionFunc'
>
> 4: Rewritten to pack the notifier within the job, instead of the job within
> a subclassed notifier. [Paolo]
> 5: Remove junk assert and extraneous local BDS variable. [Kevin]
>
> ________________________________________________________________________________
>
> For convenience, this branch is available at:
> https://github.com/jnsnow/qemu.git branch block-multijob2
> https://github.com/jnsnow/qemu/tree/block-multijob2
>
> This version is tagged block-multijob2-v2:
> https://github.com/jnsnow/qemu/releases/tag/block-multijob2-v2
>
> John Snow (5):
> block: Allow mirror_start to return job references
> block: Allow stream_start to return job references
> block: allow backup_start to return job references
> block/backup: Pack Notifier within BackupBlockJob
> blockjob: add Job parameter to BlockCompletionFunc
>
> block/backup.c | 58 +++++++------
> block/commit.c | 2 +-
> block/mirror.c | 74 ++++++++--------
> block/stream.c | 10 ++-
> blockdev.c | 210 +++++++++++++++++++++++++---------------------
> blockjob.c | 13 ++-
> include/block/block.h | 2 +
> include/block/block_int.h | 27 +++---
> include/block/blockjob.h | 6 +-
> qemu-img.c | 16 ++--
> tests/test-blockjob-txn.c | 4 +-
> 11 files changed, 237 insertions(+), 185 deletions(-)
>
--
—js
^ permalink raw reply [flat|nested] 9+ messages in thread