On Wed, 12/17 16:20, John Snow wrote:
> On 12/17/2014 07:51 AM, Fam Zheng wrote:
> >Signed-off-by: Fam Zheng <f...@redhat.com>
> >---
> > blockdev.c | 79
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > qapi-schema.json | 2 ++
> > 2 files changed, 81 insertions(+)
> >
> >diff --git a/blockdev.c b/blockdev.c
> >index dbbab1d..9f9ae88 100644
> >--- a/blockdev.c
> >+++ b/blockdev.c
> >@@ -1559,6 +1559,79 @@ static void drive_backup_clean(BlkTransactionState
> >*common)
> > }
> > }
> >
> >+typedef struct BlockdevBackupState {
> >+ BlkTransactionState common;
> >+ BlockDriverState *bs;
> >+ BlockJob *job;
> >+ AioContext *aio_context;
> >+} BlockdevBackupState;
> >+
> >+static void blockdev_backup_prepare(BlkTransactionState *common, Error
> >**errp)
> >+{
> >+ BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common,
> >common);
> >+ BlockdevBackup *backup;
> >+ BlockDriverState *bs, *target;
> >+ Error *local_err = NULL;
> >+
> >+ assert(common->action->kind == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP);
> >+ backup = common->action->blockdev_backup;
> >+
> >+ bs = bdrv_find(backup->device);
> >+ if (!bs) {
> >+ error_set(errp, QERR_DEVICE_NOT_FOUND, backup->device);
> >+ return;
> >+ }
> >+
> >+ target = bdrv_find(backup->target);
> >+ if (!target) {
> >+ error_set(errp, QERR_DEVICE_NOT_FOUND, backup->target);
> >+ return;
> >+ }
> >+
> >+ /* AioContext is released in .clean() */
> >+ state->aio_context = bdrv_get_aio_context(bs);
> >+ if (state->aio_context != bdrv_get_aio_context(target)) {
> >+ state->aio_context = NULL;
> >+ error_setg(errp, "Backup between two IO threads is not
> >implemented");
> >+ return;
> >+ }
> >+ aio_context_acquire(state->aio_context);
> >+
> >+ qmp_blockdev_backup(backup->device, backup->target,
> >+ backup->sync,
> >+ backup->has_speed, backup->speed,
> >+ backup->has_on_source_error,
> >backup->on_source_error,
> >+ backup->has_on_target_error,
> >backup->on_target_error,
> >+ &local_err);
> >+ if (local_err) {
> >+ error_propagate(errp, local_err);
> >+ return;
> >+ }
> >+
> >+ state->bs = bdrv_find(backup->device);
>
> Just some questions:
>
> why not use 'bs' instead of calling the function again? Granted, it should
> work a second time if it worked once.
Yeah, it should be "bs", a mistake but not wrong.
>
> >+ state->job = state->bs->job;
>
> Why stash both bs and bs->job? Can the BDS change what its job member points
> to between .prepare() and .abort() ?
Copied from drive_backup_prepare. Actually there is a comment about this in
blockdev_backup_abort below:
>
> >+}
> >+
> >+static void blockdev_backup_abort(BlkTransactionState *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) {
I think there is one case this is useful: a transaction containing two block
jobs, which *will* fail and abort(). With this check, only the right (started)
block_job_cancel_sync is called.
> >+ block_job_cancel_sync(bs->job);
> >+ }
> >+}
> >+
> >+static void blockdev_backup_clean(BlkTransactionState *common)
> >+{
> >+ BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common,
> >common);
> >+
> >+ if (state->aio_context) {
> >+ aio_context_release(state->aio_context);
> >+ }
> >+}
> >+
> > static void abort_prepare(BlkTransactionState *common, Error **errp)
> > {
> > error_setg(errp, "Transaction aborted using Abort action");
> >@@ -1582,6 +1655,12 @@ static const BdrvActionOps actions[] = {
> > .abort = drive_backup_abort,
> > .clean = drive_backup_clean,
> > },
> >+ [TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP] = {
> >+ .instance_size = sizeof(BlockdevBackupState),
> >+ .prepare = blockdev_backup_prepare,
> >+ .abort = blockdev_backup_abort,
> >+ .clean = blockdev_backup_clean,
> >+ },
> > [TRANSACTION_ACTION_KIND_ABORT] = {
> > .instance_size = sizeof(BlkTransactionState),
> > .prepare = abort_prepare,
> >diff --git a/qapi-schema.json b/qapi-schema.json
> >index 47d99cf..fbfc52f 100644
> >--- a/qapi-schema.json
> >+++ b/qapi-schema.json
> >@@ -1260,11 +1260,13 @@
> > # drive-backup since 1.6
> > # abort since 1.6
> > # blockdev-snapshot-internal-sync since 1.7
> >+# blockdev-backup since 2.3
> > ##
> > { 'union': 'TransactionAction',
> > 'data': {
> > 'blockdev-snapshot-sync': 'BlockdevSnapshot',
> > 'drive-backup': 'DriveBackup',
> >+ 'blockdev-backup': 'BlockdevBackup',
> > 'abort': 'Abort',
> > 'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal'
> > } }
> >
>
> At any rate, regardless of the answers to my questions:
> Reviewed-by: John Snow <js...@redhat.com>
>
Thanks! I'm going to fix the above bdrv_find() while keeping your rev-by. I
hope that is OK for you. :)
Fam
