Re: [Qemu-devel] [PATCH 07/11] block: drive_backup transaction callback support

[Max Reitz <mreitz@redhat.com>]

On 2015-03-04 at 23:15, John Snow wrote:
> This patch actually implements the transactional callback system
> for the drive_backup transaction.
>
> (1) We manually pick up a reference to the bitmap if present to allow
>      its cleanup to be delayed until after all drive_backup jobs launched
>      by the transaction have fully completed.
>
> (2) We create a functional closure that envelops the original drive_backup
>      callback, to be able to intercept the completion status and return code
>      for the job.
>
> (3) We add the drive_backup_cb method for the drive_backup action, which
>      unpacks the completion information and invokes the final cleanup.
>
> (4) backup_transaction_complete will perform the final cleanup on the
>      backup job.
>
> (5) In the case of transaction cancellation, drive_backup_cb is still
>      responsible for cleaning up the mess we may have already made.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   block/backup.c            |  9 +++++++
>   blockdev.c                | 64 ++++++++++++++++++++++++++++++++++++++---------
>   include/block/block_int.h |  8 ++++++
>   3 files changed, 69 insertions(+), 12 deletions(-)
>
> diff --git a/block/backup.c b/block/backup.c
> index 4332df4..3673fb0 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -233,6 +233,15 @@ typedef struct {
>       int ret;
>   } BackupCompleteData;
>   
> +void backup_transaction_complete(BlockJob *job, int ret)
> +{
> +    BackupBlockJob *s = container_of(job, BackupBlockJob, common);
> +
> +    if (s->sync_bitmap) {
> +        bdrv_dirty_bitmap_decref(job->bs, s->sync_bitmap, ret, NULL);
> +    }
> +}
> +
>   static void backup_complete(BlockJob *job, void *opaque)
>   {
>       BackupBlockJob *s = container_of(job, BackupBlockJob, common);
> diff --git a/blockdev.c b/blockdev.c
> index 9e3b9e9..3ff14a7 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1363,14 +1363,6 @@ static void transaction_callback(void *opaque, int ret)
>   
>   typedef void (CallbackFn)(void *opaque, int ret);
>   
> -/* Temporary. Removed in the next patch. */
> -CallbackFn *new_transaction_wrapper(BlkTransactionState *common,
> -                                    void *opaque,
> -                                    void (*callback)(void *, int),
> -                                    void **new_opaque);
> -
> -void undo_transaction_wrapper(BlkTransactionState *common);
> -
>   /**
>    * Create a new transactional callback wrapper.
>    *
> @@ -1389,7 +1381,7 @@ void undo_transaction_wrapper(BlkTransactionState *common);
>    *
>    * @return The callback to be used instead of @callback.
>    */
> -CallbackFn *new_transaction_wrapper(BlkTransactionState *common,
> +static CallbackFn *new_transaction_wrapper(BlkTransactionState *common,
>                                              void *opaque,
>                                              CallbackFn *callback,
>                                              void **new_opaque)
> @@ -1416,7 +1408,7 @@ CallbackFn *new_transaction_wrapper(BlkTransactionState *common,
>   /**
>    * Undo any actions performed by the above call.
>    */
> -void undo_transaction_wrapper(BlkTransactionState *common)
> +static void undo_transaction_wrapper(BlkTransactionState *common)
>   {
>       BlkTransactionList *btl = common->list;
>       BlkTransactionState *bts;
> @@ -1449,6 +1441,7 @@ void undo_transaction_wrapper(BlkTransactionState *common)
>       blk_put_transaction_state(common);
>   }
>   
> +static void block_job_cb(void *opaque, int ret);
>   static void _drive_backup(const char *device, const char *target,
>                             bool has_format, const char *format,
>                             enum MirrorSyncMode sync,
> @@ -1767,6 +1760,9 @@ static void drive_backup_prepare(BlkTransactionState *common, Error **errp)
>       BlockDriverState *bs;
>       DriveBackup *backup;
>       Error *local_err = NULL;
> +    CallbackFn *cb;
> +    void *opaque;
> +    BdrvDirtyBitmap *bmap = NULL;
>   
>       assert(common->action->kind == TRANSACTION_ACTION_KIND_DRIVE_BACKUP);
>       backup = common->action->drive_backup;
> @@ -1777,6 +1773,19 @@ static void drive_backup_prepare(BlkTransactionState *common, Error **errp)
>           return;
>       }
>   
> +    /* BackupBlockJob is opaque to us, so look up the bitmap ourselves */
> +    if (backup->has_bitmap) {
> +        bmap = bdrv_find_dirty_bitmap(bs, backup->bitmap);
> +        if (!bmap) {
> +            error_setg(errp, "Bitmap '%s' could not be found", backup->bitmap);
> +            return;
> +        }
> +    }
> +
> +    /* Create our transactional callback wrapper,
> +       and register that we'd like to call .cb() later. */
> +    cb = new_transaction_wrapper(common, bs, block_job_cb, &opaque);
> +
>       /* AioContext is released in .clean() */
>       state->aio_context = bdrv_get_aio_context(bs);
>       aio_context_acquire(state->aio_context);
> @@ -1789,7 +1798,7 @@ static void drive_backup_prepare(BlkTransactionState *common, Error **errp)
>                     backup->has_bitmap, backup->bitmap,
>                     backup->has_on_source_error, backup->on_source_error,
>                     backup->has_on_target_error, backup->on_target_error,
> -                  NULL, NULL,
> +                  cb, opaque,
>                     &local_err);
>       if (local_err) {
>           error_propagate(errp, local_err);
> @@ -1798,6 +1807,11 @@ static void drive_backup_prepare(BlkTransactionState *common, Error **errp)
>   
>       state->bs = bs;
>       state->job = state->bs->job;
> +    /* Keep the job alive until .cb(), too. */
> +    block_job_incref(state->job);
> +    if (bmap) {
> +        bdrv_dirty_bitmap_incref(bmap);
> +    }
>   }
>   
>   static void drive_backup_abort(BlkTransactionState *common)
> @@ -1809,6 +1823,10 @@ static void drive_backup_abort(BlkTransactionState *common)
>       if (bs && bs->job && bs->job == state->job) {
>           block_job_cancel_sync(bs->job);
>       }
> +
> +    /* Undo any callback actions we may have done. Putting down references
> +     * obtained during prepare() is handled by cb(). */
> +    undo_transaction_wrapper(common);
>   }
>   
>   static void drive_backup_clean(BlkTransactionState *common)
> @@ -1820,6 +1838,27 @@ static void drive_backup_clean(BlkTransactionState *common)
>       }
>   }
>   
> +static void drive_backup_cb(BlkTransactionState *common)
> +{
> +    BlkTransactionData *btd = common->opaque;
> +    BlockDriverState *bs = btd->opaque;
> +    DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common);
> +
> +    assert(state->bs == bs);
> +    if (bs->job) {
> +        assert(state->job == bs->job);
> +    }
> +
> +    state->aio_context = bdrv_get_aio_context(bs);
> +    aio_context_acquire(state->aio_context);
> +
> +    /* Note: We also have the individual job's return code in btd->ret */
> +    backup_transaction_complete(state->job, common->list->status);
> +    block_job_decref(state->job);
> +
> +    aio_context_release(state->aio_context);
> +}
> +
>   typedef struct BlockdevBackupState {
>       BlkTransactionState common;
>       BlockDriverState *bs;
> @@ -2004,7 +2043,8 @@ static const BdrvActionOps actions[] = {
>           .instance_size = sizeof(DriveBackupState),
>           .prepare = drive_backup_prepare,
>           .abort = drive_backup_abort,
> -        .clean = drive_backup_clean
> +        .clean = drive_backup_clean,
> +        .cb = drive_backup_cb
>       },
>       [TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP] = {
>           .instance_size = sizeof(BlockdevBackupState),
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index e0d5561..731684d 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -619,6 +619,14 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
>                     BlockCompletionFunc *cb, void *opaque,
>                     Error **errp);
>   
> +/*
> + * backup_transaction_complete
> + * @job The BackupJob that was associated with a transaction

s/BackupJob/backup block job/ or s/BackupJob/backup job/? (there is no 
structure named "BackupJob", but this looks like there might be one)

> + * @ret Amalgamated return code for the entire transaction

Hm. The call to this function you're introducing in this patch will 
probably stay the only one so there won't be anyone who'll have to worry 
about what this means, but if there was, they probably won't reach a 
conclusive result.

I know what it means because I've seen patch 3 (right now it means 
"everything OR-ed together so it's 0 for success or some non-zero (maybe 
positive, maybe negative, depending on whether you have an even or an 
odd number of errors, and depending on whether the jobs return negative 
values for errors or not) for error"), but I wouldn't be able to infer 
it from this. At the least you should add that 0 means success and 
everything else means error (if you take my suggestion for patch 3, it 
would be 0 for success and -errno for error, where that error number is 
one of the errors encountered).

Other than that, looks good (as far as I can tell with my still limited 
insights into patch 3).

Max

> + */
> +void backup_transaction_complete(BlockJob *job, int ret);
> +
> +
>   void blk_dev_change_media_cb(BlockBackend *blk, bool load);
>   bool blk_dev_has_removable_media(BlockBackend *blk);
>   void blk_dev_eject_request(BlockBackend *blk, bool force);