* [Qemu-devel] [PATCH 0/7] blockjobs: preliminary refactoring work, Pt 1
@ 2016-10-13 22:56 John Snow
2016-10-13 22:56 ` [Qemu-devel] [PATCH 1/7] blockjobs: hide internal jobs from management API John Snow
` (8 more replies)
0 siblings, 9 replies; 26+ messages in thread
From: John Snow @ 2016-10-13 22:56 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, xiecl.fnst, wency, jcody, qemu-devel, pbonzini, John Snow
This is a follow-up to patches 1-6 of:
[PATCH v2 00/11] blockjobs: Fix transactional race condition
That series started trying to refactor blockjobs with the goal of
internalizing BlockJob state as a side effect of having gone through
the effort of figuring out which commands were "safe" to call on
a Job that has no coroutine object.
I've split out the less contentious bits so I can move forward with my
original work of focusing on the transactional race condition in a
different series.
Functionally the biggest difference here is the presence of "internal"
block jobs, which do not emit QMP events or show up in block query
requests. This is done for the sake of replication jobs, which should
not be interfering with the public jobs namespace.
________________________________________________________________________________
For convenience, this branch is available at:
https://github.com/jnsnow/qemu.git branch job-refactor-pt1
https://github.com/jnsnow/qemu/tree/job-refactor-pt1
This version is tagged job-refactor-pt1-v1:
https://github.com/jnsnow/qemu/releases/tag/job-refactor-pt1-v1
John Snow (7):
blockjobs: hide internal jobs from management API
blockjobs: Allow creating internal jobs
Replication/Blockjobs: Create replication jobs as internal
blockjob: centralize QMP event emissions
Blockjobs: Internalize user_pause logic
blockjobs: split interface into public/private, Part 1
blockjobs: fix documentation
block/backup.c | 5 +-
block/commit.c | 10 +-
block/mirror.c | 28 +++--
block/replication.c | 14 +--
block/stream.c | 9 +-
block/trace-events | 5 +-
blockdev.c | 74 +++++--------
blockjob.c | 109 ++++++++++++++----
include/block/block.h | 3 +-
include/block/block_int.h | 26 ++---
include/block/blockjob.h | 257 +++++++------------------------------------
include/block/blockjob_int.h | 232 ++++++++++++++++++++++++++++++++++++++
qemu-img.c | 5 +-
tests/test-blockjob-txn.c | 5 +-
tests/test-blockjob.c | 4 +-
15 files changed, 443 insertions(+), 343 deletions(-)
create mode 100644 include/block/blockjob_int.h
--
2.7.4
^ permalink raw reply [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH 1/7] blockjobs: hide internal jobs from management API
2016-10-13 22:56 [Qemu-devel] [PATCH 0/7] blockjobs: preliminary refactoring work, Pt 1 John Snow
@ 2016-10-13 22:56 ` John Snow
2016-10-14 12:58 ` Kevin Wolf
2016-10-13 22:56 ` [Qemu-devel] [PATCH 2/7] blockjobs: Allow creating internal jobs John Snow
` (7 subsequent siblings)
8 siblings, 1 reply; 26+ messages in thread
From: John Snow @ 2016-10-13 22:56 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, xiecl.fnst, wency, jcody, qemu-devel, pbonzini, John Snow
If jobs are not created directly by the user, do not allow them to be
seen by the user/management utility. At the moment, 'internal' jobs are
those that do not have an ID. As of this patch it is impossible to
create such jobs.
Signed-off-by: John Snow <jsnow@redhat.com>
---
blockdev.c | 17 +++++++++++++----
blockjob.c | 37 +++++++++++++++++++++++++++++++------
include/block/blockjob.h | 12 ++++++++++--
3 files changed, 54 insertions(+), 12 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 07ec733..5904edb 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3915,13 +3915,22 @@ BlockJobInfoList *qmp_query_block_jobs(Error **errp)
BlockJob *job;
for (job = block_job_next(NULL); job; job = block_job_next(job)) {
- BlockJobInfoList *elem = g_new0(BlockJobInfoList, 1);
- AioContext *aio_context = blk_get_aio_context(job->blk);
+ BlockJobInfoList *elem;
+ AioContext *aio_context;
+ if (block_job_is_internal(job)) {
+ continue;
+ }
+ elem = g_new0(BlockJobInfoList, 1);
+ aio_context = blk_get_aio_context(job->blk);
aio_context_acquire(aio_context);
- elem->value = block_job_query(job);
+ elem->value = block_job_query(job, errp);
aio_context_release(aio_context);
-
+ if (!elem->value) {
+ g_free(elem);
+ qapi_free_BlockJobInfoList(head);
+ return NULL;
+ }
*p_next = elem;
p_next = &elem->next;
}
diff --git a/blockjob.c b/blockjob.c
index 43fecbe..e78ad94 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -185,6 +185,11 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
return job;
}
+bool block_job_is_internal(BlockJob *job)
+{
+ return (job->id == NULL);
+}
+
void block_job_ref(BlockJob *job)
{
++job->refcnt;
@@ -494,9 +499,15 @@ void block_job_yield(BlockJob *job)
block_job_pause_point(job);
}
-BlockJobInfo *block_job_query(BlockJob *job)
+BlockJobInfo *block_job_query(BlockJob *job, Error **errp)
{
- BlockJobInfo *info = g_new0(BlockJobInfo, 1);
+ BlockJobInfo *info;
+
+ if (block_job_is_internal(job)) {
+ error_setg(errp, "Cannot query QEMU internal Jobs");
+ return NULL;
+ }
+ info = g_new0(BlockJobInfo, 1);
info->type = g_strdup(BlockJobType_lookup[job->driver->job_type]);
info->device = g_strdup(job->id);
info->len = job->len;
@@ -519,6 +530,10 @@ static void block_job_iostatus_set_err(BlockJob *job, int error)
void block_job_event_cancelled(BlockJob *job)
{
+ if (block_job_is_internal(job)) {
+ return;
+ }
+
qapi_event_send_block_job_cancelled(job->driver->job_type,
job->id,
job->len,
@@ -529,6 +544,10 @@ void block_job_event_cancelled(BlockJob *job)
void block_job_event_completed(BlockJob *job, const char *msg)
{
+ if (block_job_is_internal(job)) {
+ return;
+ }
+
qapi_event_send_block_job_completed(job->driver->job_type,
job->id,
job->len,
@@ -543,6 +562,10 @@ void block_job_event_ready(BlockJob *job)
{
job->ready = true;
+ if (block_job_is_internal(job)) {
+ return;
+ }
+
qapi_event_send_block_job_ready(job->driver->job_type,
job->id,
job->len,
@@ -573,10 +596,12 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockdevOnError on_err,
default:
abort();
}
- qapi_event_send_block_job_error(job->id,
- is_read ? IO_OPERATION_TYPE_READ :
- IO_OPERATION_TYPE_WRITE,
- action, &error_abort);
+ if (!block_job_is_internal(job)) {
+ qapi_event_send_block_job_error(job->id,
+ is_read ? IO_OPERATION_TYPE_READ :
+ IO_OPERATION_TYPE_WRITE,
+ action, &error_abort);
+ }
if (action == BLOCK_ERROR_ACTION_STOP) {
/* make the pause user visible, which will be resumed from QMP. */
job->user_paused = true;
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 4ddb4ae..6ecfa2e 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -107,7 +107,7 @@ struct BlockJob {
BlockBackend *blk;
/**
- * The ID of the block job.
+ * The ID of the block job. May be NULL for internal jobs.
*/
char *id;
@@ -333,7 +333,7 @@ bool block_job_is_cancelled(BlockJob *job);
*
* Return information about a job.
*/
-BlockJobInfo *block_job_query(BlockJob *job);
+BlockJobInfo *block_job_query(BlockJob *job, Error **errp);
/**
* block_job_pause_point:
@@ -504,4 +504,12 @@ void block_job_txn_unref(BlockJobTxn *txn);
*/
void block_job_txn_add_job(BlockJobTxn *txn, BlockJob *job);
+/**
+ * block_job_is_internal:
+ * @job: The job to determine if it is user-visible or not.
+ *
+ * Returns true if the job should not be visible to the management layer.
+ */
+bool block_job_is_internal(BlockJob *job);
+
#endif
--
2.7.4
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH 2/7] blockjobs: Allow creating internal jobs
2016-10-13 22:56 [Qemu-devel] [PATCH 0/7] blockjobs: preliminary refactoring work, Pt 1 John Snow
2016-10-13 22:56 ` [Qemu-devel] [PATCH 1/7] blockjobs: hide internal jobs from management API John Snow
@ 2016-10-13 22:56 ` John Snow
2016-10-14 14:40 ` Kevin Wolf
2016-10-26 4:48 ` Jeff Cody
2016-10-13 22:56 ` [Qemu-devel] [PATCH 3/7] Replication/Blockjobs: Create replication jobs as internal John Snow
` (6 subsequent siblings)
8 siblings, 2 replies; 26+ messages in thread
From: John Snow @ 2016-10-13 22:56 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, xiecl.fnst, wency, jcody, qemu-devel, pbonzini, John Snow
Add the ability to create jobs without an ID.
Signed-off-by: John Snow <jsnow@redhat.com>
---
block/backup.c | 2 +-
block/commit.c | 2 +-
block/mirror.c | 3 ++-
block/stream.c | 2 +-
blockjob.c | 25 ++++++++++++++++---------
include/block/blockjob.h | 7 ++++++-
tests/test-blockjob-txn.c | 3 ++-
tests/test-blockjob.c | 2 +-
8 files changed, 30 insertions(+), 16 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 582bd0f..5acb5c4 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -596,7 +596,7 @@ void backup_start(const char *job_id, BlockDriverState *bs,
}
job = block_job_create(job_id, &backup_job_driver, bs, speed,
- cb, opaque, errp);
+ BLOCK_JOB_DEFAULT, cb, opaque, errp);
if (!job) {
goto error;
}
diff --git a/block/commit.c b/block/commit.c
index 9f67a8b..f29e341 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -233,7 +233,7 @@ void commit_start(const char *job_id, BlockDriverState *bs,
}
s = block_job_create(job_id, &commit_job_driver, bs, speed,
- cb, opaque, errp);
+ BLOCK_JOB_DEFAULT, cb, opaque, errp);
if (!s) {
return;
}
diff --git a/block/mirror.c b/block/mirror.c
index f9d1fec..74c03ae 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -936,7 +936,8 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
buf_size = DEFAULT_MIRROR_BUF_SIZE;
}
- s = block_job_create(job_id, driver, bs, speed, cb, opaque, errp);
+ s = block_job_create(job_id, driver, bs, speed,
+ BLOCK_JOB_DEFAULT, cb, opaque, errp);
if (!s) {
return;
}
diff --git a/block/stream.c b/block/stream.c
index 3187481..eeb6f52 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -222,7 +222,7 @@ void stream_start(const char *job_id, BlockDriverState *bs,
StreamBlockJob *s;
s = block_job_create(job_id, &stream_job_driver, bs, speed,
- cb, opaque, errp);
+ BLOCK_JOB_DEFAULT, cb, opaque, errp);
if (!s) {
return;
}
diff --git a/blockjob.c b/blockjob.c
index e78ad94..017905a 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -118,7 +118,7 @@ static void block_job_detach_aio_context(void *opaque)
}
void *block_job_create(const char *job_id, const BlockJobDriver *driver,
- BlockDriverState *bs, int64_t speed,
+ BlockDriverState *bs, int64_t speed, int flags,
BlockCompletionFunc *cb, void *opaque, Error **errp)
{
BlockBackend *blk;
@@ -130,7 +130,7 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
return NULL;
}
- if (job_id == NULL) {
+ if (job_id == NULL && !(flags & BLOCK_JOB_INTERNAL)) {
job_id = bdrv_get_device_name(bs);
if (!*job_id) {
error_setg(errp, "An explicit job ID is required for this node");
@@ -138,14 +138,21 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
}
}
- if (!id_wellformed(job_id)) {
- error_setg(errp, "Invalid job ID '%s'", job_id);
- return NULL;
- }
+ if (job_id) {
+ if (flags & BLOCK_JOB_INTERNAL) {
+ error_setg(errp, "Cannot specify job ID for internal block job");
+ return NULL;
+ }
- if (block_job_get(job_id)) {
- error_setg(errp, "Job ID '%s' already in use", job_id);
- return NULL;
+ if (!id_wellformed(job_id)) {
+ error_setg(errp, "Invalid job ID '%s'", job_id);
+ return NULL;
+ }
+
+ if (block_job_get(job_id)) {
+ error_setg(errp, "Job ID '%s' already in use", job_id);
+ return NULL;
+ }
}
blk = blk_new();
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 6ecfa2e..fdb31e0 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -200,6 +200,11 @@ struct BlockJob {
QLIST_ENTRY(BlockJob) txn_list;
};
+typedef enum BlockJobCreateFlags {
+ BLOCK_JOB_DEFAULT = 0x00,
+ BLOCK_JOB_INTERNAL = 0x01,
+} BlockJobCreateFlags;
+
/**
* block_job_next:
* @job: A block job, or %NULL.
@@ -242,7 +247,7 @@ BlockJob *block_job_get(const char *id);
* called from a wrapper that is specific to the job type.
*/
void *block_job_create(const char *job_id, const BlockJobDriver *driver,
- BlockDriverState *bs, int64_t speed,
+ BlockDriverState *bs, int64_t speed, int flags,
BlockCompletionFunc *cb, void *opaque, Error **errp);
/**
diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
index d049cba..b79e0c6 100644
--- a/tests/test-blockjob-txn.c
+++ b/tests/test-blockjob-txn.c
@@ -98,7 +98,8 @@ static BlockJob *test_block_job_start(unsigned int iterations,
bs = bdrv_new();
snprintf(job_id, sizeof(job_id), "job%u", counter++);
s = block_job_create(job_id, &test_block_job_driver, bs, 0,
- test_block_job_cb, data, &error_abort);
+ BLOCK_JOB_DEFAULT, test_block_job_cb,
+ data, &error_abort);
s->iterations = iterations;
s->use_timer = use_timer;
s->rc = rc;
diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
index 5b0e934..18bf850 100644
--- a/tests/test-blockjob.c
+++ b/tests/test-blockjob.c
@@ -31,7 +31,7 @@ static BlockJob *do_test_id(BlockBackend *blk, const char *id,
Error *errp = NULL;
job = block_job_create(id, &test_block_job_driver, blk_bs(blk), 0,
- block_job_cb, NULL, &errp);
+ BLOCK_JOB_DEFAULT, block_job_cb, NULL, &errp);
if (should_succeed) {
g_assert_null(errp);
g_assert_nonnull(job);
--
2.7.4
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH 3/7] Replication/Blockjobs: Create replication jobs as internal
2016-10-13 22:56 [Qemu-devel] [PATCH 0/7] blockjobs: preliminary refactoring work, Pt 1 John Snow
2016-10-13 22:56 ` [Qemu-devel] [PATCH 1/7] blockjobs: hide internal jobs from management API John Snow
2016-10-13 22:56 ` [Qemu-devel] [PATCH 2/7] blockjobs: Allow creating internal jobs John Snow
@ 2016-10-13 22:56 ` John Snow
2016-10-14 14:40 ` Kevin Wolf
2016-10-26 4:48 ` Jeff Cody
2016-10-13 22:56 ` [Qemu-devel] [PATCH 4/7] blockjob: centralize QMP event emissions John Snow
` (5 subsequent siblings)
8 siblings, 2 replies; 26+ messages in thread
From: John Snow @ 2016-10-13 22:56 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, xiecl.fnst, wency, jcody, qemu-devel, pbonzini, John Snow
Bubble up the internal interface to commit and backup jobs, then switch
replication tasks over to using this methodology.
Signed-off-by: John Snow <jsnow@redhat.com>
---
block/backup.c | 3 ++-
block/mirror.c | 21 ++++++++++-----------
block/replication.c | 14 +++++++-------
blockdev.c | 11 +++++++----
include/block/block_int.h | 9 +++++++--
qemu-img.c | 5 +++--
6 files changed, 36 insertions(+), 27 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 5acb5c4..6a60ca8 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -527,6 +527,7 @@ void backup_start(const char *job_id, BlockDriverState *bs,
bool compress,
BlockdevOnError on_source_error,
BlockdevOnError on_target_error,
+ int creation_flags,
BlockCompletionFunc *cb, void *opaque,
BlockJobTxn *txn, Error **errp)
{
@@ -596,7 +597,7 @@ void backup_start(const char *job_id, BlockDriverState *bs,
}
job = block_job_create(job_id, &backup_job_driver, bs, speed,
- BLOCK_JOB_DEFAULT, cb, opaque, errp);
+ creation_flags, cb, opaque, errp);
if (!job) {
goto error;
}
diff --git a/block/mirror.c b/block/mirror.c
index 74c03ae..15d2d10 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -906,9 +906,9 @@ static const BlockJobDriver commit_active_job_driver = {
};
static void mirror_start_job(const char *job_id, BlockDriverState *bs,
- BlockDriverState *target, const char *replaces,
- int64_t speed, uint32_t granularity,
- int64_t buf_size,
+ int creation_flags, BlockDriverState *target,
+ const char *replaces, int64_t speed,
+ uint32_t granularity, int64_t buf_size,
BlockMirrorBackingMode backing_mode,
BlockdevOnError on_source_error,
BlockdevOnError on_target_error,
@@ -936,8 +936,8 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
buf_size = DEFAULT_MIRROR_BUF_SIZE;
}
- s = block_job_create(job_id, driver, bs, speed,
- BLOCK_JOB_DEFAULT, cb, opaque, errp);
+ s = block_job_create(job_id, driver, bs, speed, creation_flags,
+ cb, opaque, errp);
if (!s) {
return;
}
@@ -992,17 +992,16 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
}
is_none_mode = mode == MIRROR_SYNC_MODE_NONE;
base = mode == MIRROR_SYNC_MODE_TOP ? backing_bs(bs) : NULL;
- mirror_start_job(job_id, bs, target, replaces,
+ mirror_start_job(job_id, bs, BLOCK_JOB_DEFAULT, target, replaces,
speed, granularity, buf_size, backing_mode,
on_source_error, on_target_error, unmap, cb, opaque, errp,
&mirror_job_driver, is_none_mode, base, false);
}
void commit_active_start(const char *job_id, BlockDriverState *bs,
- BlockDriverState *base, int64_t speed,
- BlockdevOnError on_error,
- BlockCompletionFunc *cb,
- void *opaque, Error **errp,
+ BlockDriverState *base, int creation_flags,
+ int64_t speed, BlockdevOnError on_error,
+ BlockCompletionFunc *cb, void *opaque, Error **errp,
bool auto_complete)
{
int64_t length, base_length;
@@ -1041,7 +1040,7 @@ void commit_active_start(const char *job_id, BlockDriverState *bs,
}
}
- mirror_start_job(job_id, bs, base, NULL, speed, 0, 0,
+ mirror_start_job(job_id, bs, creation_flags, base, NULL, speed, 0, 0,
MIRROR_LEAVE_BACKING_CHAIN,
on_error, on_error, false, cb, opaque, &local_err,
&commit_active_job_driver, false, base, auto_complete);
diff --git a/block/replication.c b/block/replication.c
index 3bd1cf1..d4f4a7b 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -496,10 +496,11 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
bdrv_op_block_all(top_bs, s->blocker);
bdrv_op_unblock(top_bs, BLOCK_OP_TYPE_DATAPLANE, s->blocker);
- backup_start("replication-backup", s->secondary_disk->bs,
- s->hidden_disk->bs, 0, MIRROR_SYNC_MODE_NONE, NULL, false,
+ backup_start(NULL, s->secondary_disk->bs, s->hidden_disk->bs, 0,
+ MIRROR_SYNC_MODE_NONE, NULL, false,
BLOCKDEV_ON_ERROR_REPORT, BLOCKDEV_ON_ERROR_REPORT,
- backup_job_completed, s, NULL, &local_err);
+ BLOCK_JOB_INTERNAL, backup_job_completed, s,
+ NULL, &local_err);
if (local_err) {
error_propagate(errp, local_err);
backup_job_cleanup(s);
@@ -621,10 +622,9 @@ static void replication_stop(ReplicationState *rs, bool failover, Error **errp)
}
s->replication_state = BLOCK_REPLICATION_FAILOVER;
- commit_active_start("replication-commit", s->active_disk->bs,
- s->secondary_disk->bs, 0, BLOCKDEV_ON_ERROR_REPORT,
- replication_done,
- bs, errp, true);
+ commit_active_start(NULL, s->active_disk->bs, s->secondary_disk->bs,
+ BLOCK_JOB_INTERNAL, 0, BLOCKDEV_ON_ERROR_REPORT,
+ replication_done, bs, errp, true);
break;
default:
aio_context_release(aio_context);
diff --git a/blockdev.c b/blockdev.c
index 5904edb..0ce305c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3083,8 +3083,9 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
" but 'top' is the active layer");
goto out;
}
- commit_active_start(has_job_id ? job_id : NULL, bs, base_bs, speed,
- on_error, block_job_cb, bs, &local_err, false);
+ commit_active_start(has_job_id ? job_id : NULL, bs, base_bs,
+ BLOCK_JOB_DEFAULT, speed, on_error, block_job_cb,
+ bs, &local_err, false);
} else {
commit_start(has_job_id ? job_id : NULL, bs, base_bs, top_bs, speed,
on_error, block_job_cb, bs,
@@ -3208,7 +3209,8 @@ static void do_drive_backup(DriveBackup *backup, BlockJobTxn *txn, Error **errp)
backup_start(backup->job_id, bs, target_bs, backup->speed, backup->sync,
bmap, backup->compress, backup->on_source_error,
- backup->on_target_error, block_job_cb, bs, txn, &local_err);
+ backup->on_target_error, BLOCK_JOB_DEFAULT,
+ block_job_cb, bs, txn, &local_err);
bdrv_unref(target_bs);
if (local_err != NULL) {
error_propagate(errp, local_err);
@@ -3278,7 +3280,8 @@ void do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn, Error **errp)
}
backup_start(backup->job_id, bs, target_bs, backup->speed, backup->sync,
NULL, backup->compress, backup->on_source_error,
- backup->on_target_error, block_job_cb, bs, txn, &local_err);
+ backup->on_target_error, BLOCK_JOB_DEFAULT,
+ block_job_cb, bs, txn, &local_err);
if (local_err != NULL) {
error_propagate(errp, local_err);
}
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 3e79228..98f1c7f 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -688,6 +688,8 @@ void commit_start(const char *job_id, BlockDriverState *bs,
* device name of @bs.
* @bs: Active block device to be committed.
* @base: Block device that will be written into, and become the new top.
+ * @creation_flags: Flags that control the behavior of the Job lifetime.
+ * See @BlockJobCreateFlags
* @speed: The maximum speed, in bytes per second, or 0 for unlimited.
* @on_error: The action to take upon error.
* @cb: Completion function for the job.
@@ -697,8 +699,8 @@ void commit_start(const char *job_id, BlockDriverState *bs,
*
*/
void commit_active_start(const char *job_id, BlockDriverState *bs,
- BlockDriverState *base, int64_t speed,
- BlockdevOnError on_error,
+ BlockDriverState *base, int creation_flags,
+ int64_t speed, BlockdevOnError on_error,
BlockCompletionFunc *cb,
void *opaque, Error **errp, bool auto_complete);
/*
@@ -747,6 +749,8 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
* @sync_bitmap: The dirty bitmap if sync_mode is MIRROR_SYNC_MODE_INCREMENTAL.
* @on_source_error: The action to take upon error reading from the source.
* @on_target_error: The action to take upon error writing to the target.
+ * @creation_flags: Flags that control the behavior of the Job lifetime.
+ * See @BlockJobCreateFlags
* @cb: Completion function for the job.
* @opaque: Opaque pointer value passed to @cb.
* @txn: Transaction that this job is part of (may be NULL).
@@ -760,6 +764,7 @@ void backup_start(const char *job_id, BlockDriverState *bs,
bool compress,
BlockdevOnError on_source_error,
BlockdevOnError on_target_error,
+ int creation_flags,
BlockCompletionFunc *cb, void *opaque,
BlockJobTxn *txn, Error **errp);
diff --git a/qemu-img.c b/qemu-img.c
index 02c07b9..3897d82 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -928,8 +928,9 @@ static int img_commit(int argc, char **argv)
.bs = bs,
};
- commit_active_start("commit", bs, base_bs, 0, BLOCKDEV_ON_ERROR_REPORT,
- common_block_job_cb, &cbi, &local_err, false);
+ commit_active_start("commit", bs, base_bs, BLOCK_JOB_DEFAULT, 0,
+ BLOCKDEV_ON_ERROR_REPORT, common_block_job_cb, &cbi,
+ &local_err, false);
if (local_err) {
goto done;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH 4/7] blockjob: centralize QMP event emissions
2016-10-13 22:56 [Qemu-devel] [PATCH 0/7] blockjobs: preliminary refactoring work, Pt 1 John Snow
` (2 preceding siblings ...)
2016-10-13 22:56 ` [Qemu-devel] [PATCH 3/7] Replication/Blockjobs: Create replication jobs as internal John Snow
@ 2016-10-13 22:56 ` John Snow
2016-10-14 15:45 ` Kevin Wolf
2016-10-26 4:49 ` Jeff Cody
2016-10-13 22:57 ` [Qemu-devel] [PATCH 5/7] Blockjobs: Internalize user_pause logic John Snow
` (4 subsequent siblings)
8 siblings, 2 replies; 26+ messages in thread
From: John Snow @ 2016-10-13 22:56 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, xiecl.fnst, wency, jcody, qemu-devel, pbonzini, John Snow
There's no reason to leave this to blockdev; we can do it in blockjobs
directly and get rid of an extra callback for most users.
All non-internal events, even those created outside of QMP, will
consistently emit events.
Signed-off-by: John Snow <jsnow@redhat.com>
---
block/commit.c | 8 ++++----
block/mirror.c | 6 ++----
block/stream.c | 7 +++----
block/trace-events | 5 ++---
blockdev.c | 42 ++++++++----------------------------------
blockjob.c | 23 +++++++++++++++++++----
include/block/block_int.h | 17 ++++-------------
include/block/blockjob.h | 17 -----------------
8 files changed, 42 insertions(+), 83 deletions(-)
diff --git a/block/commit.c b/block/commit.c
index f29e341..475a375 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -209,8 +209,8 @@ static const BlockJobDriver commit_job_driver = {
void commit_start(const char *job_id, BlockDriverState *bs,
BlockDriverState *base, BlockDriverState *top, int64_t speed,
- BlockdevOnError on_error, BlockCompletionFunc *cb,
- void *opaque, const char *backing_file_str, Error **errp)
+ BlockdevOnError on_error, const char *backing_file_str,
+ Error **errp)
{
CommitBlockJob *s;
BlockReopenQueue *reopen_queue = NULL;
@@ -233,7 +233,7 @@ void commit_start(const char *job_id, BlockDriverState *bs,
}
s = block_job_create(job_id, &commit_job_driver, bs, speed,
- BLOCK_JOB_DEFAULT, cb, opaque, errp);
+ BLOCK_JOB_DEFAULT, NULL, NULL, errp);
if (!s) {
return;
}
@@ -276,7 +276,7 @@ void commit_start(const char *job_id, BlockDriverState *bs,
s->on_error = on_error;
s->common.co = qemu_coroutine_create(commit_run, s);
- trace_commit_start(bs, base, top, s, s->common.co, opaque);
+ trace_commit_start(bs, base, top, s, s->common.co);
qemu_coroutine_enter(s->common.co);
}
diff --git a/block/mirror.c b/block/mirror.c
index 15d2d10..4374fb4 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -979,9 +979,7 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
MirrorSyncMode mode, BlockMirrorBackingMode backing_mode,
BlockdevOnError on_source_error,
BlockdevOnError on_target_error,
- bool unmap,
- BlockCompletionFunc *cb,
- void *opaque, Error **errp)
+ bool unmap, Error **errp)
{
bool is_none_mode;
BlockDriverState *base;
@@ -994,7 +992,7 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
base = mode == MIRROR_SYNC_MODE_TOP ? backing_bs(bs) : NULL;
mirror_start_job(job_id, bs, BLOCK_JOB_DEFAULT, target, replaces,
speed, granularity, buf_size, backing_mode,
- on_source_error, on_target_error, unmap, cb, opaque, errp,
+ on_source_error, on_target_error, unmap, NULL, NULL, errp,
&mirror_job_driver, is_none_mode, base, false);
}
diff --git a/block/stream.c b/block/stream.c
index eeb6f52..7d6877d 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -216,13 +216,12 @@ static const BlockJobDriver stream_job_driver = {
void stream_start(const char *job_id, BlockDriverState *bs,
BlockDriverState *base, const char *backing_file_str,
- int64_t speed, BlockdevOnError on_error,
- BlockCompletionFunc *cb, void *opaque, Error **errp)
+ int64_t speed, BlockdevOnError on_error, Error **errp)
{
StreamBlockJob *s;
s = block_job_create(job_id, &stream_job_driver, bs, speed,
- BLOCK_JOB_DEFAULT, cb, opaque, errp);
+ BLOCK_JOB_DEFAULT, NULL, NULL, errp);
if (!s) {
return;
}
@@ -232,6 +231,6 @@ void stream_start(const char *job_id, BlockDriverState *bs,
s->on_error = on_error;
s->common.co = qemu_coroutine_create(stream_run, s);
- trace_stream_start(bs, base, s, s->common.co, opaque);
+ trace_stream_start(bs, base, s, s->common.co);
qemu_coroutine_enter(s->common.co);
}
diff --git a/block/trace-events b/block/trace-events
index 05fa13c..c12f91b 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -20,11 +20,11 @@ bdrv_co_do_copy_on_readv(void *bs, int64_t offset, unsigned int bytes, int64_t c
# block/stream.c
stream_one_iteration(void *s, int64_t sector_num, int nb_sectors, int is_allocated) "s %p sector_num %"PRId64" nb_sectors %d is_allocated %d"
-stream_start(void *bs, void *base, void *s, void *co, void *opaque) "bs %p base %p s %p co %p opaque %p"
+stream_start(void *bs, void *base, void *s, void *co) "bs %p base %p s %p co %p"
# block/commit.c
commit_one_iteration(void *s, int64_t sector_num, int nb_sectors, int is_allocated) "s %p sector_num %"PRId64" nb_sectors %d is_allocated %d"
-commit_start(void *bs, void *base, void *top, void *s, void *co, void *opaque) "bs %p base %p top %p s %p co %p opaque %p"
+commit_start(void *bs, void *base, void *top, void *s, void *co) "bs %p base %p top %p s %p co %p"
# block/mirror.c
mirror_start(void *bs, void *s, void *co, void *opaque) "bs %p s %p co %p opaque %p"
@@ -52,7 +52,6 @@ qmp_block_job_cancel(void *job) "job %p"
qmp_block_job_pause(void *job) "job %p"
qmp_block_job_resume(void *job) "job %p"
qmp_block_job_complete(void *job) "job %p"
-block_job_cb(void *bs, void *job, int ret) "bs %p job %p ret %d"
qmp_block_stream(void *bs, void *job) "bs %p job %p"
# block/raw-win32.c
diff --git a/blockdev.c b/blockdev.c
index 0ce305c..22a1280 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2905,31 +2905,6 @@ out:
aio_context_release(aio_context);
}
-static void block_job_cb(void *opaque, 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);
-
- if (ret < 0) {
- msg = strerror(-ret);
- }
-
- if (block_job_is_cancelled(bs->job)) {
- block_job_event_cancelled(bs->job);
- } else {
- block_job_event_completed(bs->job, msg);
- }
-}
-
void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
bool has_base, const char *base,
bool has_backing_file, const char *backing_file,
@@ -2981,7 +2956,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
base_name = has_backing_file ? backing_file : base_name;
stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
- has_speed ? speed : 0, on_error, block_job_cb, bs, &local_err);
+ has_speed ? speed : 0, on_error, &local_err);
if (local_err) {
error_propagate(errp, local_err);
goto out;
@@ -3084,12 +3059,12 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
goto out;
}
commit_active_start(has_job_id ? job_id : NULL, bs, base_bs,
- BLOCK_JOB_DEFAULT, speed, on_error, block_job_cb,
- bs, &local_err, false);
+ BLOCK_JOB_DEFAULT, speed, on_error, NULL, NULL,
+ &local_err, false);
} else {
commit_start(has_job_id ? job_id : NULL, bs, base_bs, top_bs, speed,
- on_error, block_job_cb, bs,
- has_backing_file ? backing_file : NULL, &local_err);
+ on_error, has_backing_file ? backing_file : NULL,
+ &local_err);
}
if (local_err != NULL) {
error_propagate(errp, local_err);
@@ -3210,7 +3185,7 @@ static void do_drive_backup(DriveBackup *backup, BlockJobTxn *txn, Error **errp)
backup_start(backup->job_id, bs, target_bs, backup->speed, backup->sync,
bmap, backup->compress, backup->on_source_error,
backup->on_target_error, BLOCK_JOB_DEFAULT,
- block_job_cb, bs, txn, &local_err);
+ NULL, NULL, txn, &local_err);
bdrv_unref(target_bs);
if (local_err != NULL) {
error_propagate(errp, local_err);
@@ -3281,7 +3256,7 @@ void do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn, Error **errp)
backup_start(backup->job_id, bs, target_bs, backup->speed, backup->sync,
NULL, backup->compress, backup->on_source_error,
backup->on_target_error, BLOCK_JOB_DEFAULT,
- block_job_cb, bs, txn, &local_err);
+ NULL, NULL, txn, &local_err);
if (local_err != NULL) {
error_propagate(errp, local_err);
}
@@ -3360,8 +3335,7 @@ static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
mirror_start(job_id, bs, target,
has_replaces ? replaces : NULL,
speed, granularity, buf_size, sync, backing_mode,
- on_source_error, on_target_error, unmap,
- block_job_cb, bs, errp);
+ on_source_error, on_target_error, unmap, errp);
}
void qmp_drive_mirror(DriveMirror *arg, Error **errp)
diff --git a/blockjob.c b/blockjob.c
index 017905a..e32cb78 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -38,6 +38,9 @@
#include "qemu/timer.h"
#include "qapi-event.h"
+static void block_job_event_cancelled(BlockJob *job);
+static void block_job_event_completed(BlockJob *job, const char *msg);
+
/* Transactional group of block jobs */
struct BlockJobTxn {
@@ -124,7 +127,6 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
BlockBackend *blk;
BlockJob *job;
- assert(cb);
if (bs->job) {
error_setg(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
return NULL;
@@ -230,7 +232,20 @@ static void block_job_completed_single(BlockJob *job)
job->driver->abort(job);
}
}
- job->cb(job->opaque, job->ret);
+
+ if (job->cb) {
+ job->cb(job->opaque, job->ret);
+ }
+ if (block_job_is_cancelled(job)) {
+ block_job_event_cancelled(job);
+ } else {
+ const char *msg = NULL;
+ if (job->ret < 0) {
+ msg = strerror(-job->ret);
+ }
+ block_job_event_completed(job, msg);
+ }
+
if (job->txn) {
block_job_txn_unref(job->txn);
}
@@ -535,7 +550,7 @@ static void block_job_iostatus_set_err(BlockJob *job, int error)
}
}
-void block_job_event_cancelled(BlockJob *job)
+static void block_job_event_cancelled(BlockJob *job)
{
if (block_job_is_internal(job)) {
return;
@@ -549,7 +564,7 @@ void block_job_event_cancelled(BlockJob *job)
&error_abort);
}
-void block_job_event_completed(BlockJob *job, const char *msg)
+static void block_job_event_completed(BlockJob *job, const char *msg)
{
if (block_job_is_internal(job)) {
return;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 98f1c7f..dfbc53d 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -647,8 +647,6 @@ int is_windows_drive(const char *filename);
* the new backing file if the job completes. Ignored if @base is %NULL.
* @speed: The maximum speed, in bytes per second, or 0 for unlimited.
* @on_error: The action to take upon error.
- * @cb: Completion function for the job.
- * @opaque: Opaque pointer value passed to @cb.
* @errp: Error object.
*
* Start a streaming operation on @bs. Clusters that are unallocated
@@ -660,8 +658,7 @@ int is_windows_drive(const char *filename);
*/
void stream_start(const char *job_id, BlockDriverState *bs,
BlockDriverState *base, const char *backing_file_str,
- int64_t speed, BlockdevOnError on_error,
- BlockCompletionFunc *cb, void *opaque, Error **errp);
+ int64_t speed, BlockdevOnError on_error, Error **errp);
/**
* commit_start:
@@ -672,16 +669,14 @@ void stream_start(const char *job_id, BlockDriverState *bs,
* @base: Block device that will be written into, and become the new top.
* @speed: The maximum speed, in bytes per second, or 0 for unlimited.
* @on_error: The action to take upon error.
- * @cb: Completion function for the job.
- * @opaque: Opaque pointer value passed to @cb.
* @backing_file_str: String to use as the backing file in @top's overlay
* @errp: Error object.
*
*/
void commit_start(const char *job_id, BlockDriverState *bs,
BlockDriverState *base, BlockDriverState *top, int64_t speed,
- BlockdevOnError on_error, BlockCompletionFunc *cb,
- void *opaque, const char *backing_file_str, Error **errp);
+ BlockdevOnError on_error, const char *backing_file_str,
+ Error **errp);
/**
* commit_active_start:
* @job_id: The id of the newly-created job, or %NULL to use the
@@ -719,8 +714,6 @@ void commit_active_start(const char *job_id, BlockDriverState *bs,
* @on_source_error: The action to take upon error reading from the source.
* @on_target_error: The action to take upon error writing to the target.
* @unmap: Whether to unmap target where source sectors only contain zeroes.
- * @cb: Completion function for the job.
- * @opaque: Opaque pointer value passed to @cb.
* @errp: Error object.
*
* Start a mirroring operation on @bs. Clusters that are allocated
@@ -734,9 +727,7 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
MirrorSyncMode mode, BlockMirrorBackingMode backing_mode,
BlockdevOnError on_source_error,
BlockdevOnError on_target_error,
- bool unmap,
- BlockCompletionFunc *cb,
- void *opaque, Error **errp);
+ bool unmap, Error **errp);
/*
* backup_start:
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index fdb31e0..928f0b8 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -374,23 +374,6 @@ void block_job_resume(BlockJob *job);
void block_job_enter(BlockJob *job);
/**
- * block_job_event_cancelled:
- * @job: The job whose information is requested.
- *
- * Send a BLOCK_JOB_CANCELLED event for the specified job.
- */
-void block_job_event_cancelled(BlockJob *job);
-
-/**
- * block_job_ready:
- * @job: The job which is now ready to complete.
- * @msg: Error message. Only present on failure.
- *
- * Send a BLOCK_JOB_COMPLETED event for the specified job.
- */
-void block_job_event_completed(BlockJob *job, const char *msg);
-
-/**
* block_job_ready:
* @job: The job which is now ready to complete.
*
--
2.7.4
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH 5/7] Blockjobs: Internalize user_pause logic
2016-10-13 22:56 [Qemu-devel] [PATCH 0/7] blockjobs: preliminary refactoring work, Pt 1 John Snow
` (3 preceding siblings ...)
2016-10-13 22:56 ` [Qemu-devel] [PATCH 4/7] blockjob: centralize QMP event emissions John Snow
@ 2016-10-13 22:57 ` John Snow
2016-10-14 15:46 ` Kevin Wolf
2016-10-26 4:50 ` Jeff Cody
2016-10-13 22:57 ` [Qemu-devel] [PATCH 6/7] blockjobs: split interface into public/private, Part 1 John Snow
` (3 subsequent siblings)
8 siblings, 2 replies; 26+ messages in thread
From: John Snow @ 2016-10-13 22:57 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, xiecl.fnst, wency, jcody, qemu-devel, pbonzini, John Snow
BlockJobs will begin hiding their state in preparation for some
refactorings anyway, so let's internalize the user_pause mechanism
instead of leaving it to callers to correctly manage.
Signed-off-by: John Snow <jsnow@redhat.com>
---
blockdev.c | 12 +++++-------
blockjob.c | 22 ++++++++++++++++++++--
include/block/blockjob.h | 26 ++++++++++++++++++++++++++
3 files changed, 51 insertions(+), 9 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 22a1280..1661d08 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3579,7 +3579,7 @@ void qmp_block_job_cancel(const char *device,
force = false;
}
- if (job->user_paused && !force) {
+ if (block_job_user_paused(job) && !force) {
error_setg(errp, "The block job for device '%s' is currently paused",
device);
goto out;
@@ -3596,13 +3596,12 @@ void qmp_block_job_pause(const char *device, Error **errp)
AioContext *aio_context;
BlockJob *job = find_block_job(device, &aio_context, errp);
- if (!job || job->user_paused) {
+ if (!job || block_job_user_paused(job)) {
return;
}
- job->user_paused = true;
trace_qmp_block_job_pause(job);
- block_job_pause(job);
+ block_job_user_pause(job);
aio_context_release(aio_context);
}
@@ -3611,14 +3610,13 @@ void qmp_block_job_resume(const char *device, Error **errp)
AioContext *aio_context;
BlockJob *job = find_block_job(device, &aio_context, errp);
- if (!job || !job->user_paused) {
+ if (!job || !block_job_user_paused(job)) {
return;
}
- job->user_paused = false;
trace_qmp_block_job_resume(job);
block_job_iostatus_reset(job);
- block_job_resume(job);
+ block_job_user_resume(job);
aio_context_release(aio_context);
}
diff --git a/blockjob.c b/blockjob.c
index e32cb78..d118a1f 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -362,11 +362,22 @@ void block_job_pause(BlockJob *job)
job->pause_count++;
}
+void block_job_user_pause(BlockJob *job)
+{
+ job->user_paused = true;
+ block_job_pause(job);
+}
+
static bool block_job_should_pause(BlockJob *job)
{
return job->pause_count > 0;
}
+bool block_job_user_paused(BlockJob *job)
+{
+ return job ? job->user_paused : 0;
+}
+
void coroutine_fn block_job_pause_point(BlockJob *job)
{
if (!block_job_should_pause(job)) {
@@ -403,6 +414,14 @@ void block_job_resume(BlockJob *job)
block_job_enter(job);
}
+void block_job_user_resume(BlockJob *job)
+{
+ if (job && job->user_paused && job->pause_count > 0) {
+ job->user_paused = false;
+ block_job_resume(job);
+ }
+}
+
void block_job_enter(BlockJob *job)
{
if (job->co && !job->busy) {
@@ -626,8 +645,7 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockdevOnError on_err,
}
if (action == BLOCK_ERROR_ACTION_STOP) {
/* make the pause user visible, which will be resumed from QMP. */
- job->user_paused = true;
- block_job_pause(job);
+ block_job_user_pause(job);
block_job_iostatus_set_err(job, error);
}
return action;
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 928f0b8..5b61140 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -358,6 +358,23 @@ void coroutine_fn block_job_pause_point(BlockJob *job);
void block_job_pause(BlockJob *job);
/**
+ * block_job_user_pause:
+ * @job: The job to be paused.
+ *
+ * Asynchronously pause the specified job.
+ * Do not allow a resume until a matching call to block_job_user_resume.
+ */
+void block_job_user_pause(BlockJob *job);
+
+/**
+ * block_job_paused:
+ * @job: The job to query.
+ *
+ * Returns true if the job is user-paused.
+ */
+bool block_job_user_paused(BlockJob *job);
+
+/**
* block_job_resume:
* @job: The job to be resumed.
*
@@ -366,6 +383,15 @@ void block_job_pause(BlockJob *job);
void block_job_resume(BlockJob *job);
/**
+ * block_job_user_resume:
+ * @job: The job to be resumed.
+ *
+ * Resume the specified job.
+ * Must be paired with a preceding block_job_user_pause.
+ */
+void block_job_user_resume(BlockJob *job);
+
+/**
* block_job_enter:
* @job: The job to enter.
*
--
2.7.4
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH 6/7] blockjobs: split interface into public/private, Part 1
2016-10-13 22:56 [Qemu-devel] [PATCH 0/7] blockjobs: preliminary refactoring work, Pt 1 John Snow
` (4 preceding siblings ...)
2016-10-13 22:57 ` [Qemu-devel] [PATCH 5/7] Blockjobs: Internalize user_pause logic John Snow
@ 2016-10-13 22:57 ` John Snow
2016-10-14 15:46 ` Kevin Wolf
2016-10-26 4:51 ` Jeff Cody
2016-10-13 22:57 ` [Qemu-devel] [PATCH 7/7] blockjobs: fix documentation John Snow
` (2 subsequent siblings)
8 siblings, 2 replies; 26+ messages in thread
From: John Snow @ 2016-10-13 22:57 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, xiecl.fnst, wency, jcody, qemu-devel, pbonzini, John Snow
To make it a little more obvious which functions are intended to be
public interface and which are intended to be for use only by jobs
themselves, split the interface into "public" and "private" files.
Convert blockjobs (e.g. block/backup) to using the private interface.
Leave blockdev and others on the public interface.
There are remaining uses of private state by qemu-img, and several
cases in blockdev.c and block/io.c where we grab job->blk for the
purposes of acquiring an AIOContext.
These will be corrected in future patches.
Signed-off-by: John Snow <jsnow@redhat.com>
---
block/backup.c | 2 +-
block/commit.c | 2 +-
block/mirror.c | 2 +-
block/stream.c | 2 +-
blockjob.c | 2 +-
include/block/block.h | 3 +-
include/block/blockjob.h | 205 +-------------------------------------
include/block/blockjob_int.h | 232 +++++++++++++++++++++++++++++++++++++++++++
tests/test-blockjob-txn.c | 2 +-
tests/test-blockjob.c | 2 +-
10 files changed, 244 insertions(+), 210 deletions(-)
create mode 100644 include/block/blockjob_int.h
diff --git a/block/backup.c b/block/backup.c
index 6a60ca8..6d12100 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -16,7 +16,7 @@
#include "trace.h"
#include "block/block.h"
#include "block/block_int.h"
-#include "block/blockjob.h"
+#include "block/blockjob_int.h"
#include "block/block_backup.h"
#include "qapi/error.h"
#include "qapi/qmp/qerror.h"
diff --git a/block/commit.c b/block/commit.c
index 475a375..d555600 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -15,7 +15,7 @@
#include "qemu/osdep.h"
#include "trace.h"
#include "block/block_int.h"
-#include "block/blockjob.h"
+#include "block/blockjob_int.h"
#include "qapi/error.h"
#include "qapi/qmp/qerror.h"
#include "qemu/ratelimit.h"
diff --git a/block/mirror.c b/block/mirror.c
index 4374fb4..c81b5e0 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -13,7 +13,7 @@
#include "qemu/osdep.h"
#include "trace.h"
-#include "block/blockjob.h"
+#include "block/blockjob_int.h"
#include "block/block_int.h"
#include "sysemu/block-backend.h"
#include "qapi/error.h"
diff --git a/block/stream.c b/block/stream.c
index 7d6877d..906f7f3 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -14,7 +14,7 @@
#include "qemu/osdep.h"
#include "trace.h"
#include "block/block_int.h"
-#include "block/blockjob.h"
+#include "block/blockjob_int.h"
#include "qapi/error.h"
#include "qapi/qmp/qerror.h"
#include "qemu/ratelimit.h"
diff --git a/blockjob.c b/blockjob.c
index d118a1f..e6f0d97 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -27,7 +27,7 @@
#include "qemu-common.h"
#include "trace.h"
#include "block/block.h"
-#include "block/blockjob.h"
+#include "block/blockjob_int.h"
#include "block/block_int.h"
#include "sysemu/block-backend.h"
#include "qapi/qmp/qerror.h"
diff --git a/include/block/block.h b/include/block/block.h
index 107c603..89b5feb 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -7,16 +7,15 @@
#include "qemu/coroutine.h"
#include "block/accounting.h"
#include "block/dirty-bitmap.h"
+#include "block/blockjob.h"
#include "qapi/qmp/qobject.h"
#include "qapi-types.h"
#include "qemu/hbitmap.h"
/* block.c */
typedef struct BlockDriver BlockDriver;
-typedef struct BlockJob BlockJob;
typedef struct BdrvChild BdrvChild;
typedef struct BdrvChildRole BdrvChildRole;
-typedef struct BlockJobTxn BlockJobTxn;
typedef struct BlockDriverInfo {
/* in bytes, 0 if irrelevant */
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 5b61140..bfc8233 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -28,78 +28,15 @@
#include "block/block.h"
-/**
- * BlockJobDriver:
- *
- * A class type for block job driver.
- */
-typedef struct BlockJobDriver {
- /** Derived BlockJob struct size */
- size_t instance_size;
-
- /** String describing the operation, part of query-block-jobs QMP API */
- BlockJobType job_type;
-
- /** Optional callback for job types that support setting a speed limit */
- void (*set_speed)(BlockJob *job, int64_t speed, Error **errp);
-
- /** Optional callback for job types that need to forward I/O status reset */
- void (*iostatus_reset)(BlockJob *job);
-
- /**
- * Optional callback for job types whose completion must be triggered
- * manually.
- */
- void (*complete)(BlockJob *job, Error **errp);
-
- /**
- * If the callback is not NULL, it will be invoked when all the jobs
- * belonging to the same transaction complete; or upon this job's
- * completion if it is not in a transaction. Skipped if NULL.
- *
- * All jobs will complete with a call to either .commit() or .abort() but
- * never both.
- */
- void (*commit)(BlockJob *job);
-
- /**
- * If the callback is not NULL, it will be invoked when any job in the
- * same transaction fails; or upon this job's failure (due to error or
- * cancellation) if it is not in a transaction. Skipped if NULL.
- *
- * All jobs will complete with a call to either .commit() or .abort() but
- * never both.
- */
- void (*abort)(BlockJob *job);
-
- /**
- * If the callback is not NULL, it will be invoked when the job transitions
- * into the paused state. Paused jobs must not perform any asynchronous
- * I/O or event loop activity. This callback is used to quiesce jobs.
- */
- void coroutine_fn (*pause)(BlockJob *job);
-
- /**
- * If the callback is not NULL, it will be invoked when the job transitions
- * out of the paused state. Any asynchronous I/O or event loop activity
- * should be restarted from this callback.
- */
- void coroutine_fn (*resume)(BlockJob *job);
-
- /*
- * If the callback is not NULL, it will be invoked before the job is
- * resumed in a new AioContext. This is the place to move any resources
- * besides job->blk to the new AioContext.
- */
- void (*attached_aio_context)(BlockJob *job, AioContext *new_context);
-} BlockJobDriver;
+typedef struct BlockJobDriver BlockJobDriver;
+typedef struct BlockJobTxn BlockJobTxn;
/**
* BlockJob:
*
* Long-running operation on a BlockDriverState.
*/
-struct BlockJob {
+typedef struct BlockJob {
/** The job type, including the job vtable. */
const BlockJobDriver *driver;
@@ -198,7 +135,7 @@ struct BlockJob {
/** Non-NULL if this job is part of a transaction */
BlockJobTxn *txn;
QLIST_ENTRY(BlockJob) txn_list;
-};
+} BlockJob;
typedef enum BlockJobCreateFlags {
BLOCK_JOB_DEFAULT = 0x00,
@@ -227,76 +164,6 @@ BlockJob *block_job_next(BlockJob *job);
BlockJob *block_job_get(const char *id);
/**
- * block_job_create:
- * @job_id: The id of the newly-created job, or %NULL to have one
- * generated automatically.
- * @job_type: The class object for the newly-created job.
- * @bs: The block
- * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
- * @cb: Completion function for the job.
- * @opaque: Opaque pointer value passed to @cb.
- * @errp: Error object.
- *
- * Create a new long-running block device job and return it. The job
- * will call @cb asynchronously when the job completes. Note that
- * @bs may have been closed at the time the @cb it is called. If
- * this is the case, the job may be reported as either cancelled or
- * completed.
- *
- * This function is not part of the public job interface; it should be
- * called from a wrapper that is specific to the job type.
- */
-void *block_job_create(const char *job_id, const BlockJobDriver *driver,
- BlockDriverState *bs, int64_t speed, int flags,
- BlockCompletionFunc *cb, void *opaque, Error **errp);
-
-/**
- * block_job_sleep_ns:
- * @job: The job that calls the function.
- * @clock: The clock to sleep on.
- * @ns: How many nanoseconds to stop for.
- *
- * Put the job to sleep (assuming that it wasn't canceled) for @ns
- * nanoseconds. Canceling the job will interrupt the wait immediately.
- */
-void block_job_sleep_ns(BlockJob *job, QEMUClockType type, int64_t ns);
-
-/**
- * block_job_yield:
- * @job: The job that calls the function.
- *
- * Yield the block job coroutine.
- */
-void block_job_yield(BlockJob *job);
-
-/**
- * block_job_ref:
- * @bs: The block device.
- *
- * Grab a reference to the block job. Should be paired with block_job_unref.
- */
-void block_job_ref(BlockJob *job);
-
-/**
- * block_job_unref:
- * @bs: The block device.
- *
- * Release reference to the block job and release resources if it is the last
- * reference.
- */
-void block_job_unref(BlockJob *job);
-
-/**
- * block_job_completed:
- * @job: The job being completed.
- * @ret: The status code.
- *
- * Call the completion function that was registered at creation time, and
- * free @job.
- */
-void block_job_completed(BlockJob *job, int ret);
-
-/**
* block_job_set_speed:
* @job: The job to set the speed for.
* @speed: The new value
@@ -325,14 +192,6 @@ void block_job_cancel(BlockJob *job);
void block_job_complete(BlockJob *job, Error **errp);
/**
- * block_job_is_cancelled:
- * @job: The job being queried.
- *
- * Returns whether the job is scheduled for cancellation.
- */
-bool block_job_is_cancelled(BlockJob *job);
-
-/**
* block_job_query:
* @job: The job to get information about.
*
@@ -341,15 +200,6 @@ bool block_job_is_cancelled(BlockJob *job);
BlockJobInfo *block_job_query(BlockJob *job, Error **errp);
/**
- * block_job_pause_point:
- * @job: The job that is ready to pause.
- *
- * Pause now if block_job_pause() has been called. Block jobs that perform
- * lots of I/O must call this between requests so that the job can be paused.
- */
-void coroutine_fn block_job_pause_point(BlockJob *job);
-
-/**
* block_job_pause:
* @job: The job to be paused.
*
@@ -392,22 +242,6 @@ void block_job_resume(BlockJob *job);
void block_job_user_resume(BlockJob *job);
/**
- * block_job_enter:
- * @job: The job to enter.
- *
- * Continue the specified job by entering the coroutine.
- */
-void block_job_enter(BlockJob *job);
-
-/**
- * block_job_ready:
- * @job: The job which is now ready to complete.
- *
- * Send a BLOCK_JOB_READY event for the specified job.
- */
-void block_job_event_ready(BlockJob *job);
-
-/**
* block_job_cancel_sync:
* @job: The job to be canceled.
*
@@ -453,37 +287,6 @@ int block_job_complete_sync(BlockJob *job, Error **errp);
void block_job_iostatus_reset(BlockJob *job);
/**
- * block_job_error_action:
- * @job: The job to signal an error for.
- * @on_err: The error action setting.
- * @is_read: Whether the operation was a read.
- * @error: The error that was reported.
- *
- * Report an I/O error for a block job and possibly stop the VM. Return the
- * action that was selected based on @on_err and @error.
- */
-BlockErrorAction block_job_error_action(BlockJob *job, BlockdevOnError on_err,
- int is_read, int error);
-
-typedef void BlockJobDeferToMainLoopFn(BlockJob *job, void *opaque);
-
-/**
- * block_job_defer_to_main_loop:
- * @job: The job
- * @fn: The function to run in the main loop
- * @opaque: The opaque value that is passed to @fn
- *
- * Execute a given function in the main loop with the BlockDriverState
- * AioContext acquired. Block jobs must call bdrv_unref(), bdrv_close(), and
- * anything that uses bdrv_drain_all() in the main loop.
- *
- * The @job AioContext is held while @fn executes.
- */
-void block_job_defer_to_main_loop(BlockJob *job,
- BlockJobDeferToMainLoopFn *fn,
- void *opaque);
-
-/**
* block_job_txn_new:
*
* Allocate and return a new block job transaction. Jobs can be added to the
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
new file mode 100644
index 0000000..8eced19
--- /dev/null
+++ b/include/block/blockjob_int.h
@@ -0,0 +1,232 @@
+/*
+ * Declarations for long-running block device operations
+ *
+ * Copyright (c) 2011 IBM Corp.
+ * Copyright (c) 2012 Red Hat, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef BLOCKJOB_INT_H
+#define BLOCKJOB_INT_H
+
+#include "block/blockjob.h"
+#include "block/block.h"
+
+/**
+ * BlockJobDriver:
+ *
+ * A class type for block job driver.
+ */
+struct BlockJobDriver {
+ /** Derived BlockJob struct size */
+ size_t instance_size;
+
+ /** String describing the operation, part of query-block-jobs QMP API */
+ BlockJobType job_type;
+
+ /** Optional callback for job types that support setting a speed limit */
+ void (*set_speed)(BlockJob *job, int64_t speed, Error **errp);
+
+ /** Optional callback for job types that need to forward I/O status reset */
+ void (*iostatus_reset)(BlockJob *job);
+
+ /**
+ * Optional callback for job types whose completion must be triggered
+ * manually.
+ */
+ void (*complete)(BlockJob *job, Error **errp);
+
+ /**
+ * If the callback is not NULL, it will be invoked when all the jobs
+ * belonging to the same transaction complete; or upon this job's
+ * completion if it is not in a transaction. Skipped if NULL.
+ *
+ * All jobs will complete with a call to either .commit() or .abort() but
+ * never both.
+ */
+ void (*commit)(BlockJob *job);
+
+ /**
+ * If the callback is not NULL, it will be invoked when any job in the
+ * same transaction fails; or upon this job's failure (due to error or
+ * cancellation) if it is not in a transaction. Skipped if NULL.
+ *
+ * All jobs will complete with a call to either .commit() or .abort() but
+ * never both.
+ */
+ void (*abort)(BlockJob *job);
+
+ /**
+ * If the callback is not NULL, it will be invoked when the job transitions
+ * into the paused state. Paused jobs must not perform any asynchronous
+ * I/O or event loop activity. This callback is used to quiesce jobs.
+ */
+ void coroutine_fn (*pause)(BlockJob *job);
+
+ /**
+ * If the callback is not NULL, it will be invoked when the job transitions
+ * out of the paused state. Any asynchronous I/O or event loop activity
+ * should be restarted from this callback.
+ */
+ void coroutine_fn (*resume)(BlockJob *job);
+
+ /*
+ * If the callback is not NULL, it will be invoked before the job is
+ * resumed in a new AioContext. This is the place to move any resources
+ * besides job->blk to the new AioContext.
+ */
+ void (*attached_aio_context)(BlockJob *job, AioContext *new_context);
+};
+
+/**
+ * block_job_create:
+ * @job_id: The id of the newly-created job, or %NULL to have one
+ * generated automatically.
+ * @job_type: The class object for the newly-created job.
+ * @bs: The block
+ * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
+ * @cb: Completion function for the job.
+ * @opaque: Opaque pointer value passed to @cb.
+ * @errp: Error object.
+ *
+ * Create a new long-running block device job and return it. The job
+ * will call @cb asynchronously when the job completes. Note that
+ * @bs may have been closed at the time the @cb it is called. If
+ * this is the case, the job may be reported as either cancelled or
+ * completed.
+ *
+ * This function is not part of the public job interface; it should be
+ * called from a wrapper that is specific to the job type.
+ */
+void *block_job_create(const char *job_id, const BlockJobDriver *driver,
+ BlockDriverState *bs, int64_t speed, int flags,
+ BlockCompletionFunc *cb, void *opaque, Error **errp);
+
+/**
+ * block_job_sleep_ns:
+ * @job: The job that calls the function.
+ * @clock: The clock to sleep on.
+ * @ns: How many nanoseconds to stop for.
+ *
+ * Put the job to sleep (assuming that it wasn't canceled) for @ns
+ * nanoseconds. Canceling the job will interrupt the wait immediately.
+ */
+void block_job_sleep_ns(BlockJob *job, QEMUClockType type, int64_t ns);
+
+/**
+ * block_job_yield:
+ * @job: The job that calls the function.
+ *
+ * Yield the block job coroutine.
+ */
+void block_job_yield(BlockJob *job);
+
+/**
+ * block_job_ref:
+ * @bs: The block device.
+ *
+ * Grab a reference to the block job. Should be paired with block_job_unref.
+ */
+void block_job_ref(BlockJob *job);
+
+/**
+ * block_job_unref:
+ * @bs: The block device.
+ *
+ * Release reference to the block job and release resources if it is the last
+ * reference.
+ */
+void block_job_unref(BlockJob *job);
+
+/**
+ * block_job_completed:
+ * @job: The job being completed.
+ * @ret: The status code.
+ *
+ * Call the completion function that was registered at creation time, and
+ * free @job.
+ */
+void block_job_completed(BlockJob *job, int ret);
+
+/**
+ * block_job_is_cancelled:
+ * @job: The job being queried.
+ *
+ * Returns whether the job is scheduled for cancellation.
+ */
+bool block_job_is_cancelled(BlockJob *job);
+
+/**
+ * block_job_pause_point:
+ * @job: The job that is ready to pause.
+ *
+ * Pause now if block_job_pause() has been called. Block jobs that perform
+ * lots of I/O must call this between requests so that the job can be paused.
+ */
+void coroutine_fn block_job_pause_point(BlockJob *job);
+
+/**
+ * block_job_enter:
+ * @job: The job to enter.
+ *
+ * Continue the specified job by entering the coroutine.
+ */
+void block_job_enter(BlockJob *job);
+
+/**
+ * block_job_ready:
+ * @job: The job which is now ready to complete.
+ *
+ * Send a BLOCK_JOB_READY event for the specified job.
+ */
+void block_job_event_ready(BlockJob *job);
+
+/**
+ * block_job_error_action:
+ * @job: The job to signal an error for.
+ * @on_err: The error action setting.
+ * @is_read: Whether the operation was a read.
+ * @error: The error that was reported.
+ *
+ * Report an I/O error for a block job and possibly stop the VM. Return the
+ * action that was selected based on @on_err and @error.
+ */
+BlockErrorAction block_job_error_action(BlockJob *job, BlockdevOnError on_err,
+ int is_read, int error);
+
+typedef void BlockJobDeferToMainLoopFn(BlockJob *job, void *opaque);
+
+/**
+ * block_job_defer_to_main_loop:
+ * @job: The job
+ * @fn: The function to run in the main loop
+ * @opaque: The opaque value that is passed to @fn
+ *
+ * Execute a given function in the main loop with the BlockDriverState
+ * AioContext acquired. Block jobs must call bdrv_unref(), bdrv_close(), and
+ * anything that uses bdrv_drain_all() in the main loop.
+ *
+ * The @job AioContext is held while @fn executes.
+ */
+void block_job_defer_to_main_loop(BlockJob *job,
+ BlockJobDeferToMainLoopFn *fn,
+ void *opaque);
+
+#endif
diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
index b79e0c6..f9afc3b 100644
--- a/tests/test-blockjob-txn.c
+++ b/tests/test-blockjob-txn.c
@@ -13,7 +13,7 @@
#include "qemu/osdep.h"
#include "qapi/error.h"
#include "qemu/main-loop.h"
-#include "block/blockjob.h"
+#include "block/blockjob_int.h"
#include "sysemu/block-backend.h"
typedef struct {
diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
index 18bf850..60b78a3 100644
--- a/tests/test-blockjob.c
+++ b/tests/test-blockjob.c
@@ -13,7 +13,7 @@
#include "qemu/osdep.h"
#include "qapi/error.h"
#include "qemu/main-loop.h"
-#include "block/blockjob.h"
+#include "block/blockjob_int.h"
#include "sysemu/block-backend.h"
static const BlockJobDriver test_block_job_driver = {
--
2.7.4
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [Qemu-devel] [PATCH 7/7] blockjobs: fix documentation
2016-10-13 22:56 [Qemu-devel] [PATCH 0/7] blockjobs: preliminary refactoring work, Pt 1 John Snow
` (5 preceding siblings ...)
2016-10-13 22:57 ` [Qemu-devel] [PATCH 6/7] blockjobs: split interface into public/private, Part 1 John Snow
@ 2016-10-13 22:57 ` John Snow
2016-10-14 15:46 ` Kevin Wolf
2016-10-26 4:51 ` Jeff Cody
2016-10-14 5:33 ` [Qemu-devel] [PATCH 0/7] blockjobs: preliminary refactoring work, Pt 1 no-reply
2016-10-14 18:32 ` John Snow
8 siblings, 2 replies; 26+ messages in thread
From: John Snow @ 2016-10-13 22:57 UTC (permalink / raw)
To: qemu-block
Cc: kwolf, xiecl.fnst, wency, jcody, qemu-devel, pbonzini, John Snow
(Trivial)
Fix wrong function names in documentation.
Signed-off-by: John Snow <jsnow@redhat.com>
---
include/block/blockjob_int.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index 8eced19..10ebb38 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -191,8 +191,8 @@ void coroutine_fn block_job_pause_point(BlockJob *job);
void block_job_enter(BlockJob *job);
/**
- * block_job_ready:
- * @job: The job which is now ready to complete.
+ * block_job_event_ready:
+ * @job: The job which is now ready to be completed.
*
* Send a BLOCK_JOB_READY event for the specified job.
*/
--
2.7.4
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 0/7] blockjobs: preliminary refactoring work, Pt 1
2016-10-13 22:56 [Qemu-devel] [PATCH 0/7] blockjobs: preliminary refactoring work, Pt 1 John Snow
` (6 preceding siblings ...)
2016-10-13 22:57 ` [Qemu-devel] [PATCH 7/7] blockjobs: fix documentation John Snow
@ 2016-10-14 5:33 ` no-reply
2016-10-14 18:32 ` John Snow
8 siblings, 0 replies; 26+ messages in thread
From: no-reply @ 2016-10-14 5:33 UTC (permalink / raw)
To: jsnow; +Cc: famz, qemu-block, kwolf, xiecl.fnst, jcody, qemu-devel, pbonzini
Hi,
Your series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 1476399422-8028-1-git-send-email-jsnow@redhat.com
Subject: [Qemu-devel] [PATCH 0/7] blockjobs: preliminary refactoring work, Pt 1
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
42e8049 blockjobs: fix documentation
545d6e6 blockjobs: split interface into public/private, Part 1
6b894fe Blockjobs: Internalize user_pause logic
0863a3c blockjob: centralize QMP event emissions
1b493ef Replication/Blockjobs: Create replication jobs as internal
ba80fb1 blockjobs: Allow creating internal jobs
76110a3 blockjobs: hide internal jobs from management API
=== OUTPUT BEGIN ===
Checking PATCH 1/7: blockjobs: hide internal jobs from management API...
Checking PATCH 2/7: blockjobs: Allow creating internal jobs...
Checking PATCH 3/7: Replication/Blockjobs: Create replication jobs as internal...
Checking PATCH 4/7: blockjob: centralize QMP event emissions...
Checking PATCH 5/7: Blockjobs: Internalize user_pause logic...
Checking PATCH 6/7: blockjobs: split interface into public/private, Part 1...
ERROR: struct BlockJobDriver should normally be const
#182: FILE: include/block/blockjob.h:31:
+typedef struct BlockJobDriver BlockJobDriver;
ERROR: struct BlockJobDriver should normally be const
#415: FILE: include/block/blockjob_int.h:37:
+struct BlockJobDriver {
ERROR: space prohibited between function name and open parenthesis '('
#459: FILE: include/block/blockjob_int.h:81:
+ void coroutine_fn (*pause)(BlockJob *job);
ERROR: space prohibited between function name and open parenthesis '('
#466: FILE: include/block/blockjob_int.h:88:
+ void coroutine_fn (*resume)(BlockJob *job);
total: 4 errors, 0 warnings, 558 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 7/7: blockjobs: fix documentation...
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 1/7] blockjobs: hide internal jobs from management API
2016-10-13 22:56 ` [Qemu-devel] [PATCH 1/7] blockjobs: hide internal jobs from management API John Snow
@ 2016-10-14 12:58 ` Kevin Wolf
2016-10-14 17:32 ` John Snow
0 siblings, 1 reply; 26+ messages in thread
From: Kevin Wolf @ 2016-10-14 12:58 UTC (permalink / raw)
To: John Snow; +Cc: qemu-block, xiecl.fnst, wency, jcody, qemu-devel, pbonzini
Am 14.10.2016 um 00:56 hat John Snow geschrieben:
> If jobs are not created directly by the user, do not allow them to be
> seen by the user/management utility. At the moment, 'internal' jobs are
> those that do not have an ID. As of this patch it is impossible to
> create such jobs.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
block_job_get() still has a strcmp(id, job->id) for all block jobs
without checking job->id != NULL first.
job->id is also used in the error message in block_job_complete(),
though you could argue that we have a bug if this is ever triggered for
internal jobs. Still, there are platform on which this would crash, so
maybe better catch it.
> -BlockJobInfo *block_job_query(BlockJob *job)
> +BlockJobInfo *block_job_query(BlockJob *job, Error **errp)
> {
> - BlockJobInfo *info = g_new0(BlockJobInfo, 1);
> + BlockJobInfo *info;
> +
> + if (block_job_is_internal(job)) {
> + error_setg(errp, "Cannot query QEMU internal Jobs");
You definitely have Potential for being a good Student of German. I
agree that a Text is easier to read if you capitalise all Nouns in it,
but I'm afraid this is not Part of the current english Orthography.
> + return NULL;
> + }
> + info = g_new0(BlockJobInfo, 1);
> info->type = g_strdup(BlockJobType_lookup[job->driver->job_type]);
> info->device = g_strdup(job->id);
> info->len = job->len;
Kevin
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 2/7] blockjobs: Allow creating internal jobs
2016-10-13 22:56 ` [Qemu-devel] [PATCH 2/7] blockjobs: Allow creating internal jobs John Snow
@ 2016-10-14 14:40 ` Kevin Wolf
2016-10-26 4:48 ` Jeff Cody
1 sibling, 0 replies; 26+ messages in thread
From: Kevin Wolf @ 2016-10-14 14:40 UTC (permalink / raw)
To: John Snow; +Cc: qemu-block, xiecl.fnst, wency, jcody, qemu-devel, pbonzini
Am 14.10.2016 um 00:56 hat John Snow geschrieben:
> Add the ability to create jobs without an ID.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 3/7] Replication/Blockjobs: Create replication jobs as internal
2016-10-13 22:56 ` [Qemu-devel] [PATCH 3/7] Replication/Blockjobs: Create replication jobs as internal John Snow
@ 2016-10-14 14:40 ` Kevin Wolf
2016-10-26 4:48 ` Jeff Cody
1 sibling, 0 replies; 26+ messages in thread
From: Kevin Wolf @ 2016-10-14 14:40 UTC (permalink / raw)
To: John Snow; +Cc: qemu-block, xiecl.fnst, wency, jcody, qemu-devel, pbonzini
Am 14.10.2016 um 00:56 hat John Snow geschrieben:
> Bubble up the internal interface to commit and backup jobs, then switch
> replication tasks over to using this methodology.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 4/7] blockjob: centralize QMP event emissions
2016-10-13 22:56 ` [Qemu-devel] [PATCH 4/7] blockjob: centralize QMP event emissions John Snow
@ 2016-10-14 15:45 ` Kevin Wolf
2016-10-26 4:49 ` Jeff Cody
1 sibling, 0 replies; 26+ messages in thread
From: Kevin Wolf @ 2016-10-14 15:45 UTC (permalink / raw)
To: John Snow; +Cc: qemu-block, xiecl.fnst, wency, jcody, qemu-devel, pbonzini
Am 14.10.2016 um 00:56 hat John Snow geschrieben:
> There's no reason to leave this to blockdev; we can do it in blockjobs
> directly and get rid of an extra callback for most users.
>
> All non-internal events, even those created outside of QMP, will
> consistently emit events.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 5/7] Blockjobs: Internalize user_pause logic
2016-10-13 22:57 ` [Qemu-devel] [PATCH 5/7] Blockjobs: Internalize user_pause logic John Snow
@ 2016-10-14 15:46 ` Kevin Wolf
2016-10-26 4:50 ` Jeff Cody
1 sibling, 0 replies; 26+ messages in thread
From: Kevin Wolf @ 2016-10-14 15:46 UTC (permalink / raw)
To: John Snow; +Cc: qemu-block, xiecl.fnst, wency, jcody, qemu-devel, pbonzini
Am 14.10.2016 um 00:57 hat John Snow geschrieben:
> BlockJobs will begin hiding their state in preparation for some
> refactorings anyway, so let's internalize the user_pause mechanism
> instead of leaving it to callers to correctly manage.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 6/7] blockjobs: split interface into public/private, Part 1
2016-10-13 22:57 ` [Qemu-devel] [PATCH 6/7] blockjobs: split interface into public/private, Part 1 John Snow
@ 2016-10-14 15:46 ` Kevin Wolf
2016-10-26 4:51 ` Jeff Cody
1 sibling, 0 replies; 26+ messages in thread
From: Kevin Wolf @ 2016-10-14 15:46 UTC (permalink / raw)
To: John Snow; +Cc: qemu-block, xiecl.fnst, wency, jcody, qemu-devel, pbonzini
Am 14.10.2016 um 00:57 hat John Snow geschrieben:
> To make it a little more obvious which functions are intended to be
> public interface and which are intended to be for use only by jobs
> themselves, split the interface into "public" and "private" files.
>
> Convert blockjobs (e.g. block/backup) to using the private interface.
> Leave blockdev and others on the public interface.
>
> There are remaining uses of private state by qemu-img, and several
> cases in blockdev.c and block/io.c where we grab job->blk for the
> purposes of acquiring an AIOContext.
>
> These will be corrected in future patches.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] blockjobs: fix documentation
2016-10-13 22:57 ` [Qemu-devel] [PATCH 7/7] blockjobs: fix documentation John Snow
@ 2016-10-14 15:46 ` Kevin Wolf
2016-10-26 4:51 ` Jeff Cody
1 sibling, 0 replies; 26+ messages in thread
From: Kevin Wolf @ 2016-10-14 15:46 UTC (permalink / raw)
To: John Snow; +Cc: qemu-block, xiecl.fnst, wency, jcody, qemu-devel, pbonzini
Am 14.10.2016 um 00:57 hat John Snow geschrieben:
> (Trivial)
>
> Fix wrong function names in documentation.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 1/7] blockjobs: hide internal jobs from management API
2016-10-14 12:58 ` Kevin Wolf
@ 2016-10-14 17:32 ` John Snow
0 siblings, 0 replies; 26+ messages in thread
From: John Snow @ 2016-10-14 17:32 UTC (permalink / raw)
To: Kevin Wolf; +Cc: xiecl.fnst, qemu-block, jcody, qemu-devel, pbonzini
On 10/14/2016 08:58 AM, Kevin Wolf wrote:
> Am 14.10.2016 um 00:56 hat John Snow geschrieben:
>> If jobs are not created directly by the user, do not allow them to be
>> seen by the user/management utility. At the moment, 'internal' jobs are
>> those that do not have an ID. As of this patch it is impossible to
>> create such jobs.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>
> block_job_get() still has a strcmp(id, job->id) for all block jobs
> without checking job->id != NULL first.
>
Good catch.
> job->id is also used in the error message in block_job_complete(),
> though you could argue that we have a bug if this is ever triggered for
> internal jobs. Still, there are platform on which this would crash, so
> maybe better catch it.
>
It may be misleading to check for job->id in a function that should not
be called when that is false. I will add an assertion instead, with the
premise that any potential misuses here must necessarily be internal.
>> -BlockJobInfo *block_job_query(BlockJob *job)
>> +BlockJobInfo *block_job_query(BlockJob *job, Error **errp)
>> {
>> - BlockJobInfo *info = g_new0(BlockJobInfo, 1);
>> + BlockJobInfo *info;
>> +
>> + if (block_job_is_internal(job)) {
>> + error_setg(errp, "Cannot query QEMU internal Jobs");
>
> You definitely have Potential for being a good Student of German. I
> agree that a Text is easier to read if you capitalise all Nouns in it,
> but I'm afraid this is not Part of the current english Orthography.
>
^^^^^^^
This is my favorite email.
>> + return NULL;
>> + }
>> + info = g_new0(BlockJobInfo, 1);
>> info->type = g_strdup(BlockJobType_lookup[job->driver->job_type]);
>> info->device = g_strdup(job->id);
>> info->len = job->len;
>
> Kevin
>
--
—js
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 0/7] blockjobs: preliminary refactoring work, Pt 1
2016-10-13 22:56 [Qemu-devel] [PATCH 0/7] blockjobs: preliminary refactoring work, Pt 1 John Snow
` (7 preceding siblings ...)
2016-10-14 5:33 ` [Qemu-devel] [PATCH 0/7] blockjobs: preliminary refactoring work, Pt 1 no-reply
@ 2016-10-14 18:32 ` John Snow
2016-10-26 4:52 ` Jeff Cody
8 siblings, 1 reply; 26+ messages in thread
From: John Snow @ 2016-10-14 18:32 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, xiecl.fnst, wency, jcody, qemu-devel, pbonzini
On 10/13/2016 06:56 PM, John Snow wrote:
> This is a follow-up to patches 1-6 of:
> [PATCH v2 00/11] blockjobs: Fix transactional race condition
>
> That series started trying to refactor blockjobs with the goal of
> internalizing BlockJob state as a side effect of having gone through
> the effort of figuring out which commands were "safe" to call on
> a Job that has no coroutine object.
>
> I've split out the less contentious bits so I can move forward with my
> original work of focusing on the transactional race condition in a
> different series.
>
> Functionally the biggest difference here is the presence of "internal"
> block jobs, which do not emit QMP events or show up in block query
> requests. This is done for the sake of replication jobs, which should
> not be interfering with the public jobs namespace.
>
I have v2 ready to send out correcting Kevin's comments in patch #01,
but I'd like to have the Replication maintainers at Fujitsu take a look
at how I have modified replication and at least 'ACK' the change.
As a recap: I am creating "internal" block jobs that have no ID and
therefore do not collide with the user-specified jobs namespace. This
way users cannot query, cancel, pause, or otherwise accidentally
interfere with the replication job lifetime.
It also means that management layers such as libvirt will not be aware
of the presence of such "internal" jobs.
Relevant patches are 1-3. Please let me know if you have questions.
Thanks,
--John Snow
> ________________________________________________________________________________
>
> For convenience, this branch is available at:
> https://github.com/jnsnow/qemu.git branch job-refactor-pt1
> https://github.com/jnsnow/qemu/tree/job-refactor-pt1
>
> This version is tagged job-refactor-pt1-v1:
> https://github.com/jnsnow/qemu/releases/tag/job-refactor-pt1-v1
>
> John Snow (7):
> blockjobs: hide internal jobs from management API
> blockjobs: Allow creating internal jobs
> Replication/Blockjobs: Create replication jobs as internal
> blockjob: centralize QMP event emissions
> Blockjobs: Internalize user_pause logic
> blockjobs: split interface into public/private, Part 1
> blockjobs: fix documentation
>
> block/backup.c | 5 +-
> block/commit.c | 10 +-
> block/mirror.c | 28 +++--
> block/replication.c | 14 +--
> block/stream.c | 9 +-
> block/trace-events | 5 +-
> blockdev.c | 74 +++++--------
> blockjob.c | 109 ++++++++++++++----
> include/block/block.h | 3 +-
> include/block/block_int.h | 26 ++---
> include/block/blockjob.h | 257 +++++++------------------------------------
> include/block/blockjob_int.h | 232 ++++++++++++++++++++++++++++++++++++++
> qemu-img.c | 5 +-
> tests/test-blockjob-txn.c | 5 +-
> tests/test-blockjob.c | 4 +-
> 15 files changed, 443 insertions(+), 343 deletions(-)
> create mode 100644 include/block/blockjob_int.h
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 2/7] blockjobs: Allow creating internal jobs
2016-10-13 22:56 ` [Qemu-devel] [PATCH 2/7] blockjobs: Allow creating internal jobs John Snow
2016-10-14 14:40 ` Kevin Wolf
@ 2016-10-26 4:48 ` Jeff Cody
1 sibling, 0 replies; 26+ messages in thread
From: Jeff Cody @ 2016-10-26 4:48 UTC (permalink / raw)
To: John Snow; +Cc: qemu-block, kwolf, xiecl.fnst, wency, qemu-devel, pbonzini
On Thu, Oct 13, 2016 at 06:56:57PM -0400, John Snow wrote:
> Add the ability to create jobs without an ID.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block/backup.c | 2 +-
> block/commit.c | 2 +-
> block/mirror.c | 3 ++-
> block/stream.c | 2 +-
> blockjob.c | 25 ++++++++++++++++---------
> include/block/blockjob.h | 7 ++++++-
> tests/test-blockjob-txn.c | 3 ++-
> tests/test-blockjob.c | 2 +-
> 8 files changed, 30 insertions(+), 16 deletions(-)
>
> diff --git a/block/backup.c b/block/backup.c
> index 582bd0f..5acb5c4 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -596,7 +596,7 @@ void backup_start(const char *job_id, BlockDriverState *bs,
> }
>
> job = block_job_create(job_id, &backup_job_driver, bs, speed,
> - cb, opaque, errp);
> + BLOCK_JOB_DEFAULT, cb, opaque, errp);
> if (!job) {
> goto error;
> }
> diff --git a/block/commit.c b/block/commit.c
> index 9f67a8b..f29e341 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -233,7 +233,7 @@ void commit_start(const char *job_id, BlockDriverState *bs,
> }
>
> s = block_job_create(job_id, &commit_job_driver, bs, speed,
> - cb, opaque, errp);
> + BLOCK_JOB_DEFAULT, cb, opaque, errp);
> if (!s) {
> return;
> }
> diff --git a/block/mirror.c b/block/mirror.c
> index f9d1fec..74c03ae 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -936,7 +936,8 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
> buf_size = DEFAULT_MIRROR_BUF_SIZE;
> }
>
> - s = block_job_create(job_id, driver, bs, speed, cb, opaque, errp);
> + s = block_job_create(job_id, driver, bs, speed,
> + BLOCK_JOB_DEFAULT, cb, opaque, errp);
> if (!s) {
> return;
> }
> diff --git a/block/stream.c b/block/stream.c
> index 3187481..eeb6f52 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -222,7 +222,7 @@ void stream_start(const char *job_id, BlockDriverState *bs,
> StreamBlockJob *s;
>
> s = block_job_create(job_id, &stream_job_driver, bs, speed,
> - cb, opaque, errp);
> + BLOCK_JOB_DEFAULT, cb, opaque, errp);
> if (!s) {
> return;
> }
> diff --git a/blockjob.c b/blockjob.c
> index e78ad94..017905a 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -118,7 +118,7 @@ static void block_job_detach_aio_context(void *opaque)
> }
>
> void *block_job_create(const char *job_id, const BlockJobDriver *driver,
> - BlockDriverState *bs, int64_t speed,
> + BlockDriverState *bs, int64_t speed, int flags,
> BlockCompletionFunc *cb, void *opaque, Error **errp)
> {
> BlockBackend *blk;
> @@ -130,7 +130,7 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
> return NULL;
> }
>
> - if (job_id == NULL) {
> + if (job_id == NULL && !(flags & BLOCK_JOB_INTERNAL)) {
> job_id = bdrv_get_device_name(bs);
> if (!*job_id) {
> error_setg(errp, "An explicit job ID is required for this node");
> @@ -138,14 +138,21 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
> }
> }
>
> - if (!id_wellformed(job_id)) {
> - error_setg(errp, "Invalid job ID '%s'", job_id);
> - return NULL;
> - }
> + if (job_id) {
> + if (flags & BLOCK_JOB_INTERNAL) {
> + error_setg(errp, "Cannot specify job ID for internal block job");
> + return NULL;
> + }
>
> - if (block_job_get(job_id)) {
> - error_setg(errp, "Job ID '%s' already in use", job_id);
> - return NULL;
> + if (!id_wellformed(job_id)) {
> + error_setg(errp, "Invalid job ID '%s'", job_id);
> + return NULL;
> + }
> +
> + if (block_job_get(job_id)) {
> + error_setg(errp, "Job ID '%s' already in use", job_id);
> + return NULL;
> + }
> }
>
> blk = blk_new();
> diff --git a/include/block/blockjob.h b/include/block/blockjob.h
> index 6ecfa2e..fdb31e0 100644
> --- a/include/block/blockjob.h
> +++ b/include/block/blockjob.h
> @@ -200,6 +200,11 @@ struct BlockJob {
> QLIST_ENTRY(BlockJob) txn_list;
> };
>
> +typedef enum BlockJobCreateFlags {
> + BLOCK_JOB_DEFAULT = 0x00,
> + BLOCK_JOB_INTERNAL = 0x01,
> +} BlockJobCreateFlags;
> +
> /**
> * block_job_next:
> * @job: A block job, or %NULL.
> @@ -242,7 +247,7 @@ BlockJob *block_job_get(const char *id);
> * called from a wrapper that is specific to the job type.
> */
> void *block_job_create(const char *job_id, const BlockJobDriver *driver,
> - BlockDriverState *bs, int64_t speed,
> + BlockDriverState *bs, int64_t speed, int flags,
> BlockCompletionFunc *cb, void *opaque, Error **errp);
>
> /**
> diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
> index d049cba..b79e0c6 100644
> --- a/tests/test-blockjob-txn.c
> +++ b/tests/test-blockjob-txn.c
> @@ -98,7 +98,8 @@ static BlockJob *test_block_job_start(unsigned int iterations,
> bs = bdrv_new();
> snprintf(job_id, sizeof(job_id), "job%u", counter++);
> s = block_job_create(job_id, &test_block_job_driver, bs, 0,
> - test_block_job_cb, data, &error_abort);
> + BLOCK_JOB_DEFAULT, test_block_job_cb,
> + data, &error_abort);
> s->iterations = iterations;
> s->use_timer = use_timer;
> s->rc = rc;
> diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
> index 5b0e934..18bf850 100644
> --- a/tests/test-blockjob.c
> +++ b/tests/test-blockjob.c
> @@ -31,7 +31,7 @@ static BlockJob *do_test_id(BlockBackend *blk, const char *id,
> Error *errp = NULL;
>
> job = block_job_create(id, &test_block_job_driver, blk_bs(blk), 0,
> - block_job_cb, NULL, &errp);
> + BLOCK_JOB_DEFAULT, block_job_cb, NULL, &errp);
> if (should_succeed) {
> g_assert_null(errp);
> g_assert_nonnull(job);
> --
> 2.7.4
>
Reviewed-by: Jeff Cody <jcody@redhat.com>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 3/7] Replication/Blockjobs: Create replication jobs as internal
2016-10-13 22:56 ` [Qemu-devel] [PATCH 3/7] Replication/Blockjobs: Create replication jobs as internal John Snow
2016-10-14 14:40 ` Kevin Wolf
@ 2016-10-26 4:48 ` Jeff Cody
1 sibling, 0 replies; 26+ messages in thread
From: Jeff Cody @ 2016-10-26 4:48 UTC (permalink / raw)
To: John Snow; +Cc: qemu-block, kwolf, xiecl.fnst, wency, qemu-devel, pbonzini
On Thu, Oct 13, 2016 at 06:56:58PM -0400, John Snow wrote:
> Bubble up the internal interface to commit and backup jobs, then switch
> replication tasks over to using this methodology.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block/backup.c | 3 ++-
> block/mirror.c | 21 ++++++++++-----------
> block/replication.c | 14 +++++++-------
> blockdev.c | 11 +++++++----
> include/block/block_int.h | 9 +++++++--
> qemu-img.c | 5 +++--
> 6 files changed, 36 insertions(+), 27 deletions(-)
>
> diff --git a/block/backup.c b/block/backup.c
> index 5acb5c4..6a60ca8 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -527,6 +527,7 @@ void backup_start(const char *job_id, BlockDriverState *bs,
> bool compress,
> BlockdevOnError on_source_error,
> BlockdevOnError on_target_error,
> + int creation_flags,
> BlockCompletionFunc *cb, void *opaque,
> BlockJobTxn *txn, Error **errp)
> {
> @@ -596,7 +597,7 @@ void backup_start(const char *job_id, BlockDriverState *bs,
> }
>
> job = block_job_create(job_id, &backup_job_driver, bs, speed,
> - BLOCK_JOB_DEFAULT, cb, opaque, errp);
> + creation_flags, cb, opaque, errp);
> if (!job) {
> goto error;
> }
> diff --git a/block/mirror.c b/block/mirror.c
> index 74c03ae..15d2d10 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -906,9 +906,9 @@ static const BlockJobDriver commit_active_job_driver = {
> };
>
> static void mirror_start_job(const char *job_id, BlockDriverState *bs,
> - BlockDriverState *target, const char *replaces,
> - int64_t speed, uint32_t granularity,
> - int64_t buf_size,
> + int creation_flags, BlockDriverState *target,
> + const char *replaces, int64_t speed,
> + uint32_t granularity, int64_t buf_size,
> BlockMirrorBackingMode backing_mode,
> BlockdevOnError on_source_error,
> BlockdevOnError on_target_error,
> @@ -936,8 +936,8 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
> buf_size = DEFAULT_MIRROR_BUF_SIZE;
> }
>
> - s = block_job_create(job_id, driver, bs, speed,
> - BLOCK_JOB_DEFAULT, cb, opaque, errp);
> + s = block_job_create(job_id, driver, bs, speed, creation_flags,
> + cb, opaque, errp);
> if (!s) {
> return;
> }
> @@ -992,17 +992,16 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
> }
> is_none_mode = mode == MIRROR_SYNC_MODE_NONE;
> base = mode == MIRROR_SYNC_MODE_TOP ? backing_bs(bs) : NULL;
> - mirror_start_job(job_id, bs, target, replaces,
> + mirror_start_job(job_id, bs, BLOCK_JOB_DEFAULT, target, replaces,
> speed, granularity, buf_size, backing_mode,
> on_source_error, on_target_error, unmap, cb, opaque, errp,
> &mirror_job_driver, is_none_mode, base, false);
> }
>
> void commit_active_start(const char *job_id, BlockDriverState *bs,
> - BlockDriverState *base, int64_t speed,
> - BlockdevOnError on_error,
> - BlockCompletionFunc *cb,
> - void *opaque, Error **errp,
> + BlockDriverState *base, int creation_flags,
> + int64_t speed, BlockdevOnError on_error,
> + BlockCompletionFunc *cb, void *opaque, Error **errp,
> bool auto_complete)
> {
> int64_t length, base_length;
> @@ -1041,7 +1040,7 @@ void commit_active_start(const char *job_id, BlockDriverState *bs,
> }
> }
>
> - mirror_start_job(job_id, bs, base, NULL, speed, 0, 0,
> + mirror_start_job(job_id, bs, creation_flags, base, NULL, speed, 0, 0,
> MIRROR_LEAVE_BACKING_CHAIN,
> on_error, on_error, false, cb, opaque, &local_err,
> &commit_active_job_driver, false, base, auto_complete);
> diff --git a/block/replication.c b/block/replication.c
> index 3bd1cf1..d4f4a7b 100644
> --- a/block/replication.c
> +++ b/block/replication.c
> @@ -496,10 +496,11 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
> bdrv_op_block_all(top_bs, s->blocker);
> bdrv_op_unblock(top_bs, BLOCK_OP_TYPE_DATAPLANE, s->blocker);
>
> - backup_start("replication-backup", s->secondary_disk->bs,
> - s->hidden_disk->bs, 0, MIRROR_SYNC_MODE_NONE, NULL, false,
> + backup_start(NULL, s->secondary_disk->bs, s->hidden_disk->bs, 0,
> + MIRROR_SYNC_MODE_NONE, NULL, false,
> BLOCKDEV_ON_ERROR_REPORT, BLOCKDEV_ON_ERROR_REPORT,
> - backup_job_completed, s, NULL, &local_err);
> + BLOCK_JOB_INTERNAL, backup_job_completed, s,
> + NULL, &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> backup_job_cleanup(s);
> @@ -621,10 +622,9 @@ static void replication_stop(ReplicationState *rs, bool failover, Error **errp)
> }
>
> s->replication_state = BLOCK_REPLICATION_FAILOVER;
> - commit_active_start("replication-commit", s->active_disk->bs,
> - s->secondary_disk->bs, 0, BLOCKDEV_ON_ERROR_REPORT,
> - replication_done,
> - bs, errp, true);
> + commit_active_start(NULL, s->active_disk->bs, s->secondary_disk->bs,
> + BLOCK_JOB_INTERNAL, 0, BLOCKDEV_ON_ERROR_REPORT,
> + replication_done, bs, errp, true);
> break;
> default:
> aio_context_release(aio_context);
> diff --git a/blockdev.c b/blockdev.c
> index 5904edb..0ce305c 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3083,8 +3083,9 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
> " but 'top' is the active layer");
> goto out;
> }
> - commit_active_start(has_job_id ? job_id : NULL, bs, base_bs, speed,
> - on_error, block_job_cb, bs, &local_err, false);
> + commit_active_start(has_job_id ? job_id : NULL, bs, base_bs,
> + BLOCK_JOB_DEFAULT, speed, on_error, block_job_cb,
> + bs, &local_err, false);
> } else {
> commit_start(has_job_id ? job_id : NULL, bs, base_bs, top_bs, speed,
> on_error, block_job_cb, bs,
> @@ -3208,7 +3209,8 @@ static void do_drive_backup(DriveBackup *backup, BlockJobTxn *txn, Error **errp)
>
> backup_start(backup->job_id, bs, target_bs, backup->speed, backup->sync,
> bmap, backup->compress, backup->on_source_error,
> - backup->on_target_error, block_job_cb, bs, txn, &local_err);
> + backup->on_target_error, BLOCK_JOB_DEFAULT,
> + block_job_cb, bs, txn, &local_err);
> bdrv_unref(target_bs);
> if (local_err != NULL) {
> error_propagate(errp, local_err);
> @@ -3278,7 +3280,8 @@ void do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn, Error **errp)
> }
> backup_start(backup->job_id, bs, target_bs, backup->speed, backup->sync,
> NULL, backup->compress, backup->on_source_error,
> - backup->on_target_error, block_job_cb, bs, txn, &local_err);
> + backup->on_target_error, BLOCK_JOB_DEFAULT,
> + block_job_cb, bs, txn, &local_err);
> if (local_err != NULL) {
> error_propagate(errp, local_err);
> }
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 3e79228..98f1c7f 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -688,6 +688,8 @@ void commit_start(const char *job_id, BlockDriverState *bs,
> * device name of @bs.
> * @bs: Active block device to be committed.
> * @base: Block device that will be written into, and become the new top.
> + * @creation_flags: Flags that control the behavior of the Job lifetime.
> + * See @BlockJobCreateFlags
> * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
> * @on_error: The action to take upon error.
> * @cb: Completion function for the job.
> @@ -697,8 +699,8 @@ void commit_start(const char *job_id, BlockDriverState *bs,
> *
> */
> void commit_active_start(const char *job_id, BlockDriverState *bs,
> - BlockDriverState *base, int64_t speed,
> - BlockdevOnError on_error,
> + BlockDriverState *base, int creation_flags,
> + int64_t speed, BlockdevOnError on_error,
> BlockCompletionFunc *cb,
> void *opaque, Error **errp, bool auto_complete);
> /*
> @@ -747,6 +749,8 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
> * @sync_bitmap: The dirty bitmap if sync_mode is MIRROR_SYNC_MODE_INCREMENTAL.
> * @on_source_error: The action to take upon error reading from the source.
> * @on_target_error: The action to take upon error writing to the target.
> + * @creation_flags: Flags that control the behavior of the Job lifetime.
> + * See @BlockJobCreateFlags
> * @cb: Completion function for the job.
> * @opaque: Opaque pointer value passed to @cb.
> * @txn: Transaction that this job is part of (may be NULL).
> @@ -760,6 +764,7 @@ void backup_start(const char *job_id, BlockDriverState *bs,
> bool compress,
> BlockdevOnError on_source_error,
> BlockdevOnError on_target_error,
> + int creation_flags,
> BlockCompletionFunc *cb, void *opaque,
> BlockJobTxn *txn, Error **errp);
>
> diff --git a/qemu-img.c b/qemu-img.c
> index 02c07b9..3897d82 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -928,8 +928,9 @@ static int img_commit(int argc, char **argv)
> .bs = bs,
> };
>
> - commit_active_start("commit", bs, base_bs, 0, BLOCKDEV_ON_ERROR_REPORT,
> - common_block_job_cb, &cbi, &local_err, false);
> + commit_active_start("commit", bs, base_bs, BLOCK_JOB_DEFAULT, 0,
> + BLOCKDEV_ON_ERROR_REPORT, common_block_job_cb, &cbi,
> + &local_err, false);
> if (local_err) {
> goto done;
> }
> --
> 2.7.4
>
Reviewed-by: Jeff Cody <jcody@redhat.com>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 4/7] blockjob: centralize QMP event emissions
2016-10-13 22:56 ` [Qemu-devel] [PATCH 4/7] blockjob: centralize QMP event emissions John Snow
2016-10-14 15:45 ` Kevin Wolf
@ 2016-10-26 4:49 ` Jeff Cody
1 sibling, 0 replies; 26+ messages in thread
From: Jeff Cody @ 2016-10-26 4:49 UTC (permalink / raw)
To: John Snow; +Cc: qemu-block, kwolf, xiecl.fnst, wency, qemu-devel, pbonzini
On Thu, Oct 13, 2016 at 06:56:59PM -0400, John Snow wrote:
> There's no reason to leave this to blockdev; we can do it in blockjobs
> directly and get rid of an extra callback for most users.
>
> All non-internal events, even those created outside of QMP, will
> consistently emit events.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block/commit.c | 8 ++++----
> block/mirror.c | 6 ++----
> block/stream.c | 7 +++----
> block/trace-events | 5 ++---
> blockdev.c | 42 ++++++++----------------------------------
> blockjob.c | 23 +++++++++++++++++++----
> include/block/block_int.h | 17 ++++-------------
> include/block/blockjob.h | 17 -----------------
> 8 files changed, 42 insertions(+), 83 deletions(-)
>
> diff --git a/block/commit.c b/block/commit.c
> index f29e341..475a375 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -209,8 +209,8 @@ static const BlockJobDriver commit_job_driver = {
>
> void commit_start(const char *job_id, BlockDriverState *bs,
> BlockDriverState *base, BlockDriverState *top, int64_t speed,
> - BlockdevOnError on_error, BlockCompletionFunc *cb,
> - void *opaque, const char *backing_file_str, Error **errp)
> + BlockdevOnError on_error, const char *backing_file_str,
> + Error **errp)
> {
> CommitBlockJob *s;
> BlockReopenQueue *reopen_queue = NULL;
> @@ -233,7 +233,7 @@ void commit_start(const char *job_id, BlockDriverState *bs,
> }
>
> s = block_job_create(job_id, &commit_job_driver, bs, speed,
> - BLOCK_JOB_DEFAULT, cb, opaque, errp);
> + BLOCK_JOB_DEFAULT, NULL, NULL, errp);
> if (!s) {
> return;
> }
> @@ -276,7 +276,7 @@ void commit_start(const char *job_id, BlockDriverState *bs,
> s->on_error = on_error;
> s->common.co = qemu_coroutine_create(commit_run, s);
>
> - trace_commit_start(bs, base, top, s, s->common.co, opaque);
> + trace_commit_start(bs, base, top, s, s->common.co);
> qemu_coroutine_enter(s->common.co);
> }
>
> diff --git a/block/mirror.c b/block/mirror.c
> index 15d2d10..4374fb4 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -979,9 +979,7 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
> MirrorSyncMode mode, BlockMirrorBackingMode backing_mode,
> BlockdevOnError on_source_error,
> BlockdevOnError on_target_error,
> - bool unmap,
> - BlockCompletionFunc *cb,
> - void *opaque, Error **errp)
> + bool unmap, Error **errp)
> {
> bool is_none_mode;
> BlockDriverState *base;
> @@ -994,7 +992,7 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
> base = mode == MIRROR_SYNC_MODE_TOP ? backing_bs(bs) : NULL;
> mirror_start_job(job_id, bs, BLOCK_JOB_DEFAULT, target, replaces,
> speed, granularity, buf_size, backing_mode,
> - on_source_error, on_target_error, unmap, cb, opaque, errp,
> + on_source_error, on_target_error, unmap, NULL, NULL, errp,
> &mirror_job_driver, is_none_mode, base, false);
> }
>
> diff --git a/block/stream.c b/block/stream.c
> index eeb6f52..7d6877d 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -216,13 +216,12 @@ static const BlockJobDriver stream_job_driver = {
>
> void stream_start(const char *job_id, BlockDriverState *bs,
> BlockDriverState *base, const char *backing_file_str,
> - int64_t speed, BlockdevOnError on_error,
> - BlockCompletionFunc *cb, void *opaque, Error **errp)
> + int64_t speed, BlockdevOnError on_error, Error **errp)
> {
> StreamBlockJob *s;
>
> s = block_job_create(job_id, &stream_job_driver, bs, speed,
> - BLOCK_JOB_DEFAULT, cb, opaque, errp);
> + BLOCK_JOB_DEFAULT, NULL, NULL, errp);
> if (!s) {
> return;
> }
> @@ -232,6 +231,6 @@ void stream_start(const char *job_id, BlockDriverState *bs,
>
> s->on_error = on_error;
> s->common.co = qemu_coroutine_create(stream_run, s);
> - trace_stream_start(bs, base, s, s->common.co, opaque);
> + trace_stream_start(bs, base, s, s->common.co);
> qemu_coroutine_enter(s->common.co);
> }
> diff --git a/block/trace-events b/block/trace-events
> index 05fa13c..c12f91b 100644
> --- a/block/trace-events
> +++ b/block/trace-events
> @@ -20,11 +20,11 @@ bdrv_co_do_copy_on_readv(void *bs, int64_t offset, unsigned int bytes, int64_t c
>
> # block/stream.c
> stream_one_iteration(void *s, int64_t sector_num, int nb_sectors, int is_allocated) "s %p sector_num %"PRId64" nb_sectors %d is_allocated %d"
> -stream_start(void *bs, void *base, void *s, void *co, void *opaque) "bs %p base %p s %p co %p opaque %p"
> +stream_start(void *bs, void *base, void *s, void *co) "bs %p base %p s %p co %p"
>
> # block/commit.c
> commit_one_iteration(void *s, int64_t sector_num, int nb_sectors, int is_allocated) "s %p sector_num %"PRId64" nb_sectors %d is_allocated %d"
> -commit_start(void *bs, void *base, void *top, void *s, void *co, void *opaque) "bs %p base %p top %p s %p co %p opaque %p"
> +commit_start(void *bs, void *base, void *top, void *s, void *co) "bs %p base %p top %p s %p co %p"
>
> # block/mirror.c
> mirror_start(void *bs, void *s, void *co, void *opaque) "bs %p s %p co %p opaque %p"
> @@ -52,7 +52,6 @@ qmp_block_job_cancel(void *job) "job %p"
> qmp_block_job_pause(void *job) "job %p"
> qmp_block_job_resume(void *job) "job %p"
> qmp_block_job_complete(void *job) "job %p"
> -block_job_cb(void *bs, void *job, int ret) "bs %p job %p ret %d"
> qmp_block_stream(void *bs, void *job) "bs %p job %p"
>
> # block/raw-win32.c
> diff --git a/blockdev.c b/blockdev.c
> index 0ce305c..22a1280 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2905,31 +2905,6 @@ out:
> aio_context_release(aio_context);
> }
>
> -static void block_job_cb(void *opaque, 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);
> -
> - if (ret < 0) {
> - msg = strerror(-ret);
> - }
> -
> - if (block_job_is_cancelled(bs->job)) {
> - block_job_event_cancelled(bs->job);
> - } else {
> - block_job_event_completed(bs->job, msg);
> - }
> -}
> -
> void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
> bool has_base, const char *base,
> bool has_backing_file, const char *backing_file,
> @@ -2981,7 +2956,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
> base_name = has_backing_file ? backing_file : base_name;
>
> stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
> - has_speed ? speed : 0, on_error, block_job_cb, bs, &local_err);
> + has_speed ? speed : 0, on_error, &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> goto out;
> @@ -3084,12 +3059,12 @@ void qmp_block_commit(bool has_job_id, const char *job_id, const char *device,
> goto out;
> }
> commit_active_start(has_job_id ? job_id : NULL, bs, base_bs,
> - BLOCK_JOB_DEFAULT, speed, on_error, block_job_cb,
> - bs, &local_err, false);
> + BLOCK_JOB_DEFAULT, speed, on_error, NULL, NULL,
> + &local_err, false);
> } else {
> commit_start(has_job_id ? job_id : NULL, bs, base_bs, top_bs, speed,
> - on_error, block_job_cb, bs,
> - has_backing_file ? backing_file : NULL, &local_err);
> + on_error, has_backing_file ? backing_file : NULL,
> + &local_err);
> }
> if (local_err != NULL) {
> error_propagate(errp, local_err);
> @@ -3210,7 +3185,7 @@ static void do_drive_backup(DriveBackup *backup, BlockJobTxn *txn, Error **errp)
> backup_start(backup->job_id, bs, target_bs, backup->speed, backup->sync,
> bmap, backup->compress, backup->on_source_error,
> backup->on_target_error, BLOCK_JOB_DEFAULT,
> - block_job_cb, bs, txn, &local_err);
> + NULL, NULL, txn, &local_err);
> bdrv_unref(target_bs);
> if (local_err != NULL) {
> error_propagate(errp, local_err);
> @@ -3281,7 +3256,7 @@ void do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn, Error **errp)
> backup_start(backup->job_id, bs, target_bs, backup->speed, backup->sync,
> NULL, backup->compress, backup->on_source_error,
> backup->on_target_error, BLOCK_JOB_DEFAULT,
> - block_job_cb, bs, txn, &local_err);
> + NULL, NULL, txn, &local_err);
> if (local_err != NULL) {
> error_propagate(errp, local_err);
> }
> @@ -3360,8 +3335,7 @@ static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
> mirror_start(job_id, bs, target,
> has_replaces ? replaces : NULL,
> speed, granularity, buf_size, sync, backing_mode,
> - on_source_error, on_target_error, unmap,
> - block_job_cb, bs, errp);
> + on_source_error, on_target_error, unmap, errp);
> }
>
> void qmp_drive_mirror(DriveMirror *arg, Error **errp)
> diff --git a/blockjob.c b/blockjob.c
> index 017905a..e32cb78 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -38,6 +38,9 @@
> #include "qemu/timer.h"
> #include "qapi-event.h"
>
> +static void block_job_event_cancelled(BlockJob *job);
> +static void block_job_event_completed(BlockJob *job, const char *msg);
> +
> /* Transactional group of block jobs */
> struct BlockJobTxn {
>
> @@ -124,7 +127,6 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
> BlockBackend *blk;
> BlockJob *job;
>
> - assert(cb);
> if (bs->job) {
> error_setg(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs));
> return NULL;
> @@ -230,7 +232,20 @@ static void block_job_completed_single(BlockJob *job)
> job->driver->abort(job);
> }
> }
> - job->cb(job->opaque, job->ret);
> +
> + if (job->cb) {
> + job->cb(job->opaque, job->ret);
> + }
> + if (block_job_is_cancelled(job)) {
> + block_job_event_cancelled(job);
> + } else {
> + const char *msg = NULL;
> + if (job->ret < 0) {
> + msg = strerror(-job->ret);
> + }
> + block_job_event_completed(job, msg);
> + }
> +
> if (job->txn) {
> block_job_txn_unref(job->txn);
> }
> @@ -535,7 +550,7 @@ static void block_job_iostatus_set_err(BlockJob *job, int error)
> }
> }
>
> -void block_job_event_cancelled(BlockJob *job)
> +static void block_job_event_cancelled(BlockJob *job)
> {
> if (block_job_is_internal(job)) {
> return;
> @@ -549,7 +564,7 @@ void block_job_event_cancelled(BlockJob *job)
> &error_abort);
> }
>
> -void block_job_event_completed(BlockJob *job, const char *msg)
> +static void block_job_event_completed(BlockJob *job, const char *msg)
> {
> if (block_job_is_internal(job)) {
> return;
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 98f1c7f..dfbc53d 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -647,8 +647,6 @@ int is_windows_drive(const char *filename);
> * the new backing file if the job completes. Ignored if @base is %NULL.
> * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
> * @on_error: The action to take upon error.
> - * @cb: Completion function for the job.
> - * @opaque: Opaque pointer value passed to @cb.
> * @errp: Error object.
> *
> * Start a streaming operation on @bs. Clusters that are unallocated
> @@ -660,8 +658,7 @@ int is_windows_drive(const char *filename);
> */
> void stream_start(const char *job_id, BlockDriverState *bs,
> BlockDriverState *base, const char *backing_file_str,
> - int64_t speed, BlockdevOnError on_error,
> - BlockCompletionFunc *cb, void *opaque, Error **errp);
> + int64_t speed, BlockdevOnError on_error, Error **errp);
>
> /**
> * commit_start:
> @@ -672,16 +669,14 @@ void stream_start(const char *job_id, BlockDriverState *bs,
> * @base: Block device that will be written into, and become the new top.
> * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
> * @on_error: The action to take upon error.
> - * @cb: Completion function for the job.
> - * @opaque: Opaque pointer value passed to @cb.
> * @backing_file_str: String to use as the backing file in @top's overlay
> * @errp: Error object.
> *
> */
> void commit_start(const char *job_id, BlockDriverState *bs,
> BlockDriverState *base, BlockDriverState *top, int64_t speed,
> - BlockdevOnError on_error, BlockCompletionFunc *cb,
> - void *opaque, const char *backing_file_str, Error **errp);
> + BlockdevOnError on_error, const char *backing_file_str,
> + Error **errp);
> /**
> * commit_active_start:
> * @job_id: The id of the newly-created job, or %NULL to use the
> @@ -719,8 +714,6 @@ void commit_active_start(const char *job_id, BlockDriverState *bs,
> * @on_source_error: The action to take upon error reading from the source.
> * @on_target_error: The action to take upon error writing to the target.
> * @unmap: Whether to unmap target where source sectors only contain zeroes.
> - * @cb: Completion function for the job.
> - * @opaque: Opaque pointer value passed to @cb.
> * @errp: Error object.
> *
> * Start a mirroring operation on @bs. Clusters that are allocated
> @@ -734,9 +727,7 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
> MirrorSyncMode mode, BlockMirrorBackingMode backing_mode,
> BlockdevOnError on_source_error,
> BlockdevOnError on_target_error,
> - bool unmap,
> - BlockCompletionFunc *cb,
> - void *opaque, Error **errp);
> + bool unmap, Error **errp);
>
> /*
> * backup_start:
> diff --git a/include/block/blockjob.h b/include/block/blockjob.h
> index fdb31e0..928f0b8 100644
> --- a/include/block/blockjob.h
> +++ b/include/block/blockjob.h
> @@ -374,23 +374,6 @@ void block_job_resume(BlockJob *job);
> void block_job_enter(BlockJob *job);
>
> /**
> - * block_job_event_cancelled:
> - * @job: The job whose information is requested.
> - *
> - * Send a BLOCK_JOB_CANCELLED event for the specified job.
> - */
> -void block_job_event_cancelled(BlockJob *job);
> -
> -/**
> - * block_job_ready:
> - * @job: The job which is now ready to complete.
> - * @msg: Error message. Only present on failure.
> - *
> - * Send a BLOCK_JOB_COMPLETED event for the specified job.
> - */
> -void block_job_event_completed(BlockJob *job, const char *msg);
> -
> -/**
> * block_job_ready:
> * @job: The job which is now ready to complete.
> *
> --
> 2.7.4
>
Reviewed-by: Jeff Cody <jcody@redhat.com>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 5/7] Blockjobs: Internalize user_pause logic
2016-10-13 22:57 ` [Qemu-devel] [PATCH 5/7] Blockjobs: Internalize user_pause logic John Snow
2016-10-14 15:46 ` Kevin Wolf
@ 2016-10-26 4:50 ` Jeff Cody
1 sibling, 0 replies; 26+ messages in thread
From: Jeff Cody @ 2016-10-26 4:50 UTC (permalink / raw)
To: John Snow; +Cc: qemu-block, kwolf, xiecl.fnst, wency, qemu-devel, pbonzini
On Thu, Oct 13, 2016 at 06:57:00PM -0400, John Snow wrote:
> BlockJobs will begin hiding their state in preparation for some
> refactorings anyway, so let's internalize the user_pause mechanism
> instead of leaving it to callers to correctly manage.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> blockdev.c | 12 +++++-------
> blockjob.c | 22 ++++++++++++++++++++--
> include/block/blockjob.h | 26 ++++++++++++++++++++++++++
> 3 files changed, 51 insertions(+), 9 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 22a1280..1661d08 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3579,7 +3579,7 @@ void qmp_block_job_cancel(const char *device,
> force = false;
> }
>
> - if (job->user_paused && !force) {
> + if (block_job_user_paused(job) && !force) {
> error_setg(errp, "The block job for device '%s' is currently paused",
> device);
> goto out;
> @@ -3596,13 +3596,12 @@ void qmp_block_job_pause(const char *device, Error **errp)
> AioContext *aio_context;
> BlockJob *job = find_block_job(device, &aio_context, errp);
>
> - if (!job || job->user_paused) {
> + if (!job || block_job_user_paused(job)) {
> return;
> }
>
> - job->user_paused = true;
> trace_qmp_block_job_pause(job);
> - block_job_pause(job);
> + block_job_user_pause(job);
> aio_context_release(aio_context);
> }
>
> @@ -3611,14 +3610,13 @@ void qmp_block_job_resume(const char *device, Error **errp)
> AioContext *aio_context;
> BlockJob *job = find_block_job(device, &aio_context, errp);
>
> - if (!job || !job->user_paused) {
> + if (!job || !block_job_user_paused(job)) {
> return;
> }
>
> - job->user_paused = false;
> trace_qmp_block_job_resume(job);
> block_job_iostatus_reset(job);
> - block_job_resume(job);
> + block_job_user_resume(job);
> aio_context_release(aio_context);
> }
>
> diff --git a/blockjob.c b/blockjob.c
> index e32cb78..d118a1f 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -362,11 +362,22 @@ void block_job_pause(BlockJob *job)
> job->pause_count++;
> }
>
> +void block_job_user_pause(BlockJob *job)
> +{
> + job->user_paused = true;
> + block_job_pause(job);
> +}
> +
> static bool block_job_should_pause(BlockJob *job)
> {
> return job->pause_count > 0;
> }
>
> +bool block_job_user_paused(BlockJob *job)
> +{
> + return job ? job->user_paused : 0;
> +}
> +
> void coroutine_fn block_job_pause_point(BlockJob *job)
> {
> if (!block_job_should_pause(job)) {
> @@ -403,6 +414,14 @@ void block_job_resume(BlockJob *job)
> block_job_enter(job);
> }
>
> +void block_job_user_resume(BlockJob *job)
> +{
> + if (job && job->user_paused && job->pause_count > 0) {
> + job->user_paused = false;
> + block_job_resume(job);
> + }
> +}
> +
> void block_job_enter(BlockJob *job)
> {
> if (job->co && !job->busy) {
> @@ -626,8 +645,7 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockdevOnError on_err,
> }
> if (action == BLOCK_ERROR_ACTION_STOP) {
> /* make the pause user visible, which will be resumed from QMP. */
> - job->user_paused = true;
> - block_job_pause(job);
> + block_job_user_pause(job);
> block_job_iostatus_set_err(job, error);
> }
> return action;
> diff --git a/include/block/blockjob.h b/include/block/blockjob.h
> index 928f0b8..5b61140 100644
> --- a/include/block/blockjob.h
> +++ b/include/block/blockjob.h
> @@ -358,6 +358,23 @@ void coroutine_fn block_job_pause_point(BlockJob *job);
> void block_job_pause(BlockJob *job);
>
> /**
> + * block_job_user_pause:
> + * @job: The job to be paused.
> + *
> + * Asynchronously pause the specified job.
> + * Do not allow a resume until a matching call to block_job_user_resume.
> + */
> +void block_job_user_pause(BlockJob *job);
> +
> +/**
> + * block_job_paused:
> + * @job: The job to query.
> + *
> + * Returns true if the job is user-paused.
> + */
> +bool block_job_user_paused(BlockJob *job);
> +
> +/**
> * block_job_resume:
> * @job: The job to be resumed.
> *
> @@ -366,6 +383,15 @@ void block_job_pause(BlockJob *job);
> void block_job_resume(BlockJob *job);
>
> /**
> + * block_job_user_resume:
> + * @job: The job to be resumed.
> + *
> + * Resume the specified job.
> + * Must be paired with a preceding block_job_user_pause.
> + */
> +void block_job_user_resume(BlockJob *job);
> +
> +/**
> * block_job_enter:
> * @job: The job to enter.
> *
> --
> 2.7.4
>
Reviewed-by: Jeff Cody <jcody@redhat.com>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 6/7] blockjobs: split interface into public/private, Part 1
2016-10-13 22:57 ` [Qemu-devel] [PATCH 6/7] blockjobs: split interface into public/private, Part 1 John Snow
2016-10-14 15:46 ` Kevin Wolf
@ 2016-10-26 4:51 ` Jeff Cody
1 sibling, 0 replies; 26+ messages in thread
From: Jeff Cody @ 2016-10-26 4:51 UTC (permalink / raw)
To: John Snow; +Cc: qemu-block, kwolf, xiecl.fnst, wency, qemu-devel, pbonzini
On Thu, Oct 13, 2016 at 06:57:01PM -0400, John Snow wrote:
> To make it a little more obvious which functions are intended to be
> public interface and which are intended to be for use only by jobs
> themselves, split the interface into "public" and "private" files.
>
> Convert blockjobs (e.g. block/backup) to using the private interface.
> Leave blockdev and others on the public interface.
>
> There are remaining uses of private state by qemu-img, and several
> cases in blockdev.c and block/io.c where we grab job->blk for the
> purposes of acquiring an AIOContext.
>
> These will be corrected in future patches.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> block/backup.c | 2 +-
> block/commit.c | 2 +-
> block/mirror.c | 2 +-
> block/stream.c | 2 +-
> blockjob.c | 2 +-
> include/block/block.h | 3 +-
> include/block/blockjob.h | 205 +-------------------------------------
> include/block/blockjob_int.h | 232 +++++++++++++++++++++++++++++++++++++++++++
> tests/test-blockjob-txn.c | 2 +-
> tests/test-blockjob.c | 2 +-
> 10 files changed, 244 insertions(+), 210 deletions(-)
> create mode 100644 include/block/blockjob_int.h
>
> diff --git a/block/backup.c b/block/backup.c
> index 6a60ca8..6d12100 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -16,7 +16,7 @@
> #include "trace.h"
> #include "block/block.h"
> #include "block/block_int.h"
> -#include "block/blockjob.h"
> +#include "block/blockjob_int.h"
> #include "block/block_backup.h"
> #include "qapi/error.h"
> #include "qapi/qmp/qerror.h"
> diff --git a/block/commit.c b/block/commit.c
> index 475a375..d555600 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -15,7 +15,7 @@
> #include "qemu/osdep.h"
> #include "trace.h"
> #include "block/block_int.h"
> -#include "block/blockjob.h"
> +#include "block/blockjob_int.h"
> #include "qapi/error.h"
> #include "qapi/qmp/qerror.h"
> #include "qemu/ratelimit.h"
> diff --git a/block/mirror.c b/block/mirror.c
> index 4374fb4..c81b5e0 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -13,7 +13,7 @@
>
> #include "qemu/osdep.h"
> #include "trace.h"
> -#include "block/blockjob.h"
> +#include "block/blockjob_int.h"
> #include "block/block_int.h"
> #include "sysemu/block-backend.h"
> #include "qapi/error.h"
> diff --git a/block/stream.c b/block/stream.c
> index 7d6877d..906f7f3 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -14,7 +14,7 @@
> #include "qemu/osdep.h"
> #include "trace.h"
> #include "block/block_int.h"
> -#include "block/blockjob.h"
> +#include "block/blockjob_int.h"
> #include "qapi/error.h"
> #include "qapi/qmp/qerror.h"
> #include "qemu/ratelimit.h"
> diff --git a/blockjob.c b/blockjob.c
> index d118a1f..e6f0d97 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -27,7 +27,7 @@
> #include "qemu-common.h"
> #include "trace.h"
> #include "block/block.h"
> -#include "block/blockjob.h"
> +#include "block/blockjob_int.h"
> #include "block/block_int.h"
> #include "sysemu/block-backend.h"
> #include "qapi/qmp/qerror.h"
> diff --git a/include/block/block.h b/include/block/block.h
> index 107c603..89b5feb 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -7,16 +7,15 @@
> #include "qemu/coroutine.h"
> #include "block/accounting.h"
> #include "block/dirty-bitmap.h"
> +#include "block/blockjob.h"
> #include "qapi/qmp/qobject.h"
> #include "qapi-types.h"
> #include "qemu/hbitmap.h"
>
> /* block.c */
> typedef struct BlockDriver BlockDriver;
> -typedef struct BlockJob BlockJob;
> typedef struct BdrvChild BdrvChild;
> typedef struct BdrvChildRole BdrvChildRole;
> -typedef struct BlockJobTxn BlockJobTxn;
>
> typedef struct BlockDriverInfo {
> /* in bytes, 0 if irrelevant */
> diff --git a/include/block/blockjob.h b/include/block/blockjob.h
> index 5b61140..bfc8233 100644
> --- a/include/block/blockjob.h
> +++ b/include/block/blockjob.h
> @@ -28,78 +28,15 @@
>
> #include "block/block.h"
>
> -/**
> - * BlockJobDriver:
> - *
> - * A class type for block job driver.
> - */
> -typedef struct BlockJobDriver {
> - /** Derived BlockJob struct size */
> - size_t instance_size;
> -
> - /** String describing the operation, part of query-block-jobs QMP API */
> - BlockJobType job_type;
> -
> - /** Optional callback for job types that support setting a speed limit */
> - void (*set_speed)(BlockJob *job, int64_t speed, Error **errp);
> -
> - /** Optional callback for job types that need to forward I/O status reset */
> - void (*iostatus_reset)(BlockJob *job);
> -
> - /**
> - * Optional callback for job types whose completion must be triggered
> - * manually.
> - */
> - void (*complete)(BlockJob *job, Error **errp);
> -
> - /**
> - * If the callback is not NULL, it will be invoked when all the jobs
> - * belonging to the same transaction complete; or upon this job's
> - * completion if it is not in a transaction. Skipped if NULL.
> - *
> - * All jobs will complete with a call to either .commit() or .abort() but
> - * never both.
> - */
> - void (*commit)(BlockJob *job);
> -
> - /**
> - * If the callback is not NULL, it will be invoked when any job in the
> - * same transaction fails; or upon this job's failure (due to error or
> - * cancellation) if it is not in a transaction. Skipped if NULL.
> - *
> - * All jobs will complete with a call to either .commit() or .abort() but
> - * never both.
> - */
> - void (*abort)(BlockJob *job);
> -
> - /**
> - * If the callback is not NULL, it will be invoked when the job transitions
> - * into the paused state. Paused jobs must not perform any asynchronous
> - * I/O or event loop activity. This callback is used to quiesce jobs.
> - */
> - void coroutine_fn (*pause)(BlockJob *job);
> -
> - /**
> - * If the callback is not NULL, it will be invoked when the job transitions
> - * out of the paused state. Any asynchronous I/O or event loop activity
> - * should be restarted from this callback.
> - */
> - void coroutine_fn (*resume)(BlockJob *job);
> -
> - /*
> - * If the callback is not NULL, it will be invoked before the job is
> - * resumed in a new AioContext. This is the place to move any resources
> - * besides job->blk to the new AioContext.
> - */
> - void (*attached_aio_context)(BlockJob *job, AioContext *new_context);
> -} BlockJobDriver;
> +typedef struct BlockJobDriver BlockJobDriver;
> +typedef struct BlockJobTxn BlockJobTxn;
>
> /**
> * BlockJob:
> *
> * Long-running operation on a BlockDriverState.
> */
> -struct BlockJob {
> +typedef struct BlockJob {
> /** The job type, including the job vtable. */
> const BlockJobDriver *driver;
>
> @@ -198,7 +135,7 @@ struct BlockJob {
> /** Non-NULL if this job is part of a transaction */
> BlockJobTxn *txn;
> QLIST_ENTRY(BlockJob) txn_list;
> -};
> +} BlockJob;
>
> typedef enum BlockJobCreateFlags {
> BLOCK_JOB_DEFAULT = 0x00,
> @@ -227,76 +164,6 @@ BlockJob *block_job_next(BlockJob *job);
> BlockJob *block_job_get(const char *id);
>
> /**
> - * block_job_create:
> - * @job_id: The id of the newly-created job, or %NULL to have one
> - * generated automatically.
> - * @job_type: The class object for the newly-created job.
> - * @bs: The block
> - * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
> - * @cb: Completion function for the job.
> - * @opaque: Opaque pointer value passed to @cb.
> - * @errp: Error object.
> - *
> - * Create a new long-running block device job and return it. The job
> - * will call @cb asynchronously when the job completes. Note that
> - * @bs may have been closed at the time the @cb it is called. If
> - * this is the case, the job may be reported as either cancelled or
> - * completed.
> - *
> - * This function is not part of the public job interface; it should be
> - * called from a wrapper that is specific to the job type.
> - */
> -void *block_job_create(const char *job_id, const BlockJobDriver *driver,
> - BlockDriverState *bs, int64_t speed, int flags,
> - BlockCompletionFunc *cb, void *opaque, Error **errp);
> -
> -/**
> - * block_job_sleep_ns:
> - * @job: The job that calls the function.
> - * @clock: The clock to sleep on.
> - * @ns: How many nanoseconds to stop for.
> - *
> - * Put the job to sleep (assuming that it wasn't canceled) for @ns
> - * nanoseconds. Canceling the job will interrupt the wait immediately.
> - */
> -void block_job_sleep_ns(BlockJob *job, QEMUClockType type, int64_t ns);
> -
> -/**
> - * block_job_yield:
> - * @job: The job that calls the function.
> - *
> - * Yield the block job coroutine.
> - */
> -void block_job_yield(BlockJob *job);
> -
> -/**
> - * block_job_ref:
> - * @bs: The block device.
> - *
> - * Grab a reference to the block job. Should be paired with block_job_unref.
> - */
> -void block_job_ref(BlockJob *job);
> -
> -/**
> - * block_job_unref:
> - * @bs: The block device.
> - *
> - * Release reference to the block job and release resources if it is the last
> - * reference.
> - */
> -void block_job_unref(BlockJob *job);
> -
> -/**
> - * block_job_completed:
> - * @job: The job being completed.
> - * @ret: The status code.
> - *
> - * Call the completion function that was registered at creation time, and
> - * free @job.
> - */
> -void block_job_completed(BlockJob *job, int ret);
> -
> -/**
> * block_job_set_speed:
> * @job: The job to set the speed for.
> * @speed: The new value
> @@ -325,14 +192,6 @@ void block_job_cancel(BlockJob *job);
> void block_job_complete(BlockJob *job, Error **errp);
>
> /**
> - * block_job_is_cancelled:
> - * @job: The job being queried.
> - *
> - * Returns whether the job is scheduled for cancellation.
> - */
> -bool block_job_is_cancelled(BlockJob *job);
> -
> -/**
> * block_job_query:
> * @job: The job to get information about.
> *
> @@ -341,15 +200,6 @@ bool block_job_is_cancelled(BlockJob *job);
> BlockJobInfo *block_job_query(BlockJob *job, Error **errp);
>
> /**
> - * block_job_pause_point:
> - * @job: The job that is ready to pause.
> - *
> - * Pause now if block_job_pause() has been called. Block jobs that perform
> - * lots of I/O must call this between requests so that the job can be paused.
> - */
> -void coroutine_fn block_job_pause_point(BlockJob *job);
> -
> -/**
> * block_job_pause:
> * @job: The job to be paused.
> *
> @@ -392,22 +242,6 @@ void block_job_resume(BlockJob *job);
> void block_job_user_resume(BlockJob *job);
>
> /**
> - * block_job_enter:
> - * @job: The job to enter.
> - *
> - * Continue the specified job by entering the coroutine.
> - */
> -void block_job_enter(BlockJob *job);
> -
> -/**
> - * block_job_ready:
> - * @job: The job which is now ready to complete.
> - *
> - * Send a BLOCK_JOB_READY event for the specified job.
> - */
> -void block_job_event_ready(BlockJob *job);
> -
> -/**
> * block_job_cancel_sync:
> * @job: The job to be canceled.
> *
> @@ -453,37 +287,6 @@ int block_job_complete_sync(BlockJob *job, Error **errp);
> void block_job_iostatus_reset(BlockJob *job);
>
> /**
> - * block_job_error_action:
> - * @job: The job to signal an error for.
> - * @on_err: The error action setting.
> - * @is_read: Whether the operation was a read.
> - * @error: The error that was reported.
> - *
> - * Report an I/O error for a block job and possibly stop the VM. Return the
> - * action that was selected based on @on_err and @error.
> - */
> -BlockErrorAction block_job_error_action(BlockJob *job, BlockdevOnError on_err,
> - int is_read, int error);
> -
> -typedef void BlockJobDeferToMainLoopFn(BlockJob *job, void *opaque);
> -
> -/**
> - * block_job_defer_to_main_loop:
> - * @job: The job
> - * @fn: The function to run in the main loop
> - * @opaque: The opaque value that is passed to @fn
> - *
> - * Execute a given function in the main loop with the BlockDriverState
> - * AioContext acquired. Block jobs must call bdrv_unref(), bdrv_close(), and
> - * anything that uses bdrv_drain_all() in the main loop.
> - *
> - * The @job AioContext is held while @fn executes.
> - */
> -void block_job_defer_to_main_loop(BlockJob *job,
> - BlockJobDeferToMainLoopFn *fn,
> - void *opaque);
> -
> -/**
> * block_job_txn_new:
> *
> * Allocate and return a new block job transaction. Jobs can be added to the
> diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
> new file mode 100644
> index 0000000..8eced19
> --- /dev/null
> +++ b/include/block/blockjob_int.h
> @@ -0,0 +1,232 @@
> +/*
> + * Declarations for long-running block device operations
> + *
> + * Copyright (c) 2011 IBM Corp.
> + * Copyright (c) 2012 Red Hat, Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#ifndef BLOCKJOB_INT_H
> +#define BLOCKJOB_INT_H
> +
> +#include "block/blockjob.h"
> +#include "block/block.h"
> +
> +/**
> + * BlockJobDriver:
> + *
> + * A class type for block job driver.
> + */
> +struct BlockJobDriver {
> + /** Derived BlockJob struct size */
> + size_t instance_size;
> +
> + /** String describing the operation, part of query-block-jobs QMP API */
> + BlockJobType job_type;
> +
> + /** Optional callback for job types that support setting a speed limit */
> + void (*set_speed)(BlockJob *job, int64_t speed, Error **errp);
> +
> + /** Optional callback for job types that need to forward I/O status reset */
> + void (*iostatus_reset)(BlockJob *job);
> +
> + /**
> + * Optional callback for job types whose completion must be triggered
> + * manually.
> + */
> + void (*complete)(BlockJob *job, Error **errp);
> +
> + /**
> + * If the callback is not NULL, it will be invoked when all the jobs
> + * belonging to the same transaction complete; or upon this job's
> + * completion if it is not in a transaction. Skipped if NULL.
> + *
> + * All jobs will complete with a call to either .commit() or .abort() but
> + * never both.
> + */
> + void (*commit)(BlockJob *job);
> +
> + /**
> + * If the callback is not NULL, it will be invoked when any job in the
> + * same transaction fails; or upon this job's failure (due to error or
> + * cancellation) if it is not in a transaction. Skipped if NULL.
> + *
> + * All jobs will complete with a call to either .commit() or .abort() but
> + * never both.
> + */
> + void (*abort)(BlockJob *job);
> +
> + /**
> + * If the callback is not NULL, it will be invoked when the job transitions
> + * into the paused state. Paused jobs must not perform any asynchronous
> + * I/O or event loop activity. This callback is used to quiesce jobs.
> + */
> + void coroutine_fn (*pause)(BlockJob *job);
> +
> + /**
> + * If the callback is not NULL, it will be invoked when the job transitions
> + * out of the paused state. Any asynchronous I/O or event loop activity
> + * should be restarted from this callback.
> + */
> + void coroutine_fn (*resume)(BlockJob *job);
> +
> + /*
> + * If the callback is not NULL, it will be invoked before the job is
> + * resumed in a new AioContext. This is the place to move any resources
> + * besides job->blk to the new AioContext.
> + */
> + void (*attached_aio_context)(BlockJob *job, AioContext *new_context);
> +};
> +
> +/**
> + * block_job_create:
> + * @job_id: The id of the newly-created job, or %NULL to have one
> + * generated automatically.
> + * @job_type: The class object for the newly-created job.
> + * @bs: The block
> + * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
> + * @cb: Completion function for the job.
> + * @opaque: Opaque pointer value passed to @cb.
> + * @errp: Error object.
> + *
> + * Create a new long-running block device job and return it. The job
> + * will call @cb asynchronously when the job completes. Note that
> + * @bs may have been closed at the time the @cb it is called. If
> + * this is the case, the job may be reported as either cancelled or
> + * completed.
> + *
> + * This function is not part of the public job interface; it should be
> + * called from a wrapper that is specific to the job type.
> + */
> +void *block_job_create(const char *job_id, const BlockJobDriver *driver,
> + BlockDriverState *bs, int64_t speed, int flags,
> + BlockCompletionFunc *cb, void *opaque, Error **errp);
> +
> +/**
> + * block_job_sleep_ns:
> + * @job: The job that calls the function.
> + * @clock: The clock to sleep on.
> + * @ns: How many nanoseconds to stop for.
> + *
> + * Put the job to sleep (assuming that it wasn't canceled) for @ns
> + * nanoseconds. Canceling the job will interrupt the wait immediately.
> + */
> +void block_job_sleep_ns(BlockJob *job, QEMUClockType type, int64_t ns);
> +
> +/**
> + * block_job_yield:
> + * @job: The job that calls the function.
> + *
> + * Yield the block job coroutine.
> + */
> +void block_job_yield(BlockJob *job);
> +
> +/**
> + * block_job_ref:
> + * @bs: The block device.
> + *
> + * Grab a reference to the block job. Should be paired with block_job_unref.
> + */
> +void block_job_ref(BlockJob *job);
> +
> +/**
> + * block_job_unref:
> + * @bs: The block device.
> + *
> + * Release reference to the block job and release resources if it is the last
> + * reference.
> + */
> +void block_job_unref(BlockJob *job);
> +
> +/**
> + * block_job_completed:
> + * @job: The job being completed.
> + * @ret: The status code.
> + *
> + * Call the completion function that was registered at creation time, and
> + * free @job.
> + */
> +void block_job_completed(BlockJob *job, int ret);
> +
> +/**
> + * block_job_is_cancelled:
> + * @job: The job being queried.
> + *
> + * Returns whether the job is scheduled for cancellation.
> + */
> +bool block_job_is_cancelled(BlockJob *job);
> +
> +/**
> + * block_job_pause_point:
> + * @job: The job that is ready to pause.
> + *
> + * Pause now if block_job_pause() has been called. Block jobs that perform
> + * lots of I/O must call this between requests so that the job can be paused.
> + */
> +void coroutine_fn block_job_pause_point(BlockJob *job);
> +
> +/**
> + * block_job_enter:
> + * @job: The job to enter.
> + *
> + * Continue the specified job by entering the coroutine.
> + */
> +void block_job_enter(BlockJob *job);
> +
> +/**
> + * block_job_ready:
> + * @job: The job which is now ready to complete.
> + *
> + * Send a BLOCK_JOB_READY event for the specified job.
> + */
> +void block_job_event_ready(BlockJob *job);
> +
> +/**
> + * block_job_error_action:
> + * @job: The job to signal an error for.
> + * @on_err: The error action setting.
> + * @is_read: Whether the operation was a read.
> + * @error: The error that was reported.
> + *
> + * Report an I/O error for a block job and possibly stop the VM. Return the
> + * action that was selected based on @on_err and @error.
> + */
> +BlockErrorAction block_job_error_action(BlockJob *job, BlockdevOnError on_err,
> + int is_read, int error);
> +
> +typedef void BlockJobDeferToMainLoopFn(BlockJob *job, void *opaque);
> +
> +/**
> + * block_job_defer_to_main_loop:
> + * @job: The job
> + * @fn: The function to run in the main loop
> + * @opaque: The opaque value that is passed to @fn
> + *
> + * Execute a given function in the main loop with the BlockDriverState
> + * AioContext acquired. Block jobs must call bdrv_unref(), bdrv_close(), and
> + * anything that uses bdrv_drain_all() in the main loop.
> + *
> + * The @job AioContext is held while @fn executes.
> + */
> +void block_job_defer_to_main_loop(BlockJob *job,
> + BlockJobDeferToMainLoopFn *fn,
> + void *opaque);
> +
> +#endif
> diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
> index b79e0c6..f9afc3b 100644
> --- a/tests/test-blockjob-txn.c
> +++ b/tests/test-blockjob-txn.c
> @@ -13,7 +13,7 @@
> #include "qemu/osdep.h"
> #include "qapi/error.h"
> #include "qemu/main-loop.h"
> -#include "block/blockjob.h"
> +#include "block/blockjob_int.h"
> #include "sysemu/block-backend.h"
>
> typedef struct {
> diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
> index 18bf850..60b78a3 100644
> --- a/tests/test-blockjob.c
> +++ b/tests/test-blockjob.c
> @@ -13,7 +13,7 @@
> #include "qemu/osdep.h"
> #include "qapi/error.h"
> #include "qemu/main-loop.h"
> -#include "block/blockjob.h"
> +#include "block/blockjob_int.h"
> #include "sysemu/block-backend.h"
>
> static const BlockJobDriver test_block_job_driver = {
> --
> 2.7.4
>
Reviewed-by: Jeff Cody <jcody@redhat.com>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] blockjobs: fix documentation
2016-10-13 22:57 ` [Qemu-devel] [PATCH 7/7] blockjobs: fix documentation John Snow
2016-10-14 15:46 ` Kevin Wolf
@ 2016-10-26 4:51 ` Jeff Cody
1 sibling, 0 replies; 26+ messages in thread
From: Jeff Cody @ 2016-10-26 4:51 UTC (permalink / raw)
To: John Snow; +Cc: qemu-block, kwolf, xiecl.fnst, wency, qemu-devel, pbonzini
On Thu, Oct 13, 2016 at 06:57:02PM -0400, John Snow wrote:
> (Trivial)
>
> Fix wrong function names in documentation.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
> include/block/blockjob_int.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
> index 8eced19..10ebb38 100644
> --- a/include/block/blockjob_int.h
> +++ b/include/block/blockjob_int.h
> @@ -191,8 +191,8 @@ void coroutine_fn block_job_pause_point(BlockJob *job);
> void block_job_enter(BlockJob *job);
>
> /**
> - * block_job_ready:
> - * @job: The job which is now ready to complete.
> + * block_job_event_ready:
> + * @job: The job which is now ready to be completed.
> *
> * Send a BLOCK_JOB_READY event for the specified job.
> */
> --
> 2.7.4
>
Reviewed-by: Jeff Cody <jcody@redhat.com>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 0/7] blockjobs: preliminary refactoring work, Pt 1
2016-10-14 18:32 ` John Snow
@ 2016-10-26 4:52 ` Jeff Cody
2016-10-26 16:09 ` John Snow
0 siblings, 1 reply; 26+ messages in thread
From: Jeff Cody @ 2016-10-26 4:52 UTC (permalink / raw)
To: John Snow; +Cc: qemu-block, kwolf, xiecl.fnst, wency, qemu-devel, pbonzini
On Fri, Oct 14, 2016 at 02:32:55PM -0400, John Snow wrote:
>
>
> On 10/13/2016 06:56 PM, John Snow wrote:
> >This is a follow-up to patches 1-6 of:
> >[PATCH v2 00/11] blockjobs: Fix transactional race condition
> >
> >That series started trying to refactor blockjobs with the goal of
> >internalizing BlockJob state as a side effect of having gone through
> >the effort of figuring out which commands were "safe" to call on
> >a Job that has no coroutine object.
> >
> >I've split out the less contentious bits so I can move forward with my
> >original work of focusing on the transactional race condition in a
> >different series.
> >
> >Functionally the biggest difference here is the presence of "internal"
> >block jobs, which do not emit QMP events or show up in block query
> >requests. This is done for the sake of replication jobs, which should
> >not be interfering with the public jobs namespace.
> >
>
> I have v2 ready to send out correcting Kevin's comments in patch #01, but
> I'd like to have the Replication maintainers at Fujitsu take a look at how I
> have modified replication and at least 'ACK' the change.
>
> As a recap: I am creating "internal" block jobs that have no ID and
> therefore do not collide with the user-specified jobs namespace. This way
> users cannot query, cancel, pause, or otherwise accidentally interfere with
> the replication job lifetime.
>
> It also means that management layers such as libvirt will not be aware of
> the presence of such "internal" jobs.
>
> Relevant patches are 1-3. Please let me know if you have questions.
>
> Thanks,
> --John Snow
>
Looks good to me, once you address Kevin's comments in patch 1.
>
> >________________________________________________________________________________
> >
> >For convenience, this branch is available at:
> >https://github.com/jnsnow/qemu.git branch job-refactor-pt1
> >https://github.com/jnsnow/qemu/tree/job-refactor-pt1
> >
> >This version is tagged job-refactor-pt1-v1:
> >https://github.com/jnsnow/qemu/releases/tag/job-refactor-pt1-v1
> >
> >John Snow (7):
> > blockjobs: hide internal jobs from management API
> > blockjobs: Allow creating internal jobs
> > Replication/Blockjobs: Create replication jobs as internal
> > blockjob: centralize QMP event emissions
> > Blockjobs: Internalize user_pause logic
> > blockjobs: split interface into public/private, Part 1
> > blockjobs: fix documentation
> >
> > block/backup.c | 5 +-
> > block/commit.c | 10 +-
> > block/mirror.c | 28 +++--
> > block/replication.c | 14 +--
> > block/stream.c | 9 +-
> > block/trace-events | 5 +-
> > blockdev.c | 74 +++++--------
> > blockjob.c | 109 ++++++++++++++----
> > include/block/block.h | 3 +-
> > include/block/block_int.h | 26 ++---
> > include/block/blockjob.h | 257 +++++++------------------------------------
> > include/block/blockjob_int.h | 232 ++++++++++++++++++++++++++++++++++++++
> > qemu-img.c | 5 +-
> > tests/test-blockjob-txn.c | 5 +-
> > tests/test-blockjob.c | 4 +-
> > 15 files changed, 443 insertions(+), 343 deletions(-)
> > create mode 100644 include/block/blockjob_int.h
> >
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH 0/7] blockjobs: preliminary refactoring work, Pt 1
2016-10-26 4:52 ` Jeff Cody
@ 2016-10-26 16:09 ` John Snow
0 siblings, 0 replies; 26+ messages in thread
From: John Snow @ 2016-10-26 16:09 UTC (permalink / raw)
To: Jeff Cody; +Cc: kwolf, xiecl.fnst, qemu-block, qemu-devel, pbonzini
On 10/26/2016 12:52 AM, Jeff Cody wrote:
> On Fri, Oct 14, 2016 at 02:32:55PM -0400, John Snow wrote:
>>
>>
>> On 10/13/2016 06:56 PM, John Snow wrote:
>>> This is a follow-up to patches 1-6 of:
>>> [PATCH v2 00/11] blockjobs: Fix transactional race condition
>>>
>>> That series started trying to refactor blockjobs with the goal of
>>> internalizing BlockJob state as a side effect of having gone through
>>> the effort of figuring out which commands were "safe" to call on
>>> a Job that has no coroutine object.
>>>
>>> I've split out the less contentious bits so I can move forward with my
>>> original work of focusing on the transactional race condition in a
>>> different series.
>>>
>>> Functionally the biggest difference here is the presence of "internal"
>>> block jobs, which do not emit QMP events or show up in block query
>>> requests. This is done for the sake of replication jobs, which should
>>> not be interfering with the public jobs namespace.
>>>
>>
>> I have v2 ready to send out correcting Kevin's comments in patch #01, but
>> I'd like to have the Replication maintainers at Fujitsu take a look at how I
>> have modified replication and at least 'ACK' the change.
>>
>> As a recap: I am creating "internal" block jobs that have no ID and
>> therefore do not collide with the user-specified jobs namespace. This way
>> users cannot query, cancel, pause, or otherwise accidentally interfere with
>> the replication job lifetime.
>>
>> It also means that management layers such as libvirt will not be aware of
>> the presence of such "internal" jobs.
>>
>> Relevant patches are 1-3. Please let me know if you have questions.
>>
>> Thanks,
>> --John Snow
>>
>
> Looks good to me, once you address Kevin's comments in patch 1.
>
Excellent, gracias.
>>
>>> ________________________________________________________________________________
>>>
>>> For convenience, this branch is available at:
>>> https://github.com/jnsnow/qemu.git branch job-refactor-pt1
>>> https://github.com/jnsnow/qemu/tree/job-refactor-pt1
>>>
>>> This version is tagged job-refactor-pt1-v1:
>>> https://github.com/jnsnow/qemu/releases/tag/job-refactor-pt1-v1
>>>
>>> John Snow (7):
>>> blockjobs: hide internal jobs from management API
>>> blockjobs: Allow creating internal jobs
>>> Replication/Blockjobs: Create replication jobs as internal
>>> blockjob: centralize QMP event emissions
>>> Blockjobs: Internalize user_pause logic
>>> blockjobs: split interface into public/private, Part 1
>>> blockjobs: fix documentation
>>>
>>> block/backup.c | 5 +-
>>> block/commit.c | 10 +-
>>> block/mirror.c | 28 +++--
>>> block/replication.c | 14 +--
>>> block/stream.c | 9 +-
>>> block/trace-events | 5 +-
>>> blockdev.c | 74 +++++--------
>>> blockjob.c | 109 ++++++++++++++----
>>> include/block/block.h | 3 +-
>>> include/block/block_int.h | 26 ++---
>>> include/block/blockjob.h | 257 +++++++------------------------------------
>>> include/block/blockjob_int.h | 232 ++++++++++++++++++++++++++++++++++++++
>>> qemu-img.c | 5 +-
>>> tests/test-blockjob-txn.c | 5 +-
>>> tests/test-blockjob.c | 4 +-
>>> 15 files changed, 443 insertions(+), 343 deletions(-)
>>> create mode 100644 include/block/blockjob_int.h
>>>
>
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2016-10-26 16:09 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-13 22:56 [Qemu-devel] [PATCH 0/7] blockjobs: preliminary refactoring work, Pt 1 John Snow
2016-10-13 22:56 ` [Qemu-devel] [PATCH 1/7] blockjobs: hide internal jobs from management API John Snow
2016-10-14 12:58 ` Kevin Wolf
2016-10-14 17:32 ` John Snow
2016-10-13 22:56 ` [Qemu-devel] [PATCH 2/7] blockjobs: Allow creating internal jobs John Snow
2016-10-14 14:40 ` Kevin Wolf
2016-10-26 4:48 ` Jeff Cody
2016-10-13 22:56 ` [Qemu-devel] [PATCH 3/7] Replication/Blockjobs: Create replication jobs as internal John Snow
2016-10-14 14:40 ` Kevin Wolf
2016-10-26 4:48 ` Jeff Cody
2016-10-13 22:56 ` [Qemu-devel] [PATCH 4/7] blockjob: centralize QMP event emissions John Snow
2016-10-14 15:45 ` Kevin Wolf
2016-10-26 4:49 ` Jeff Cody
2016-10-13 22:57 ` [Qemu-devel] [PATCH 5/7] Blockjobs: Internalize user_pause logic John Snow
2016-10-14 15:46 ` Kevin Wolf
2016-10-26 4:50 ` Jeff Cody
2016-10-13 22:57 ` [Qemu-devel] [PATCH 6/7] blockjobs: split interface into public/private, Part 1 John Snow
2016-10-14 15:46 ` Kevin Wolf
2016-10-26 4:51 ` Jeff Cody
2016-10-13 22:57 ` [Qemu-devel] [PATCH 7/7] blockjobs: fix documentation John Snow
2016-10-14 15:46 ` Kevin Wolf
2016-10-26 4:51 ` Jeff Cody
2016-10-14 5:33 ` [Qemu-devel] [PATCH 0/7] blockjobs: preliminary refactoring work, Pt 1 no-reply
2016-10-14 18:32 ` John Snow
2016-10-26 4:52 ` Jeff Cody
2016-10-26 16:09 ` John Snow
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.