Re: [Qemu-block] [PATCH v8 4/7] block: introduce backup-top filter driver

[Max Reitz]

On 14.06.19 11:04, Vladimir Sementsov-Ogievskiy wrote:
> 13.06.2019 18:57, Max Reitz wrote:
>> On 29.05.19 17:46, 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  |  64 +++++++++
>>>   block/backup-top.c  | 322 ++++++++++++++++++++++++++++++++++++++++++++
>>>   block/Makefile.objs |   2 +
>>>   3 files changed, 388 insertions(+)
>>>   create mode 100644 block/backup-top.h
>>>   create mode 100644 block/backup-top.c
>>>
>>> diff --git a/block/backup-top.h b/block/backup-top.h
>>> new file mode 100644
>>> index 0000000000..788e18c358
>>> --- /dev/null
>>> +++ b/block/backup-top.h

[...]

>>> +/*
>>> + * bdrv_backup_top_append
>>> + *
>>> + * Append backup_top filter node above @source node. @target node will 
>>> receive
>>> + * the data backed up during CBE operations. New filter together with 
>>> @target
>>> + * node are attached to @source aio context.
>>> + *
>>> + * The resulting filter node is implicit.
>>
>> Why?  It’s just as easy for the caller to just make it implicit if it
>> should be.  (And usually the caller should decide that.)
> 
> Personally, I don't know what are the reasons for filters to bi implicit or 
> not,
> I just made it like other job-filters.. I can move making-implicit to the 
> caller
> or drop it at all (if it will work).

Nodes are implicit if they haven’t been added consciously by the user.
A node added by a block job can be non-implicit, too, as mirror shows;
If the user specifies the filter-node-name option, they will know about
the node, thus it is no longer implicit.

If the user doesn’t know about the node (they didn’t give the
filter-node-name option), the node is implicit.

[...]

>>> +static int coroutine_fn backup_top_co_flush(BlockDriverState *bs)
>>> +{
>>> +    if (!bs->backing) {
>>> +        return 0;
>>> +    }
>>> +
>>> +    return bdrv_co_flush(bs->backing->bs);
>>
>> Should we flush the target, too?
> 
> Hm, you've asked it already, on previous version :)

I wasn’t sure...

> Backup don't do it,
> so I just keep old behavior. And what is the reason to flush backup target
> on any guest flush?

Hm, well, what’s the reason not to do it?
Also, there are not only guest flushes.  bdrv_flush_all() exists, which
is called when the guest is stopped.  So who is going to flush the
target if not its parent?

[...]

>>> +
>>> +    if (role == &child_file) {
>>> +        /*
>>> +         * Target child
>>> +         *
>>> +         * Share write to target (child_file), to not interfere
>>> +         * with guest writes to its disk which may be in target backing 
>>> chain.
>>> +         */
>>> +        if (perm & BLK_PERM_WRITE) {
>>> +            *nshared = *nshared | BLK_PERM_WRITE;
>>
>> Why not always share WRITE on the target?
> 
> Hmm, it's a bad thing to share writes on target, so I'm trying to reduce 
> number of
> cases when we have share it.

Is it?  First of all, this filter doesn’t care.  It doesn’t even read
from the target (related note: we probably don’t need CONSISTENT_READ on
the target).

Second, there is generally going to be a parent on backup-top that has
the WRITE permission taken.  So this does not really effectively reduce
that number of cases.

>>> +        }
>>> +    } else {
>>> +        /* Source child */
>>> +        if (perm & BLK_PERM_WRITE) {
>>
>> Or WRITE_UNCHANGED, no?
> 
> Why? We don't need doing CBW for unchanged write.

But we will do it still, won’t we?

(If an unchanging write comes in, this driver will handle it just like a
normal write, will it not?)

[...]

>>> +
>>> +BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
>>> +                                         BlockDriverState *target,
>>> +                                         HBitmap *copy_bitmap,
>>> +                                         Error **errp)
>>> +{

[...]

>>> +}
>>
>> I guess it would be nice if it users could blockdev-add a backup-top
>> node to basically get a backup with sync=none.
>>
>> (The bdrv_open implementation should then create a new bitmap and
>> initialize it to be fully set.)
>>
>> But maybe it wouldn’t be so nice and I just have feverish dreams.
> 
> When series begun, I was trying to make exactly this - user-available filter,
> which may be used in separate, but you was against)

Past me is stupid.

> Maybe, not totally against, but I decided not to argue long, and instead make
> filter implicit and drop public api (like mirror and commit filters), but 
> place it
> in a separate file (no one will argue against putting large enough and 
> complete
> new feature, represented by object into a separate file :). And this actually
> makes it easy to publish this filter at some moment. And now I think it was
> good decision anyway, as we postponed arguing around API and around shared 
> dirty
> bitmaps.
> 
> So publishing the filter is future step.

OK, sure.

>>> +void bdrv_backup_top_set_progress_callback(
>>> +        BlockDriverState *bs, BackupTopProgressCallback progress_cb,
>>> +        void *progress_opaque)
>>> +{
>>> +    BDRVBackupTopState *s = bs->opaque;
>>> +
>>> +    s->progress_cb = progress_cb;
>>> +    s->progress_opaque = progress_opaque;
>>> +}
>>> +
>>> +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);
>>
>> Pre-existing in other jobs, but I think calling this function is
>> dangerous.  (Which is why I remove it in my “block: Ignore loosening
>> perm restrictions failures” series.)
> 
> Hmm, good thing.. Should I rebase on it?

It would help me at least.

>>> +    bdrv_replace_node(bs, backing_bs(bs), &error_abort);
>>> +    bdrv_set_backing_hd(bs, NULL, &error_abort);
>>
>> I think some of this function should be in a .bdrv_close()
>> implementation, for example this bdrv_set_backing_hd() call.
> 
> Why? We don't have .bdrv_open, so why to have .bdrv_close? I think, when
> we publish this filter most of _add() and _drop() will be refactored to
> open() and close(). Is there a real reason to implement .close() now?

Not really if it isn’t a usable block driver yet, no.

Max

signature.asc
Description: OpenPGP digital signature