* [Qemu-devel] [PATCH 0/5] block: reduce reliance on bs->job pointer
@ 2016-01-12 0:36 John Snow
2016-01-12 0:36 ` [Qemu-devel] [PATCH 1/5] block: Allow mirror_start to return job references John Snow
` (4 more replies)
0 siblings, 5 replies; 13+ messages in thread
From: John Snow @ 2016-01-12 0:36 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, jcody, John Snow, armbru, qemu-devel
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.
________________________________________________________________________________
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-v1:
https://github.com/jnsnow/qemu/releases/tag/block-multijob2-v1
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: Add subclassed notifier
blockjob: add Job parameter to BlockCompletionFunc
block/backup.c | 68 +++++++++------
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, 250 insertions(+), 182 deletions(-)
--
2.4.3
^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH 1/5] block: Allow mirror_start to return job references
2016-01-12 0:36 [Qemu-devel] [PATCH 0/5] block: reduce reliance on bs->job pointer John Snow
@ 2016-01-12 0:36 ` John Snow
2016-01-12 0:36 ` [Qemu-devel] [PATCH 2/5] block: Allow stream_start " John Snow
` (3 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: John Snow @ 2016-01-12 0:36 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.
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 f201f2b..92706ab 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -706,17 +706,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;
@@ -731,12 +732,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) {
@@ -748,19 +749,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);
@@ -777,7 +778,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);
@@ -787,9 +788,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,
@@ -803,6 +806,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");
@@ -810,27 +814,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);
@@ -859,19 +867,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 2df0c6d..d31bb03 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2956,6 +2956,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
@@ -3037,8 +3038,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);
@@ -3049,6 +3050,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 256609d..a68c7dc 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -630,11 +630,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 3d48b4f..7649f80 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -647,8 +647,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 {
@@ -662,6 +663,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)
@@ -670,6 +673,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;
@@ -758,8 +762,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;
}
@@ -772,7 +776,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] 13+ messages in thread
* [Qemu-devel] [PATCH 2/5] block: Allow stream_start to return job references
2016-01-12 0:36 [Qemu-devel] [PATCH 0/5] block: reduce reliance on bs->job pointer John Snow
2016-01-12 0:36 ` [Qemu-devel] [PATCH 1/5] block: Allow mirror_start to return job references John Snow
@ 2016-01-12 0:36 ` John Snow
2016-01-12 0:36 ` [Qemu-devel] [PATCH 3/5] block: allow backup_start " John Snow
` (2 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: John Snow @ 2016-01-12 0:36 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.
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 25af7ef..1dfeac0 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -213,7 +213,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,
@@ -225,19 +225,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 d31bb03..f66cac8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2884,6 +2884,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;
@@ -2933,14 +2934,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 a68c7dc..ea3b06b 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -597,10 +597,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] 13+ messages in thread
* [Qemu-devel] [PATCH 3/5] block: allow backup_start to return job references
2016-01-12 0:36 [Qemu-devel] [PATCH 0/5] block: reduce reliance on bs->job pointer John Snow
2016-01-12 0:36 ` [Qemu-devel] [PATCH 1/5] block: Allow mirror_start to return job references John Snow
2016-01-12 0:36 ` [Qemu-devel] [PATCH 2/5] block: Allow stream_start " John Snow
@ 2016-01-12 0:36 ` John Snow
2016-01-12 0:36 ` [Qemu-devel] [PATCH 4/5] block/backup: Add subclassed notifier John Snow
2016-01-12 0:36 ` [Qemu-devel] [PATCH 5/5] blockjob: add Job parameter to BlockCompletionFunc John Snow
4 siblings, 0 replies; 13+ messages in thread
From: John Snow @ 2016-01-12 0:36 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.
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 705bb77..325e247 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -487,13 +487,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;
@@ -503,53 +503,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);
@@ -574,13 +574,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 f66cac8..9b37ace 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1806,17 +1806,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)
{
@@ -1846,31 +1846,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);
}
}
@@ -1882,6 +1880,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 {
@@ -1891,14 +1894,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)
{
@@ -1938,28 +1941,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);
}
}
@@ -1971,6 +1972,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 {
@@ -3058,22 +3064,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;
@@ -3099,7 +3106,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);
@@ -3182,9 +3189,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);
@@ -3193,6 +3200,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,
@@ -3205,12 +3213,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)
@@ -3218,18 +3229,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;
@@ -3246,7 +3258,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);
@@ -3272,14 +3284,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,
@@ -3291,10 +3304,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 ea3b06b..b27842f 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -683,7 +683,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] 13+ messages in thread
* [Qemu-devel] [PATCH 4/5] block/backup: Add subclassed notifier
2016-01-12 0:36 [Qemu-devel] [PATCH 0/5] block: reduce reliance on bs->job pointer John Snow
` (2 preceding siblings ...)
2016-01-12 0:36 ` [Qemu-devel] [PATCH 3/5] block: allow backup_start " John Snow
@ 2016-01-12 0:36 ` John Snow
2016-01-12 8:46 ` Paolo Bonzini
2016-01-12 0:36 ` [Qemu-devel] [PATCH 5/5] blockjob: add Job parameter to BlockCompletionFunc John Snow
4 siblings, 1 reply; 13+ messages in thread
From: John Snow @ 2016-01-12 0:36 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.
Extend the Notifier to include a job pointer, and include a reference
to the job registering the callback. This cuts out a few more cases
where we have to rely on bs->job.
Signed-off-by: John Snow <jsnow@redhat.com>
---
block/backup.c | 30 ++++++++++++++++++++++--------
1 file changed, 22 insertions(+), 8 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 325e247..58c76be 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -89,11 +89,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;
@@ -187,10 +187,17 @@ out:
return ret;
}
+/* Extend the generic Notifier interface */
+typedef struct BackupNotifier {
+ NotifierWithReturn common;
+ BackupBlockJob *job;
+} BackupNotifier;
+
static int coroutine_fn backup_before_write_notify(
NotifierWithReturn *notifier,
void *opaque)
{
+ BackupNotifier *bnotifier = (BackupNotifier *)notifier;
BdrvTrackedRequest *req = opaque;
int64_t sector_num = req->offset >> BDRV_SECTOR_BITS;
int nb_sectors = req->bytes >> BDRV_SECTOR_BITS;
@@ -198,7 +205,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, bnotifier->job, sector_num,
+ nb_sectors, NULL, true);
}
static void backup_set_speed(BlockJob *job, int64_t speed, Error **errp)
@@ -346,7 +354,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) &&
@@ -382,8 +391,11 @@ 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,
+ BackupNotifier before_write = {
+ .common = {
+ .notify = backup_before_write_notify,
+ },
+ .job = job,
};
int64_t start, end;
int ret = 0;
@@ -402,7 +414,8 @@ static void coroutine_fn backup_run(void *opaque)
blk_iostatus_enable(target->blk);
}
- bdrv_add_before_write_notifier(bs, &before_write);
+ block_job_ref(&job->common);
+ bdrv_add_before_write_notifier(bs, (NotifierWithReturn *)&before_write);
if (job->sync_mode == MIRROR_SYNC_MODE_NONE) {
while (!block_job_is_cancelled(&job->common)) {
@@ -454,7 +467,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 */
@@ -470,7 +483,8 @@ static void coroutine_fn backup_run(void *opaque)
}
}
- notifier_with_return_remove(&before_write);
+ notifier_with_return_remove((NotifierWithReturn *)&before_write);
+ block_job_unref(&job->common);
/* 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] 13+ messages in thread
* [Qemu-devel] [PATCH 5/5] blockjob: add Job parameter to BlockCompletionFunc
2016-01-12 0:36 [Qemu-devel] [PATCH 0/5] block: reduce reliance on bs->job pointer John Snow
` (3 preceding siblings ...)
2016-01-12 0:36 ` [Qemu-devel] [PATCH 4/5] block/backup: Add subclassed notifier John Snow
@ 2016-01-12 0:36 ` John Snow
2016-01-18 14:25 ` Kevin Wolf
4 siblings, 1 reply; 13+ messages in thread
From: John Snow @ 2016-01-12 0:36 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, 39 insertions(+), 26 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 58c76be..cadb880 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -506,7 +506,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 a5d02aa..ef4fd5a 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -202,7 +202,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 92706ab..18134e4 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -714,7 +714,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)
@@ -801,7 +801,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;
@@ -826,7 +826,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 1dfeac0..1bd8220 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -216,7 +216,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 9b37ace..6713ecb 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2855,28 +2855,28 @@ 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;
+ BlockDriverState *bs = job->bs;
const char *msg = NULL;
- trace_block_job_cb(bs, bs->job, ret);
+ trace_block_job_cb(bs, job, ret);
- assert(bs->job);
+ assert(job);
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 c96923d..8bd9f41 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 b27842f..ee18c1d 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -600,7 +600,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);
/**
@@ -618,7 +618,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:
@@ -634,7 +634,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:
@@ -664,7 +664,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);
/*
@@ -688,7 +688,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 7649f80..463aba9 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -638,9 +638,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] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 4/5] block/backup: Add subclassed notifier
2016-01-12 0:36 ` [Qemu-devel] [PATCH 4/5] block/backup: Add subclassed notifier John Snow
@ 2016-01-12 8:46 ` Paolo Bonzini
2016-01-12 17:57 ` John Snow
0 siblings, 1 reply; 13+ messages in thread
From: Paolo Bonzini @ 2016-01-12 8:46 UTC (permalink / raw)
To: John Snow, qemu-block; +Cc: kwolf, armbru, qemu-devel
On 12/01/2016 01:36, 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.
>
> Extend the Notifier to include a job pointer, and include a reference
> to the job registering the callback. This cuts out a few more cases
> where we have to rely on bs->job.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
Why don't you just put the NotifierWithReturn inside the BackupBlockJob
struct, and use container_of to get from NWR to BackupBlockJob?
Paolo
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 4/5] block/backup: Add subclassed notifier
2016-01-12 8:46 ` Paolo Bonzini
@ 2016-01-12 17:57 ` John Snow
2016-01-12 18:01 ` Paolo Bonzini
0 siblings, 1 reply; 13+ messages in thread
From: John Snow @ 2016-01-12 17:57 UTC (permalink / raw)
To: Paolo Bonzini, qemu-block; +Cc: kwolf, armbru, qemu-devel
On 01/12/2016 03:46 AM, Paolo Bonzini wrote:
>
>
> On 12/01/2016 01:36, 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.
>>
>> Extend the Notifier to include a job pointer, and include a reference
>> to the job registering the callback. This cuts out a few more cases
>> where we have to rely on bs->job.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>
> Why don't you just put the NotifierWithReturn inside the BackupBlockJob
> struct, and use container_of to get from NWR to BackupBlockJob?
>
> Paolo
>
That's another way (including the notifier within the job vs. including
the job within the notifier.) This one simply occurred to me first. Any
strong benefit to that method, or just a matter of style?
--js
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 4/5] block/backup: Add subclassed notifier
2016-01-12 17:57 ` John Snow
@ 2016-01-12 18:01 ` Paolo Bonzini
2016-01-12 18:02 ` John Snow
0 siblings, 1 reply; 13+ messages in thread
From: Paolo Bonzini @ 2016-01-12 18:01 UTC (permalink / raw)
To: John Snow, qemu-block; +Cc: kwolf, armbru, qemu-devel
On 12/01/2016 18:57, John Snow wrote:
>
>
> On 01/12/2016 03:46 AM, Paolo Bonzini wrote:
>>
>>
>> On 12/01/2016 01:36, 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.
>>>
>>> Extend the Notifier to include a job pointer, and include a reference
>>> to the job registering the callback. This cuts out a few more cases
>>> where we have to rely on bs->job.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>
>> Why don't you just put the NotifierWithReturn inside the BackupBlockJob
>> struct, and use container_of to get from NWR to BackupBlockJob?
>>
>> Paolo
>>
>
> That's another way (including the notifier within the job vs. including
> the job within the notifier.) This one simply occurred to me first. Any
> strong benefit to that method, or just a matter of style?
It's usually the one that is used with notifiers, no other reason.
Paolo
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 4/5] block/backup: Add subclassed notifier
2016-01-12 18:01 ` Paolo Bonzini
@ 2016-01-12 18:02 ` John Snow
2016-01-18 14:29 ` Kevin Wolf
0 siblings, 1 reply; 13+ messages in thread
From: John Snow @ 2016-01-12 18:02 UTC (permalink / raw)
To: Paolo Bonzini, qemu-block; +Cc: kwolf, armbru, qemu-devel
On 01/12/2016 01:01 PM, Paolo Bonzini wrote:
>
>
> On 12/01/2016 18:57, John Snow wrote:
>>
>>
>> On 01/12/2016 03:46 AM, Paolo Bonzini wrote:
>>>
>>>
>>> On 12/01/2016 01:36, 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.
>>>>
>>>> Extend the Notifier to include a job pointer, and include a reference
>>>> to the job registering the callback. This cuts out a few more cases
>>>> where we have to rely on bs->job.
>>>>
>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>
>>> Why don't you just put the NotifierWithReturn inside the BackupBlockJob
>>> struct, and use container_of to get from NWR to BackupBlockJob?
>>>
>>> Paolo
>>>
>>
>> That's another way (including the notifier within the job vs. including
>> the job within the notifier.) This one simply occurred to me first. Any
>> strong benefit to that method, or just a matter of style?
>
> It's usually the one that is used with notifiers, no other reason.
>
>
> Paolo
>
I'll follow convention, I just didn't bump into an example to model.
--js
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 5/5] blockjob: add Job parameter to BlockCompletionFunc
2016-01-12 0:36 ` [Qemu-devel] [PATCH 5/5] blockjob: add Job parameter to BlockCompletionFunc John Snow
@ 2016-01-18 14:25 ` Kevin Wolf
0 siblings, 0 replies; 13+ messages in thread
From: Kevin Wolf @ 2016-01-18 14:25 UTC (permalink / raw)
To: John Snow; +Cc: jcody, armbru, qemu-block, qemu-devel
Am 12.01.2016 um 01:36 hat John Snow geschrieben:
> 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, 39 insertions(+), 26 deletions(-)
>
> diff --git a/block/backup.c b/block/backup.c
> index 58c76be..cadb880 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -506,7 +506,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 a5d02aa..ef4fd5a 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -202,7 +202,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 92706ab..18134e4 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -714,7 +714,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)
> @@ -801,7 +801,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;
> @@ -826,7 +826,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 1dfeac0..1bd8220 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -216,7 +216,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 9b37ace..6713ecb 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2855,28 +2855,28 @@ 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;
> + BlockDriverState *bs = job->bs;
> const char *msg = NULL;
>
> - trace_block_job_cb(bs, bs->job, ret);
> + trace_block_job_cb(bs, job, ret);
You could directly use job->bs here, then the last user of the local
variable bs is gone.
> - assert(bs->job);
> + assert(job);
That's a bit late. If job == NULL, we have already segfaulted. I'd
suggest to just remove the assertion now that the job is directly
passed.
Kevin
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 4/5] block/backup: Add subclassed notifier
2016-01-12 18:02 ` John Snow
@ 2016-01-18 14:29 ` Kevin Wolf
2016-01-18 16:20 ` John Snow
0 siblings, 1 reply; 13+ messages in thread
From: Kevin Wolf @ 2016-01-18 14:29 UTC (permalink / raw)
To: John Snow; +Cc: Paolo Bonzini, armbru, qemu-block, qemu-devel
Am 12.01.2016 um 19:02 hat John Snow geschrieben:
>
>
> On 01/12/2016 01:01 PM, Paolo Bonzini wrote:
> >
> >
> > On 12/01/2016 18:57, John Snow wrote:
> >>
> >>
> >> On 01/12/2016 03:46 AM, Paolo Bonzini wrote:
> >>>
> >>>
> >>> On 12/01/2016 01:36, 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.
> >>>>
> >>>> Extend the Notifier to include a job pointer, and include a reference
> >>>> to the job registering the callback. This cuts out a few more cases
> >>>> where we have to rely on bs->job.
> >>>>
> >>>> Signed-off-by: John Snow <jsnow@redhat.com>
> >>>
> >>> Why don't you just put the NotifierWithReturn inside the BackupBlockJob
> >>> struct, and use container_of to get from NWR to BackupBlockJob?
> >>>
> >>> Paolo
> >>>
> >>
> >> That's another way (including the notifier within the job vs. including
> >> the job within the notifier.) This one simply occurred to me first. Any
> >> strong benefit to that method, or just a matter of style?
> >
> > It's usually the one that is used with notifiers, no other reason.
>
> I'll follow convention, I just didn't bump into an example to model.
This means that I should wait for a v2? (Hm, or is this Markus' area,
actually? Or Jeff's?)
Otherwise, this series is:
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Qemu-devel] [PATCH 4/5] block/backup: Add subclassed notifier
2016-01-18 14:29 ` Kevin Wolf
@ 2016-01-18 16:20 ` John Snow
0 siblings, 0 replies; 13+ messages in thread
From: John Snow @ 2016-01-18 16:20 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Paolo Bonzini, armbru, qemu-block, qemu-devel
On 01/18/2016 09:29 AM, Kevin Wolf wrote:
> Am 12.01.2016 um 19:02 hat John Snow geschrieben:
>>
>>
>> On 01/12/2016 01:01 PM, Paolo Bonzini wrote:
>>>
>>>
>>> On 12/01/2016 18:57, John Snow wrote:
>>>>
>>>>
>>>> On 01/12/2016 03:46 AM, Paolo Bonzini wrote:
>>>>>
>>>>>
>>>>> On 12/01/2016 01:36, 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.
>>>>>>
>>>>>> Extend the Notifier to include a job pointer, and include a reference
>>>>>> to the job registering the callback. This cuts out a few more cases
>>>>>> where we have to rely on bs->job.
>>>>>>
>>>>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>>>
>>>>> Why don't you just put the NotifierWithReturn inside the BackupBlockJob
>>>>> struct, and use container_of to get from NWR to BackupBlockJob?
>>>>>
>>>>> Paolo
>>>>>
>>>>
>>>> That's another way (including the notifier within the job vs. including
>>>> the job within the notifier.) This one simply occurred to me first. Any
>>>> strong benefit to that method, or just a matter of style?
>>>
>>> It's usually the one that is used with notifiers, no other reason.
>>
>> I'll follow convention, I just didn't bump into an example to model.
>
> This means that I should wait for a v2? (Hm, or is this Markus' area,
> actually? Or Jeff's?)
>
> Otherwise, this series is:
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
>
I hadn't re-rolled just yet, it seems like a matter of taste but I
usually defer to convention for predictability's sake. Waiting for Jeff,
primarily.
--js
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2016-01-18 16:21 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-12 0:36 [Qemu-devel] [PATCH 0/5] block: reduce reliance on bs->job pointer John Snow
2016-01-12 0:36 ` [Qemu-devel] [PATCH 1/5] block: Allow mirror_start to return job references John Snow
2016-01-12 0:36 ` [Qemu-devel] [PATCH 2/5] block: Allow stream_start " John Snow
2016-01-12 0:36 ` [Qemu-devel] [PATCH 3/5] block: allow backup_start " John Snow
2016-01-12 0:36 ` [Qemu-devel] [PATCH 4/5] block/backup: Add subclassed notifier John Snow
2016-01-12 8:46 ` Paolo Bonzini
2016-01-12 17:57 ` John Snow
2016-01-12 18:01 ` Paolo Bonzini
2016-01-12 18:02 ` John Snow
2016-01-18 14:29 ` Kevin Wolf
2016-01-18 16:20 ` John Snow
2016-01-12 0:36 ` [Qemu-devel] [PATCH 5/5] blockjob: add Job parameter to BlockCompletionFunc John Snow
2016-01-18 14:25 ` Kevin Wolf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.