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

[Vladimir Sementsov-Ogievskiy]

14.06.2019 15:57, Max Reitz wrote:
> 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.
> 

Ok, I understand it. But it doesn't show, why it should be implicit?
Isn't it less buggy to make all filters explicit? We don't hide implicit nodes
from query-named-block-nodes (the only interface to explore the whole graph for 
now)
anyway. And we can't absolutely hide side effects of additional node in the 
graph.

So, is there any real benefit of supporting separation into implicit and 
explicit filters?
It seems for me that it only complicates things...
In other words, what will break if we make all filters explicit?

> [...]
> 
>>>> +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?

guest flushes will be slowed down?

> 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?
> 
> [...]

Backup job? But filter should not relay on it..

But should really filter do that job, or it is here only to do CBW? Maybe target
must have another parent which will care about flushing.

Ok, I think I'll flush target here too for safety, and leave a comment, that 
something
smarter would be better.
(or, if we decide to flush all children by default in generic code, I'll drop 
this handler)

> 
>>>> +
>>>> +    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.

Ok.

> 
>>>> +        }
>>>> +    } 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?)

No, it will not, I check this flag in backup_top_co_pwritev

> 
> [...]
> 
>>>> +
>>>> +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.

And now it's already staged, so I should.

> 
>>>> +    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
> 


-- 
Best regards,
Vladimir