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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5248 bytes --]

On 17.01.19 13:13, 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
>>
>> Looks good to me overall, comments below:
>>
> 
> [..]
> 
>  >> +static coroutine_fn int backup_top_cbw(BlockDriverState *bs, uint64_t offset,
>  >> +                                          uint64_t bytes)
> 
> [..]
> 
>>> +
>>> +    /*
>>> +     * Features to be implemented:
>>> +     * F3. parallelize copying loop
>>> +     * F4. detect zeros
>>
>> Isn't there detect-zeroes for that?
> 
> Hmm interesting note. I've placed F4 here, as we do have detecting zeros by hand in
> current backup code. Should we drop it from backup, or switch to just enabling
> detect-zeros on target?

Off the top of my head I don't know a reason why we wouldn't just set
detect-zeroes.  Maybe because there may be other writers to target which
should not use that setting?  But that scenario just seems wrong to me.

>>> +     * F5. use block_status ?
>>> +     * F6. don't copy clusters which are already cached by COR [see F1]
>>> +     * F7. if target is ram-cache and it is full, there should be a possibility
>>> +     *     to drop not necessary data (cached by COR [see F1]) to handle CBW
>>> +     *     fast.
>>
>> Another idea: Read all dirty data from bs->backing (in parallel, more or
>> less), and then perform all writes (to bs->backing and s->target) in
>> parallel as well.
>>
>> (So the writes do not have to wait on one another)
> 
> Yes, we can continue guest write after read part of CBW, but we can't do it always,
> as we want to limit RAM usage to store all these in-flight requests.

But this code here already allocates a buffer to cover all of the area.

> And I think,
> in scheme with ram-cache, ram-cache itself should be actually a kind of storage
> for in-flight requests. And in this terminology, if ram-cache is full, we can't
> proceed with guest write. It's the same as we reached limit on in-flight requests
> count.
> 
> [..]
> 
>>> +
>>> +static int coroutine_fn backup_top_co_pdiscard(BlockDriverState *bs,
>>> +                                                  int64_t offset, int bytes)
>>
>> (Indentation)
> 
> looks like it was after renaming
> fleecing_hook
> to
> backup_top
> 
> will fix all

Ah :-)

> [..]
> 
>>> +
>>> +static int coroutine_fn backup_top_co_flush(BlockDriverState *bs)
>>> +{
>>> +    if (!bs->backing) {
>>> +        return 0;
>>> +    }
>>> +
>>> +    return bdrv_co_flush(bs->backing->bs);
>>
>> Why not the target as well?
> 
> Should we? It may be guest flush, why to flush backup target on it?

Hm...  Well, the current backup code didn't flush the target, and the
mirror filter doesn't either.  OK then.

Max

> [..]
> 
>>> +static void backup_top_child_perm(BlockDriverState *bs, BdrvChild *c,
>>> +                                       const BdrvChildRole *role,
>>> +                                       BlockReopenQueue *reopen_queue,
>>> +                                       uint64_t perm, uint64_t shared,
>>> +                                       uint64_t *nperm, uint64_t *nshared)
>>
>> (Indentation)
>>
>>> +{
>>> +    bdrv_filter_default_perms(bs, c, role, reopen_queue, perm, shared, nperm,
>>> +                              nshared);
>>> +
>>> +    if (role == &child_file) {
>>> +        /*
>>> +         * share write to target, to not interfere guest writes to it's disk
>>
>> *interfere with guest writes to its disk
>>
>>> +         * which will be in target backing chain
>>> +         */
>>> +        *nshared = *nshared | BLK_PERM_WRITE;
>>> +        *nperm = *nperm | BLK_PERM_WRITE;
>>
>> Why do you need to take the write permission?  This filter does not
>> perform any writes unless it receives writes, right?  (So
>> bdrv_filter_default_perms() should take care of this.)
> 
> Your commit shed a bit of light on permission system for me) Ok, I'll check, if
> that work without PERM_WRITE here.
> 
>>
>>> +    } else {
>>> +        *nperm = *nperm | BLK_PERM_CONSISTENT_READ;
>>> +    }
>>> +}
>>> +
> 
> Thank you! For other comments: argee or will-try
> 
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]