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

Vladimir Sementsov-Ogievskiy posted 15 patches 4 years, 6 months ago
Test docker-clang@ubuntu passed
Test docker-quick@centos7 passed
Test docker-mingw@fedora passed
Test checkpatch passed
Test asan passed
Test FreeBSD passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190920142056.12778-1-vsementsov@virtuozzo.com
Maintainers: Wen Congyang <wencongyang2@huawei.com>, Kevin Wolf <kwolf@redhat.com>, Xie Changlong <xiechanglong.d@gmail.com>, Eric Blake <eblake@redhat.com>, Max Reitz <mreitz@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>, Markus Armbruster <armbru@redhat.com>, Fam Zheng <fam@euphon.net>, John Snow <jsnow@redhat.com>
There is a newer version of this series
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
[PATCH v13 00/15] backup-top filter driver for backup
Posted by Vladimir Sementsov-Ogievskiy 4 years, 6 months ago
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

-- 
2.21.0


Re: [PATCH v13 00/15] backup-top filter driver for backup
Posted by Vladimir Sementsov-Ogievskiy 4 years, 6 months ago
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.

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.

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
> 

Re: [PATCH v13 00/15] backup-top filter driver for backup
Posted by Vladimir Sementsov-Ogievskiy 4 years, 6 months ago
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
>>

Re: [PATCH v13 00/15] backup-top filter driver for backup
Posted by Max Reitz 4 years, 6 months ago
On 25.09.19 21:28, Vladimir Sementsov-Ogievskiy wrote:
> 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.
I’ll drop 12 – 15.  14 is dead code without 15, so I’d rather not keep
it, actually.

Mxa

Re: [PATCH v13 00/15] backup-top filter driver for backup
Posted by Max Reitz 4 years, 6 months ago
On 20.09.19 16: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

Thanks, applied to my block branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block

Max

Re: [PATCH v13 00/15] backup-top filter driver for backup
Posted by Vladimir Sementsov-Ogievskiy 4 years, 6 months ago
20.09.2019 18:55, Max Reitz wrote:
> On 20.09.19 16: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
> 
> Thanks, applied to my block branch:
> 
> https://git.xanclic.moe/XanClic/qemu/commits/branch/block
> 

You made my day!
Thank you!!

-- 
Best regards,
Vladimir
Re: [PATCH v13 00/15] backup-top filter driver for backup
Posted by John Snow 4 years, 6 months ago

On 9/20/19 12:01 PM, Vladimir Sementsov-Ogievskiy wrote:
> 20.09.2019 18:55, Max Reitz wrote:
>> On 20.09.19 16: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
>>
>> Thanks, applied to my block branch:
>>
>> https://git.xanclic.moe/XanClic/qemu/commits/branch/block
>>
> 
> You made my day!
> Thank you!!
> 

Guess that definitively settles the need for the creation time write
notifier, huh? :)

Congrats!

--js