Refactor backup_start as backup_job_create, which only creates the job,
but does not automatically start it. The old interface, 'backup_start',
is not kept in favor of limiting the number of nearly-identical iterfaces
that would have to be edited to keep up with QAPI changes in the future.
Callers that wish to synchronously start the backup_block_job can
instead just call block_job_start immediately after calling
backup_job_create.
Transactions are updated to use the new interface, calling block_job_start
only during the .commit phase, which helps prevent race conditions where
jobs may finish before we even finish building the transaction. This may
happen, for instance, during empty blockup jobs.
Reported-by: Vladimir Sementsov-Ogievskiy <address@hidden>
Signed-off-by: John Snow <address@hidden>
---
block/backup.c | 39 ++++++----
blockdev.c | 194 ++++++++++++++++++++++++++--------------------
blockjob.c | 9 ++-
include/block/block_int.h | 19 ++---
4 files changed, 149 insertions(+), 112 deletions(-)
diff --git a/block/backup.c b/block/backup.c
index 2229e26..5878ffe 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -474,13 +474,16 @@ static void coroutine_fn backup_run(void *opaque)
block_job_defer_to_main_loop(&job->common, backup_complete, data);
}
-void backup_start(const char *job_id, BlockDriverState *bs,
- BlockDriverState *target, int64_t speed,
- MirrorSyncMode sync_mode, BdrvDirtyBitmap *sync_bitmap,
- BlockdevOnError on_source_error,
- BlockdevOnError on_target_error,
- BlockCompletionFunc *cb, void *opaque,
- BlockJobTxn *txn, Error **errp)
+BlockJob *backup_job_create(const char *job_id,
+ BlockDriverState *bs,
+ BlockDriverState *target,
+ int64_t speed,
+ MirrorSyncMode sync_mode,
+ BdrvDirtyBitmap *sync_bitmap,
+ BlockdevOnError on_source_error,
+ BlockdevOnError on_target_error,
+ BlockCompletionFunc *cb, void *opaque,
+ BlockJobTxn *txn, Error **errp)
{
int64_t len;
BlockDriverInfo bdi;
@@ -492,46 +495,46 @@ void backup_start(const char *job_id, BlockDriverState
*bs,
if (bs == target) {
error_setg(errp, "Source and target cannot be the same");
- return;
+ return NULL;
}
if (!bdrv_is_inserted(bs)) {
error_setg(errp, "Device is not inserted: %s",
bdrv_get_device_name(bs));
- return;
+ return NULL;
}
if (!bdrv_is_inserted(target)) {
error_setg(errp, "Device is not inserted: %s",
bdrv_get_device_name(target));
- return;
+ return NULL;
}
if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) {
- return;
+ return NULL;
}
if (bdrv_op_is_blocked(target, BLOCK_OP_TYPE_BACKUP_TARGET, errp)) {
- return;
+ return NULL;
}
if (sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) {
if (!sync_bitmap) {
error_setg(errp, "must provide a valid bitmap name for "
"\"incremental\" sync mode");
- return;
+ return NULL;
}
/* Create a new bitmap, and freeze/disable this one. */
if (bdrv_dirty_bitmap_create_successor(bs, sync_bitmap, errp) < 0) {
- return;
+ return NULL;
}
} else if (sync_bitmap) {
error_setg(errp,
"a sync_bitmap was provided to backup_run, "
"but received an incompatible sync_mode (%s)",
MirrorSyncMode_lookup[sync_mode]);
- return;
+ return NULL;
}
len = bdrv_getlength(bs);
@@ -578,8 +581,8 @@ void backup_start(const char *job_id, BlockDriverState *bs,
job->common.len = len;
job->common.co = qemu_coroutine_create(backup_run, job);
block_job_txn_add_job(txn, &job->common);
- block_job_start(&job->common);
- return;
+
+ return &job->common;
error:
if (sync_bitmap) {
@@ -589,4 +592,6 @@ void backup_start(const char *job_id, BlockDriverState *bs,
blk_unref(job->target);
block_job_unref(&job->common);
}
+
+ return NULL;
}
diff --git a/blockdev.c b/blockdev.c
index 2161400..4b041d9 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1838,17 +1838,17 @@ typedef struct DriveBackupState {
BlockJob *job;
} DriveBackupState;
-static void do_drive_backup(const char *job_id, const char *device,
- const char *target, bool has_format,
- const char *format, enum MirrorSyncMode sync,
- bool has_mode, enum NewImageMode mode,
- bool has_speed, int64_t speed,
- bool has_bitmap, const char *bitmap,
- bool has_on_source_error,
- BlockdevOnError on_source_error,
- bool has_on_target_error,
- BlockdevOnError on_target_error,
- BlockJobTxn *txn, Error **errp);
+static BlockJob *do_drive_backup(const char *job_id, const char *device,
+ const char *target, bool has_format,
+ const char *format, enum MirrorSyncMode sync,
+ bool has_mode, enum NewImageMode mode,
+ bool has_speed, int64_t speed,
+ bool has_bitmap, const char *bitmap,
+ bool has_on_source_error,
+ BlockdevOnError on_source_error,
+ bool has_on_target_error,
+ BlockdevOnError on_target_error,
+ BlockJobTxn *txn, Error **errp);
static void drive_backup_prepare(BlkActionState *common, Error **errp)
{
@@ -1878,32 +1878,38 @@ static void drive_backup_prepare(BlkActionState
*common, Error **errp)
bdrv_drained_begin(blk_bs(blk));
state->bs = blk_bs(blk);
- do_drive_backup(backup->has_job_id ? backup->job_id : NULL,
- backup->device, backup->target,
- backup->has_format, backup->format,
- backup->sync,
- backup->has_mode, backup->mode,
- backup->has_speed, backup->speed,
- backup->has_bitmap, backup->bitmap,
- backup->has_on_source_error, backup->on_source_error,
- backup->has_on_target_error, backup->on_target_error,
- common->block_job_txn, &local_err);
+ state->job = do_drive_backup(backup->has_job_id ? backup->job_id : NULL,
+ backup->device, backup->target,
+ backup->has_format, backup->format,
+ backup->sync,
+ backup->has_mode, backup->mode,
+ backup->has_speed, backup->speed,
+ backup->has_bitmap, backup->bitmap,
+ backup->has_on_source_error,
+ backup->on_source_error,
+ backup->has_on_target_error,
+ backup->on_target_error,
+ common->block_job_txn, &local_err);
if (local_err) {
error_propagate(errp, local_err);
return;
}
+}
- state->job = state->bs->job;
+static void drive_backup_commit(BlkActionState *common)
+{
+ DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
+ if (state->job) {
+ block_job_start(state->job);
+ }
}
static void drive_backup_abort(BlkActionState *common)
{
DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
- BlockDriverState *bs = state->bs;
- /* Only cancel if it's the job we started */
- if (bs && bs->job && bs->job == state->job) {
- block_job_cancel_sync(bs->job);
+ if (state->job) {
+ block_job_cancel_sync(state->job);
}
}
@@ -1924,14 +1930,15 @@ typedef struct BlockdevBackupState {
AioContext *aio_context;
} BlockdevBackupState;
-static void do_blockdev_backup(const char *job_id, const char *device,
- const char *target, enum MirrorSyncMode sync,
- bool has_speed, int64_t speed,
- bool has_on_source_error,
- BlockdevOnError on_source_error,
- bool has_on_target_error,
- BlockdevOnError on_target_error,
- BlockJobTxn *txn, Error **errp);
+static BlockJob *do_blockdev_backup(const char *job_id, const char *device,
+ const char *target,
+ enum MirrorSyncMode sync,
+ bool has_speed, int64_t speed,
+ bool has_on_source_error,
+ BlockdevOnError on_source_error,
+ bool has_on_target_error,
+ BlockdevOnError on_target_error,
+ BlockJobTxn *txn, Error **errp);
static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
{
@@ -1971,28 +1978,35 @@ static void blockdev_backup_prepare(BlkActionState
*common, Error **errp)
state->bs = blk_bs(blk);
bdrv_drained_begin(state->bs);
- do_blockdev_backup(backup->has_job_id ? backup->job_id : NULL,
- backup->device, backup->target, backup->sync,
- backup->has_speed, backup->speed,
- backup->has_on_source_error, backup->on_source_error,
- backup->has_on_target_error, backup->on_target_error,
- common->block_job_txn, &local_err);
+ state->job = do_blockdev_backup(backup->has_job_id ? backup->job_id : NULL,
+ backup->device, backup->target,
+ backup->sync,
+ backup->has_speed, backup->speed,
+ backup->has_on_source_error,
+ backup->on_source_error,
+ backup->has_on_target_error,
+ backup->on_target_error,
+ common->block_job_txn, &local_err);
if (local_err) {
error_propagate(errp, local_err);
return;
}
+}
- state->job = state->bs->job;
+static void blockdev_backup_commit(BlkActionState *common)
+{
+ BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common,
common);
+ if (state->job) {
+ block_job_start(state->job);
+ }
}
static void blockdev_backup_abort(BlkActionState *common)
{
BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common,
common);
- BlockDriverState *bs = state->bs;
- /* Only cancel if it's the job we started */
- if (bs && bs->job && bs->job == state->job) {
- block_job_cancel_sync(bs->job);
+ if (state->job) {
+ block_job_cancel_sync(state->job);
}
}
@@ -2142,12 +2156,14 @@ static const BlkActionOps actions[] = {
[TRANSACTION_ACTION_KIND_DRIVE_BACKUP] = {
.instance_size = sizeof(DriveBackupState),
.prepare = drive_backup_prepare,
+ .commit = drive_backup_commit,
.abort = drive_backup_abort,
.clean = drive_backup_clean,
},
[TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP] = {
.instance_size = sizeof(BlockdevBackupState),
.prepare = blockdev_backup_prepare,
+ .commit = blockdev_backup_commit,
.abort = blockdev_backup_abort,
.clean = blockdev_backup_clean,
},
@@ -3155,22 +3171,23 @@ out:
aio_context_release(aio_context);
}
-static void do_drive_backup(const char *job_id, const char *device,
- const char *target, bool has_format,
- const char *format, enum MirrorSyncMode sync,
- bool has_mode, enum NewImageMode mode,
- bool has_speed, int64_t speed,
- bool has_bitmap, const char *bitmap,
- bool has_on_source_error,
- BlockdevOnError on_source_error,
- bool has_on_target_error,
- BlockdevOnError on_target_error,
- BlockJobTxn *txn, Error **errp)
+static BlockJob *do_drive_backup(const char *job_id, const char *device,
+ const char *target, bool has_format,
+ const char *format, enum MirrorSyncMode sync,
+ bool has_mode, enum NewImageMode mode,
+ bool has_speed, int64_t speed,
+ bool has_bitmap, const char *bitmap,
+ bool has_on_source_error,
+ BlockdevOnError on_source_error,
+ bool has_on_target_error,
+ BlockdevOnError on_target_error,
+ BlockJobTxn *txn, Error **errp)
{
BlockBackend *blk;
BlockDriverState *bs;
BlockDriverState *target_bs;
BlockDriverState *source = NULL;
+ BlockJob *job = NULL;
BdrvDirtyBitmap *bmap = NULL;
AioContext *aio_context;
QDict *options = NULL;
@@ -3195,7 +3212,7 @@ static void do_drive_backup(const char *job_id, const
char *device,
if (!blk) {
error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
"Device '%s' not found", device);
- return;
+ return NULL;
}
aio_context = blk_get_aio_context(blk);
@@ -3276,9 +3293,9 @@ static void do_drive_backup(const char *job_id, const
char *device,
}
}
- backup_start(job_id, bs, target_bs, speed, sync, bmap,
- on_source_error, on_target_error,
- block_job_cb, bs, txn, &local_err);
+ job = backup_job_create(job_id, bs, target_bs, speed, sync, bmap,
+ on_source_error, on_target_error,
+ block_job_cb, bs, txn, &local_err);
bdrv_unref(target_bs);
if (local_err != NULL) {
error_propagate(errp, local_err);
@@ -3287,6 +3304,7 @@ static void do_drive_backup(const char *job_id, const
char *device,
out:
aio_context_release(aio_context);
+ return job;
}
void qmp_drive_backup(bool has_job_id, const char *job_id,
@@ -3300,13 +3318,17 @@ void qmp_drive_backup(bool has_job_id, const char
*job_id,
bool has_on_target_error, BlockdevOnError
on_target_error,
Error **errp)
{
- return do_drive_backup(has_job_id ? job_id : NULL, device, target,
- has_format, format, sync,
- has_mode, mode, has_speed, speed,
- has_bitmap, bitmap,
- has_on_source_error, on_source_error,
- has_on_target_error, on_target_error,
- NULL, errp);
+ BlockJob *job;
+ job = do_drive_backup(has_job_id ? job_id : NULL, device, target,
+ has_format, format, sync,
+ has_mode, mode, has_speed, speed,
+ has_bitmap, bitmap,
+ has_on_source_error, on_source_error,
+ has_on_target_error, on_target_error,
+ NULL, errp);
+ if (job) {
+ block_job_start(job);
+ }
}
BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp)
@@ -3314,20 +3336,21 @@ BlockDeviceInfoList *qmp_query_named_block_nodes(Error
**errp)
return bdrv_named_nodes_list(errp);
}
-void do_blockdev_backup(const char *job_id, const char *device,
- const char *target, enum MirrorSyncMode sync,
- bool has_speed, int64_t speed,
- bool has_on_source_error,
- BlockdevOnError on_source_error,
- bool has_on_target_error,
- BlockdevOnError on_target_error,
- BlockJobTxn *txn, Error **errp)
+BlockJob *do_blockdev_backup(const char *job_id, const char *device,
+ const char *target, enum MirrorSyncMode sync,
+ bool has_speed, int64_t speed,
+ bool has_on_source_error,
+ BlockdevOnError on_source_error,
+ bool has_on_target_error,
+ BlockdevOnError on_target_error,
+ BlockJobTxn *txn, Error **errp)
{
BlockBackend *blk;
BlockDriverState *bs;
BlockDriverState *target_bs;
Error *local_err = NULL;
AioContext *aio_context;
+ BlockJob *job = NULL;
if (!has_speed) {
speed = 0;
@@ -3342,7 +3365,7 @@ void do_blockdev_backup(const char *job_id, const char
*device,
blk = blk_by_name(device);
if (!blk) {
error_setg(errp, "Device '%s' not found", device);
- return;
+ return NULL;
}
aio_context = blk_get_aio_context(blk);
@@ -3370,13 +3393,14 @@ void do_blockdev_backup(const char *job_id, const char
*device,
goto out;
}
}
- backup_start(job_id, bs, target_bs, speed, sync, NULL, on_source_error,
- on_target_error, block_job_cb, bs, txn, &local_err);
+ job = backup_job_create(job_id, bs, target_bs, speed, sync, NULL,
on_source_error,
+ on_target_error, block_job_cb, bs, txn,
&local_err);
if (local_err != NULL) {
error_propagate(errp, local_err);
}
out:
aio_context_release(aio_context);
+ return job;
}
void qmp_blockdev_backup(bool has_job_id, const char *job_id,
@@ -3389,11 +3413,15 @@ void qmp_blockdev_backup(bool has_job_id, const char
*job_id,
BlockdevOnError on_target_error,
Error **errp)
{
- do_blockdev_backup(has_job_id ? job_id : NULL, device, target,
- sync, has_speed, speed,
- has_on_source_error, on_source_error,
- has_on_target_error, on_target_error,
- NULL, errp);
+ BlockJob *job;
+ job = do_blockdev_backup(has_job_id ? job_id : NULL, device, target,
+ sync, has_speed, speed,
+ has_on_source_error, on_source_error,
+ has_on_target_error, on_target_error,
+ NULL, errp);
+ if (job) {
+ block_job_start(job);
+ }
}
/* Parameter check and block job starting for drive mirroring.
diff --git a/blockjob.c b/blockjob.c
index 0d07abc..74b19b9 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -117,9 +117,12 @@ static void block_job_detach_aio_context(void *opaque)
block_job_unref(job);
}
-void *block_job_create(const char *job_id, const BlockJobDriver *driver,
- BlockDriverState *bs, int64_t speed,
- BlockCompletionFunc *cb, void *opaque, Error **errp)
+void *block_job_create(const char *job_id,
+ const BlockJobDriver *driver,
+ BlockDriverState *bs,
+ int64_t speed,
+ BlockCompletionFunc *cb,
+ void *opaque, Error **errp)
{
BlockBackend *blk;
BlockJob *job;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 47665be..ba63a8e 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -745,7 +745,7 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
void *opaque, Error **errp);
/*
- * backup_start:
+ * backup_job_create:
* @job_id: The id of the newly-created job, or %NULL to use the
* device name of @bs.
* @bs: Block device to operate on.
@@ -759,16 +759,17 @@ void mirror_start(const char *job_id, BlockDriverState
*bs,
* @opaque: Opaque pointer value passed to @cb.
* @txn: Transaction that this job is part of (may be NULL).
*
- * Start a backup operation on @bs. Clusters in @bs are written to @target
+ * Create a backup operation on @bs. Clusters in @bs are written to @target
* until the job is cancelled or manually completed.
*/
-void backup_start(const char *job_id, BlockDriverState *bs,
- BlockDriverState *target, int64_t speed,
- MirrorSyncMode sync_mode, BdrvDirtyBitmap *sync_bitmap,
- BlockdevOnError on_source_error,
- BlockdevOnError on_target_error,
- BlockCompletionFunc *cb, void *opaque,
- BlockJobTxn *txn, Error **errp);
+BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
+ BlockDriverState *target, int64_t speed,
+ MirrorSyncMode sync_mode,
+ BdrvDirtyBitmap *sync_bitmap,
+ BlockdevOnError on_source_error,
+ BlockdevOnError on_target_error,
+ BlockCompletionFunc *cb, void *opaque,
+ BlockJobTxn *txn, Error **errp);
void hmp_drive_add_node(Monitor *mon, const char *optstr);