[v5,3/4] block: Add blockdev-backup to transaction

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockdev.c       | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json |  2 ++
 2 files changed, 81 insertions(+)

On 12/17/2014 07:51 AM, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@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.

> +    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() ?

> +}
> +
> +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) {
> +        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 <jsnow@redhat.com>

On Wed, 12/17 16:20, John Snow wrote:
> On 12/17/2014 07:51 AM, Fam Zheng wrote:
> >Signed-off-by: Fam Zheng <famz@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 <jsnow@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