Re: [PATCH v13 00/15] backup-top filter driver for backup

[Vladimir Sementsov-Ogievskiy]

ops, I've sent unfinished message

On 25.09.2019 22:19, Vladimir Sementsov-Ogievskiy wrote:
> Ogh :(
> 
> And I realized that there is bigger problem with design:
> 
> Assume failed copy in filter request: we want to mark bits dirty again
> and release range lock on source.. But if we have some write reguests
> in parallel, they may already passed backup-top filter, and they are
> only waiting for range lock. When lock is free the will go on and will
> not see bitmap changes..
> 
> That means that we can't use range lock: waiting request must wait on
> backup-top level, but range lock will not work on it, as they will
> interfer with original write request.

With such design we can't mark bits dirty again. We can switch to other 
behavior: on failed block-copy in filter just cancel the whole 
block-job.. But actually I think both behaviors should be available for 
user:
1. if backup is important, better to fail guest writes if needed
2. if guest is important, better to fail backup job if failed to do 
copy-before-write

> 
> I have to rething it somehow, a kind of "intersecting requests" possibly
> will be kept. I still don't like that current backup write-notifier
> locks the whole region, even non-dirty bits, instead we should lock only
> the region which we are handling at the moment.
> 
> Patches 01-11 are still good themselves, as a preparation, let's keep them
> 
> Patches 12-13 are good, but range lock is not appropriate for backup..
> May be they will be used for rewriting copy-on-read filter to copy in
> filter code.. Still I'm not sure, as COR should work through block-copy
> finally, and may possibly reuse same locking.

better drop 12-13 for now

Patch 14 is good, let's keep it. It has correct abort() in 
backup_top_cbw(), it's not dependent on 12-13, and it's waiting for 
corrected combining of backup-top, backup and block-copy.

And patch 15 is bad, I'll rewrite it. So, 16 is not needed too.

> 
> On 20.09.2019 17:20, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> These series introduce backup-top driver. It's a filter-node, which
>> do copy-before-write operation. Mirror uses filter-node for handling
>> guest writes, let's move to filter-node (from write-notifiers) for
>> backup too.
>>
>> v11,v12 -> v13 changes:
>>
>> [v12 was two fixes in separate: [PATCH v12 0/2] backup: copy_range fixes]
>>
>> 01: new in v12, in v13 change comment
>> 02: in v12: add "Fixes: " to commit msg, in v13 add John's r-b
>> 05: rebase on 01
>> 07: rebase on 01. It still a clean movement, keep r-b
>>
>> Vladimir Sementsov-Ogievskiy (15):
>>     block/backup: fix max_transfer handling for copy_range
>>     block/backup: fix backup_cow_with_offload for last cluster
>>     block/backup: split shareable copying part from backup_do_cow
>>     block/backup: improve comment about image fleecing
>>     block/backup: introduce BlockCopyState
>>     block/backup: fix block-comment style
>>     block: move block_copy from block/backup.c to separate file
>>     block: teach bdrv_debug_breakpoint skip filters with backing
>>     iotests: prepare 124 and 257 bitmap querying for backup-top filter
>>     iotests: 257: drop unused Drive.device field
>>     iotests: 257: drop device_add
>>     block/io: refactor wait_serialising_requests
>>     block: add lock/unlock range functions
>>     block: introduce backup-top filter driver
>>     block/backup: use backup-top instead of write notifiers
>>
>>    qapi/block-core.json          |   8 +-
>>    block/backup-top.h            |  37 ++
>>    include/block/block-copy.h    |  84 ++++
>>    include/block/block_int.h     |   5 +
>>    block.c                       |  34 +-
>>    block/backup-top.c            | 240 ++++++++++++
>>    block/backup.c                | 440 ++++-----------------
>>    block/block-copy.c            | 346 ++++++++++++++++
>>    block/io.c                    |  68 +++-
>>    block/replication.c           |   2 +-
>>    blockdev.c                    |   1 +
>>    block/Makefile.objs           |   3 +
>>    block/trace-events            |  14 +-
>>    tests/qemu-iotests/056        |   8 +-
>>    tests/qemu-iotests/124        |  83 ++--
>>    tests/qemu-iotests/257        |  91 ++---
>>    tests/qemu-iotests/257.out    | 714 ++++++++++++++--------------------
>>    tests/qemu-iotests/iotests.py |  27 ++
>>    18 files changed, 1287 insertions(+), 918 deletions(-)
>>    create mode 100644 block/backup-top.h
>>    create mode 100644 include/block/block-copy.h
>>    create mode 100644 block/backup-top.c
>>    create mode 100644 block/block-copy.c
>>