Re: [Qemu-devel] [PATCH v9 12/13] block: introduce backup-top filter driver

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2026 bytes --]

On 26.08.19 18:13, Vladimir Sementsov-Ogievskiy wrote:
> Backup-top filter caches write operations and does copy-before-write
> operations.
> 
> The driver will be used in backup instead of write-notifiers.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/backup-top.h  |  37 +++++++
>  block/backup-top.c  | 245 ++++++++++++++++++++++++++++++++++++++++++++
>  block/Makefile.objs |   2 +
>  3 files changed, 284 insertions(+)
>  create mode 100644 block/backup-top.h
>  create mode 100644 block/backup-top.c

[...]

> +static void backup_top_refresh_filename(BlockDriverState *bs)
> +{
> +    if (bs->backing == NULL) {
> +        /*
> +         * we can be here after failed bdrv_attach_child in
> +         * bdrv_set_backing_hd
> +         */
> +        return;
> +    }
> +    bdrv_refresh_filename(bs->backing->bs);

bdrv_refresh_filename() should have done this already.

> +    pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),
> +            bs->backing->bs->filename);
> +}

[...]

> +BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
> +                                         const char *filter_node_name,
> +                                         Error **errp)
> +{
[...]
> +    assert(!(top->backing->shared_perm & BLK_PERM_WRITE));

Not wrong, but why?

> +
> +    return top;
> +}
> +
> +void bdrv_backup_top_set_bcs(BlockDriverState *bs, BlockCopyState *copy_state)
> +{
> +    BDRVBackupTopState *s = bs->opaque;
> +
> +    assert(blk_bs(copy_state->source) == bs->backing->bs);
> +    s->bcs = copy_state;
> +}
> +
> +void bdrv_backup_top_drop(BlockDriverState *bs)
> +{
> +    AioContext *aio_context = bdrv_get_aio_context(bs);
> +
> +    aio_context_acquire(aio_context);
> +
> +    bdrv_drained_begin(bs);
> +
> +    bdrv_child_try_set_perm(bs->backing, 0, BLK_PERM_ALL, &error_abort);

I would prefer a state->active = false and bdrv_child_refresh_perms().

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]