Re: [Qemu-devel] [PATCH v5 07/11] block: introduce backup-top filter driver

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

13.04.2019 19:08, Vladimir Sementsov-Ogievskiy wrote:
> 16.01.2019 19:02, Max Reitz wrote:
>> On 29.12.18 13:20, Vladimir Sementsov-Ogievskiy wrote:
>>> Backup-top filter does copy-before-write operation. It should be
>>> inserted above active disk and has a target node for CBW, like the
>>> following:
>>>
>>>      +-------+
>>>      | Guest |
>>>      +---+---+
>>>          |r,w
>>>          v
>>>      +---+-----------+  target   +---------------+
>>>      | backup_top    |---------->| target(qcow2) |
>>>      +---+-----------+   CBW     +---+-----------+
>>>          |
>>> backing |r,w
>>>          v
>>>      +---+---------+
>>>      | Active disk |
>>>      +-------------+
>>>
>>> The driver will be used in backup instead of write-notifiers.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   block/backup-top.h  |  43 +++++++
>>>   block/backup-top.c  | 306 ++++++++++++++++++++++++++++++++++++++++++++
>>>   block/Makefile.objs |   2 +
>>>   3 files changed, 351 insertions(+)
>>>   create mode 100644 block/backup-top.h
>>>   create mode 100644 block/backup-top.c
>>
> 
> [..]
> 
>>> +BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
>>> +                                         BlockDriverState *target,
>>> +                                         HBitmap *copy_bitmap,
>>> +                                         Error **errp)
>>> +{
>>> +    Error *local_err = NULL;
>>> +    BDRVBackupTopState *state;
>>> +    BlockDriverState *top = bdrv_new_open_driver(&bdrv_backup_top_filter,
>>> +                                                 NULL, BDRV_O_RDWR, errp);
>>> +
>>> +    if (!top) {
>>> +        return NULL;
>>> +    }
>>> +
>>> +    top->implicit = true;
>>> +    top->total_sectors = source->total_sectors;
>>> +    top->opaque = state = g_new0(BDRVBackupTopState, 1);
>>> +    state->copy_bitmap = copy_bitmap;
>>> +
>>> +    bdrv_ref(target);
>>> +    state->target = bdrv_attach_child(top, target, "target", &child_file, errp);
>>> +    if (!state->target) {
>>> +        bdrv_unref(target);
>>> +        bdrv_unref(top);
>>> +        return NULL;
>>> +    }
>>> +
>>> +    bdrv_set_aio_context(top, bdrv_get_aio_context(source));
>>> +    bdrv_set_aio_context(target, bdrv_get_aio_context(source));
>>> +
>>> +    bdrv_drained_begin(source);
>>> +
>>> +    bdrv_ref(top);
>>> +    bdrv_append(top, source, &local_err);
>>> +
>>> +    if (local_err) {
>>> +        bdrv_unref(top);
>>
>> This is done automatically by bdrv_append().
>>
>>> +    }
>>> +
>>> +    bdrv_drained_end(source);
>>> +
>>> +    if (local_err) {
>>> +        bdrv_unref_child(top, state->target);
>>> +        bdrv_unref(top);
>>> +        error_propagate(errp, local_err);
>>> +        return NULL;
>>> +    }
>>> +
>>> +    return top;
>>> +}
>>> +
>>> +void bdrv_backup_top_drop(BlockDriverState *bs)
>>> +{
>>> +    BDRVBackupTopState *s = bs->opaque;
>>> +
>>> +    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);
>>> +    bdrv_replace_node(bs, backing_bs(bs), &error_abort);
>>> +    bdrv_set_backing_hd(bs, NULL, &error_abort);
>>
>> This is done automatically in bdrv_close(), and after bs has been
>> replaced by backing_bs(bs), I don't think new requests should come in,
>> so I don't think this needs to be done here.
> 
> Following movement of backup_top back to job->blk becomes impossible then,
> if we don't share WRITE on source in backup_top_child_perm.
> 
> And I think, this function should drop all relations created by
> bdrv_backup_top_append.
> 
>>
>>> +
>>> +    bdrv_drained_end(bs);
>>> +
>>> +    if (s->target) {
>>> +        bdrv_unref_child(bs, s->target);
>>> +    }
>>
>> And this should be done in a .bdrv_close() implementation, I think.
>>

and therefore this one too. We don't have .bdrv_open, so I'd prefer not
have bdrv_close. We create this child in _top_append, seems logical to
unref it in _top_drop.


-- 
Best regards,
Vladimir