Re: [Qemu-devel] [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
> 
> 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?

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

[..]

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

[..]

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


-- 
Best regards,
Vladimir