Re: [PATCH qemu 2/4] drive-mirror: add support for conditional and always bitmap sync modes

[Max Reitz]

On 22.09.20 11:14, Fabian Grünbichler wrote:
> From: John Snow <jsnow@redhat.com>
> 
> Teach mirror two new tricks for using bitmaps:
> 
> Always: no matter what, we synchronize the copy_bitmap back to the
> sync_bitmap. In effect, this allows us resume a failed mirror at a later
> date, since the target bdrv should be exactly in the state referenced by
> the bitmap.
> 
> Conditional: On success only, we sync the bitmap. This is akin to
> incremental backup modes; we can use this bitmap to later refresh a
> successfully created mirror, or possibly re-try the whole failed mirror
> if we are able to rollback the target to the state before starting the
> mirror.
> 
> Based on original work by John Snow.
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
>  block/mirror.c | 28 ++++++++++++++++++++--------
>  blockdev.c     |  3 +++
>  2 files changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index d64c8203ef..bc4f4563d9 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c

[...]

> @@ -1781,8 +1793,8 @@ static BlockJob *mirror_start_job(
>      }
>  
>      if (s->sync_mode == MIRROR_SYNC_MODE_BITMAP) {
> -        bdrv_merge_dirty_bitmap(s->dirty_bitmap, s->sync_bitmap,
> -                                NULL, &local_err);
> +        bdrv_dirty_bitmap_merge_internal(s->dirty_bitmap, s->sync_bitmap,
> +                                         NULL, true);

(Sorry for not catching it in the previous version, but) this hunk needs
to go into patch 1, doesn’t it?

Or, rather...  Do we need it at all?  Is there anything that would
prohibit just moving this merge call to before the set_busy call?
(Which again is probably something that should be done in patch 1.)

(If you decide to keep calling *_internal, I think it would be nice to
add a comment above the call stating why we have to use *_internal here
(because the sync bitmap is busy now), and why it’s safe to do so
(because blockdev.c and/or mirror_start_job() have already checked the
bitmap).  But if it’s possible to do the merge before marking the
sync_bitmap busy, we probably should rather do that.)

>          if (local_err) {
>              goto fail;
>          }
> diff --git a/blockdev.c b/blockdev.c
> index 6baa1a33f5..0fd30a392d 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3019,6 +3019,9 @@ static void blockdev_mirror_common(const char *job_id, 
> BlockDriverState *bs,
>          if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_ALLOW_RO, errp)) {
>              return;
>          }
> +    } else if (has_bitmap_mode) {
> +        error_setg(errp, "Cannot specify bitmap sync mode without a bitmap");
> +        return;
>      }

This too I would move into patch 1.

Max

signature.asc
Description: OpenPGP digital signature