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

[Vladimir Sementsov-Ogievskiy]

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 <address@hidden>
>> ---
>>   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.
> 
> Max
> 
>> +    bdrv_unref(bs);
>> +
>> +    aio_context_release(aio_context);
>> +}
>> +


-- 
Best regards,
Vladimir