Re: [PATCH v15 5/5] block/backup: use backup-top instead of write notifiers

[Max Reitz]

On 01.10.19 15:14, Vladimir Sementsov-Ogievskiy wrote:
> Drop write notifiers and use filter node instead.
> 
> = Changes =
> 
> 1. Add filter-node-name argument for backup qmp api. We have to do it
> in this commit, as 257 needs to be fixed.
> 
> 2. There are no more write notifiers here, so is_write_notifier
> parameter is dropped from block-copy paths.
> 
> 3. To sync with in-flight requests at job finish we now have drained
> removing of the filter, we don't need rw-lock.
> 
> 4. Block-copy is now using BdrvChildren instead of BlockBackends
> 
> 5. As backup-top owns these children, we also move block-copy state
> into backup-top's ownership.
> 
> = Iotest changes =
> 
> 56: op-blocker doesn't shoot now, as we set it on source, but then
> check on filter, when trying to start second backup.
> To keep the test we instead can catch another collision: both jobs will
> get 'drive0' job-id, as job-id parameter is unspecified. To prevent
> interleaving with file-posix locks (as they are dependent on config)
> let's use another target for second backup.
> 
> Also, it's obvious now that we'd like to drop this op-blocker at all
> and add a test-case for two backups from one node (to different
> destinations) actually works. But not in these series.
> 
> 141: Output changed: prepatch, "Node is in use" comes from bdrv_has_blk
> check inside qmp_blockdev_del. But we've dropped block-copy blk
> objects, so no more blk objects on source bs (job blk is on backup-top
> filter bs). New message is from op-blocker, which is the next check in
> qmp_blockdev_add.
> 
> 257: The test wants to emulate guest write during backup. They should
> go to filter node, not to original source node, of course. Therefore we
> need to specify filter node name and use it.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  qapi/block-core.json       |   8 +-
>  include/block/block-copy.h |  14 +-
>  include/block/block_int.h  |   1 +
>  block/backup-top.c         |  21 +--
>  block/backup.c             |  73 +++------
>  block/block-copy.c         |  81 +++-------
>  block/replication.c        |   2 +-
>  blockdev.c                 |   1 +
>  tests/qemu-iotests/056     |   8 +-
>  tests/qemu-iotests/141.out |   2 +-
>  tests/qemu-iotests/257     |   7 +-
>  tests/qemu-iotests/257.out | 306 ++++++++++++++++++-------------------
>  12 files changed, 237 insertions(+), 287 deletions(-)

[...]

> diff --git a/block/block-copy.c b/block/block-copy.c
> index fcb112da14..5404bc921d 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c

[...]

> @@ -218,8 +183,8 @@ static int coroutine_fn 
> block_copy_with_offload(BlockCopyState *s,
>      nr_clusters = DIV_ROUND_UP(nbytes, s->cluster_size);
>      bdrv_reset_dirty_bitmap(s->copy_bitmap, start,
>                              s->cluster_size * nr_clusters);
> -    ret = blk_co_copy_range(s->source, start, s->target, start, nbytes,
> -                            read_flags, s->write_flags);
> +    ret = bdrv_co_copy_range(s->source, start, s->target, start, nbytes,
> +                            0, s->write_flags);

The indentation’s off here.  I’ll fix it.

Reviewed-by: Max Reitz <address@hidden>

signature.asc
Description: OpenPGP digital signature