Re: [Qemu-devel] [PATCH v6 09/13] blockdev: Block device IO during external snapshot transaction

[Max Reitz <mreitz@redhat.com>]

On 21.05.2015 08:42, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   blockdev.c | 18 ++++++++++++++++--
>   1 file changed, 16 insertions(+), 2 deletions(-)

Unchanged from v5, so the same question: Would it suffice to block I/O 
only inside external_snapshot_commit()?

> diff --git a/blockdev.c b/blockdev.c
> index 7f763d9..923fc90 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1404,6 +1404,7 @@ typedef struct ExternalSnapshotState {
>       BlockDriverState *old_bs;
>       BlockDriverState *new_bs;
>       AioContext *aio_context;
> +    Error *blocker;
>   } ExternalSnapshotState;
>   
>   static void external_snapshot_prepare(BlkTransactionState *common,
> @@ -1525,6 +1526,9 @@ static void external_snapshot_prepare(BlkTransactionState *common,
>       if (ret != 0) {
>           error_propagate(errp, local_err);
>       }
> +
> +    error_setg(&state->blocker, "snapshot in progress");
> +    bdrv_op_block(state->old_bs, BLOCK_OP_TYPE_DEVICE_IO, state->blocker);
>   }
>   
>   static void external_snapshot_commit(BlkTransactionState *common)
> @@ -1541,8 +1545,6 @@ static void external_snapshot_commit(BlkTransactionState *common)
>        * don't want to abort all of them if one of them fails the reopen */
>       bdrv_reopen(state->new_bs, state->new_bs->open_flags & ~BDRV_O_RDWR,
>                   NULL);
> -
> -    aio_context_release(state->aio_context);
>   }
>   
>   static void external_snapshot_abort(BlkTransactionState *common)
> @@ -1552,6 +1554,17 @@ static void external_snapshot_abort(BlkTransactionState *common)
>       if (state->new_bs) {
>           bdrv_unref(state->new_bs);
>       }
> +}
> +
> +static void external_snapshot_clean(BlkTransactionState *common)
> +{
> +    ExternalSnapshotState *state =
> +                             DO_UPCAST(ExternalSnapshotState, common, common);
> +
> +    if (state->old_bs) {

I'd prefer state->blocker, because this block only makes sense if it's 
non-NULL, and if it is non-NULL, state->old_bs will always be non-NULL, too.

Max

> +        bdrv_op_unblock(state->old_bs, BLOCK_OP_TYPE_DEVICE_IO, state->blocker);
> +        error_free(state->blocker);
> +    }
>       if (state->aio_context) {
>           aio_context_release(state->aio_context);
>       }
> @@ -1716,6 +1729,7 @@ static const BdrvActionOps actions[] = {
>           .prepare  = external_snapshot_prepare,
>           .commit   = external_snapshot_commit,
>           .abort = external_snapshot_abort,
> +        .clean = external_snapshot_clean,
>       },
>       [TRANSACTION_ACTION_KIND_DRIVE_BACKUP] = {
>           .instance_size = sizeof(DriveBackupState),