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