Re: [Qemu-devel] [PATCH v5 08/13] blockdev: Block device IO during internal snapshot transaction

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2912 bytes --]

On 20.05.2015 08:16, Fam Zheng wrote:
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   blockdev.c | 12 +++++++++---
>   1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 5eaf77e..7f763d9 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1262,6 +1262,7 @@ typedef struct InternalSnapshotState {
>       BlockDriverState *bs;
>       AioContext *aio_context;
>       QEMUSnapshotInfo sn;
> +    Error *blocker;
>   } InternalSnapshotState;
>   
>   static void internal_snapshot_prepare(BlkTransactionState *common,
> @@ -1300,6 +1301,10 @@ static void internal_snapshot_prepare(BlkTransactionState *common,
>       state->aio_context = bdrv_get_aio_context(bs);
>       aio_context_acquire(state->aio_context);
>   
> +    state->bs = bs;
> +    error_setg(&state->blocker, "internal snapshot in progress");
> +    bdrv_op_block(bs, BLOCK_OP_TYPE_DEVICE_IO, state->blocker);
> +
>       if (!bdrv_is_inserted(bs)) {
>           error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
>           return;
> @@ -1354,9 +1359,6 @@ static void internal_snapshot_prepare(BlkTransactionState *common,
>                            name, device);
>           return;
>       }
> -
> -    /* 4. succeed, mark a snapshot is created */
> -    state->bs = bs;
>   }

As far as I can see, the failed operation in a transaction is aborted, 
too. So with this pulled up, if the creation of the snapshot failed, 
internal_snapshot_abort() will try to delete the (non-existing) snapshot 
which will fail. What is saving us from an even worse fate 
(internal_snapshot_prepare() failing because a snapshot with the name 
already existed, which would then be deleted by 
internal_snapshot_abort()) is that sn->name will not be set until the 
snapshot is actually attempted to be taken (thus, bdrv_snapshot_delete() 
in internal_snapshot_abort() fails). Oh, and that sn->id_str will only 
be set by bdrv_snapshot_create() in case of success.

So the only visible result is " Konsole output Failed to delete snapshot 
with id '' and name '' on device 'disk' in abort: Can't find the snapshot".

One way of fixing this would probably to check whether sn->id_str[0] is 
set instead of state->bs in internal_snapshot_abort(). That is, if 
bdrv_snapshot_create() really always and only fills sn->id_str if it was 
successful.

Max

>   
>   static void internal_snapshot_abort(BlkTransactionState *common)
> @@ -1387,6 +1389,10 @@ static void internal_snapshot_clean(BlkTransactionState *common)
>       InternalSnapshotState *state = DO_UPCAST(InternalSnapshotState,
>                                                common, common);
>   
> +    if (state->bs) {
> +        bdrv_op_unblock(state->bs, BLOCK_OP_TYPE_DEVICE_IO, state->blocker);
> +        error_free(state->blocker);
> +    }
>       if (state->aio_context) {
>           aio_context_release(state->aio_context);
>       }


[-- Attachment #2: Type: text/html, Size: 3655 bytes --]