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 <vsement...@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
