From patchwork Fri Sep 30 22:00:48 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: John Snow X-Patchwork-Id: 9358989 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id B3B7A600C8 for ; Fri, 30 Sep 2016 22:20:56 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 8FB6F29F95 for ; Fri, 30 Sep 2016 22:20:56 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 7E9C329F98; Fri, 30 Sep 2016 22:20:56 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 7AD4A29F95 for ; Fri, 30 Sep 2016 22:20:55 +0000 (UTC) Received: from localhost ([::1]:47341 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bq6Ao-0004vu-Nz for patchwork-qemu-devel@patchwork.kernel.org; Fri, 30 Sep 2016 18:20:54 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55401) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bq5rl-0004FK-Hr for qemu-devel@nongnu.org; Fri, 30 Sep 2016 18:01:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bq5rj-00010i-F0 for qemu-devel@nongnu.org; Fri, 30 Sep 2016 18:01:13 -0400 Received: from mx1.redhat.com ([209.132.183.28]:56080) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bq5re-0000xw-0J; Fri, 30 Sep 2016 18:01:06 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 68E42C0528BB; Fri, 30 Sep 2016 22:01:05 +0000 (UTC) Received: from scv.usersys.redhat.com (vpn-59-41.rdu2.redhat.com [10.10.59.41]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u8UM0o5I019985; Fri, 30 Sep 2016 18:01:04 -0400 From: John Snow To: qemu-block@nongnu.org Date: Fri, 30 Sep 2016 18:00:48 -0400 Message-Id: <1475272849-19990-11-git-send-email-jsnow@redhat.com> In-Reply-To: <1475272849-19990-1-git-send-email-jsnow@redhat.com> References: <1475272849-19990-1-git-send-email-jsnow@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Fri, 30 Sep 2016 22:01:05 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 209.132.183.28 Subject: [Qemu-devel] [PATCH v2 10/11] blockjob: refactor backup_start as backup_job_create X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: kwolf@redhat.com, vsementsov@virtuozzo.com, famz@redhat.com, John Snow , jcody@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP 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 block backup jobs. Reported-by: Vladimir Sementsov-Ogievskiy Signed-off-by: John Snow --- block/backup.c | 26 ++++++++------- block/replication.c | 11 ++++--- blockdev.c | 81 +++++++++++++++++++++++++++++++---------------- include/block/block_int.h | 21 ++++++------ 4 files changed, 86 insertions(+), 53 deletions(-) diff --git a/block/backup.c b/block/backup.c index 7294169..aad69eb 100644 --- a/block/backup.c +++ b/block/backup.c @@ -527,7 +527,7 @@ static const BlockJobDriver backup_job_driver = { .attached_aio_context = backup_attached_aio_context, }; -void backup_start(const char *job_id, BlockDriverState *bs, +BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, BlockDriverState *target, int64_t speed, MirrorSyncMode sync_mode, BdrvDirtyBitmap *sync_bitmap, bool compress, @@ -546,52 +546,52 @@ 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 (compress && target->drv->bdrv_co_pwritev_compressed == NULL) { error_setg(errp, "Compression is not supported for this drive %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); @@ -638,8 +638,8 @@ void backup_start(const char *job_id, BlockDriverState *bs, bdrv_op_block_all(target, job->common.blocker); job->common.len = len; block_job_txn_add_job(txn, &job->common); - block_job_start(&job->common); - return; + + return &job->common; error: if (sync_bitmap) { @@ -649,4 +649,6 @@ void backup_start(const char *job_id, BlockDriverState *bs, blk_unref(job->target); block_job_unref(&job->common); } + + return NULL; } diff --git a/block/replication.c b/block/replication.c index b604b93..d9cdc36 100644 --- a/block/replication.c +++ b/block/replication.c @@ -409,6 +409,7 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode, int64_t active_length, hidden_length, disk_length; AioContext *aio_context; Error *local_err = NULL; + BlockJob *job; aio_context = bdrv_get_aio_context(bs); aio_context_acquire(aio_context); @@ -496,16 +497,18 @@ 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, - BLOCKDEV_ON_ERROR_REPORT, BLOCKDEV_ON_ERROR_REPORT, - backup_job_completed, s, NULL, &local_err); + job = backup_job_create("replication-backup", 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); if (local_err) { error_propagate(errp, local_err); backup_job_cleanup(s); aio_context_release(aio_context); return; } + block_job_start(job); break; default: aio_context_release(aio_context); diff --git a/blockdev.c b/blockdev.c index 0ac507f..37d78d3 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1866,7 +1866,7 @@ typedef struct DriveBackupState { BlockJob *job; } DriveBackupState; -static void do_drive_backup(DriveBackup *backup, BlockJobTxn *txn, +static BlockJob *do_drive_backup(DriveBackup *backup, BlockJobTxn *txn, Error **errp); static void drive_backup_prepare(BlkActionState *common, Error **errp) @@ -1890,23 +1890,27 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp) bdrv_drained_begin(bs); state->bs = bs; - do_drive_backup(backup, common->block_job_txn, &local_err); + state->job = do_drive_backup(backup, 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); } } @@ -1927,8 +1931,8 @@ typedef struct BlockdevBackupState { AioContext *aio_context; } BlockdevBackupState; -static void do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn, - Error **errp); +static BlockJob *do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn, + Error **errp); static void blockdev_backup_prepare(BlkActionState *common, Error **errp) { @@ -1961,23 +1965,27 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp) state->bs = bs; bdrv_drained_begin(state->bs); - do_blockdev_backup(backup, common->block_job_txn, &local_err); + state->job = do_blockdev_backup(backup, 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); } } @@ -2127,12 +2135,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, }, @@ -3126,11 +3136,13 @@ out: aio_context_release(aio_context); } -static void do_drive_backup(DriveBackup *backup, BlockJobTxn *txn, Error **errp) +static BlockJob *do_drive_backup(DriveBackup *backup, BlockJobTxn *txn, + Error **errp) { BlockDriverState *bs; BlockDriverState *target_bs; BlockDriverState *source = NULL; + BlockJob *job = NULL; BdrvDirtyBitmap *bmap = NULL; AioContext *aio_context; QDict *options = NULL; @@ -3159,7 +3171,7 @@ static void do_drive_backup(DriveBackup *backup, BlockJobTxn *txn, Error **errp) bs = qmp_get_root_bs(backup->device, errp); if (!bs) { - return; + return NULL; } aio_context = bdrv_get_aio_context(bs); @@ -3233,9 +3245,10 @@ 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, NULL, bs, txn, &local_err); + job = backup_job_create(backup->job_id, bs, target_bs, backup->speed, + backup->sync, bmap, backup->compress, + backup->on_source_error, backup->on_target_error, + NULL, bs, txn, &local_err); bdrv_unref(target_bs); if (local_err != NULL) { error_propagate(errp, local_err); @@ -3244,11 +3257,17 @@ static void do_drive_backup(DriveBackup *backup, BlockJobTxn *txn, Error **errp) out: aio_context_release(aio_context); + return job; } void qmp_drive_backup(DriveBackup *arg, Error **errp) { - return do_drive_backup(arg, NULL, errp); + + BlockJob *job; + job = do_drive_backup(arg, NULL, errp); + if (job) { + block_job_start(job); + } } BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp) @@ -3256,12 +3275,14 @@ BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp) return bdrv_named_nodes_list(errp); } -void do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn, Error **errp) +BlockJob *do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn, + Error **errp) { BlockDriverState *bs; BlockDriverState *target_bs; Error *local_err = NULL; AioContext *aio_context; + BlockJob *job = NULL; if (!backup->has_speed) { backup->speed = 0; @@ -3281,7 +3302,7 @@ void do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn, Error **errp) bs = qmp_get_root_bs(backup->device, errp); if (!bs) { - return; + return NULL; } aio_context = bdrv_get_aio_context(bs); @@ -3303,19 +3324,25 @@ void do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn, Error **errp) goto out; } } - backup_start(backup->job_id, bs, target_bs, backup->speed, backup->sync, - NULL, backup->compress, backup->on_source_error, - backup->on_target_error, NULL, bs, txn, &local_err); + job = backup_job_create(backup->job_id, bs, target_bs, backup->speed, + backup->sync, NULL, backup->compress, + backup->on_source_error, backup->on_target_error, + NULL, 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(BlockdevBackup *arg, Error **errp) { - do_blockdev_backup(arg, NULL, errp); + BlockJob *job; + job = do_blockdev_backup(arg, NULL, errp); + if (job) { + block_job_start(job); + } } /* Parameter check and block job starting for drive mirroring. diff --git a/include/block/block_int.h b/include/block/block_int.h index 686f6a8..738e4b4 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -737,7 +737,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. @@ -751,17 +751,18 @@ 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, - bool compress, - 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, + bool compress, + BlockdevOnError on_source_error, + BlockdevOnError on_target_error, + BlockCompletionFunc *cb, void *opaque, + BlockJobTxn *txn, Error **errp); /** * block_job_start: