From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45990) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bWs1d-0005kc-JY for qemu-devel@nongnu.org; Mon, 08 Aug 2016 17:24:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bWs1b-000112-4Q for qemu-devel@nongnu.org; Mon, 08 Aug 2016 17:23:57 -0400 References: <1470683381-16680-1-git-send-email-jsnow@redhat.com> <1470683381-16680-4-git-send-email-jsnow@redhat.com> From: John Snow Message-ID: Date: Mon, 8 Aug 2016 17:23:45 -0400 MIME-Version: 1.0 In-Reply-To: <1470683381-16680-4-git-send-email-jsnow@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/5] blockjob: refactor backup_start as backup_job_create List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-block@nongnu.org Cc: kwolf@redhat.com, vsementsov@virtuozzo.com, famz@redhat.com, jcody@redhat.com, qemu-devel@nongnu.org On 08/08/2016 03:09 PM, John Snow wrote: > 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 > Signed-off-by: John Snow > --- > 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); > } NACK, this will create problems for jobs that we never actually started. I'll have to think about it. Good indication this is 2.8 material. > } > > @@ -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); > >