diff mbox series

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

Message ID 20200922091418.53562-3-f.gruenbichler@proxmox.com (mailing list archive)
State New, archived
Headers show
Series mirror: implement incremental and bitmap modes | expand

Commit Message

Fabian Grünbichler Sept. 22, 2020, 9:14 a.m. UTC 
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(-)

Comments

Max Reitz Oct. 1, 2020, 5:01 p.m. UTC | #1 
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
Fabian Grünbichler Oct. 2, 2020, 8:23 a.m. UTC | #2 
On October 1, 2020 7:01 pm, Max Reitz wrote:
> 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?

yes. this was a result of keeping the original patches #1 and #2, and 
doing the cleanup on-top as separate patches. I missed folding it into 
the first instead of (now combined) second patch.

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

I think it should be possible for this instance. for the other end of 
the job, merging back happens before setting the bitmap to un-busy, so we 
need to use _internal there. will add a comment for that one why we are 
allowed to do so.

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

Ack.
diff mbox series

Patch

diff --git a/block/mirror.c b/block/mirror.c
index d64c8203ef..bc4f4563d9 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -654,8 +654,6 @@  static int mirror_exit_common(Job *job)
         bdrv_unfreeze_backing_chain(mirror_top_bs, target_bs);
     }
 
-    bdrv_release_dirty_bitmap(s->dirty_bitmap);
-
     /* Make sure that the source BDS doesn't go away during bdrv_replace_node,
      * before we can call bdrv_drained_end */
     bdrv_ref(src);
@@ -755,6 +753,18 @@  static int mirror_exit_common(Job *job)
     blk_set_perm(bjob->blk, 0, BLK_PERM_ALL, &error_abort);
     blk_insert_bs(bjob->blk, mirror_top_bs, &error_abort);
 
+    if (s->sync_bitmap) {
+        if (s->bitmap_mode == BITMAP_SYNC_MODE_ALWAYS ||
+            (s->bitmap_mode == BITMAP_SYNC_MODE_ON_SUCCESS &&
+             job->ret == 0 && ret == 0)) {
+            /* Success; synchronize copy back to sync. */
+            bdrv_clear_dirty_bitmap(s->sync_bitmap, NULL);
+            bdrv_dirty_bitmap_merge_internal(s->sync_bitmap, s->dirty_bitmap,
+                                             NULL, true);
+        }
+    }
+    bdrv_release_dirty_bitmap(s->dirty_bitmap);
+
     bs_opaque->job = NULL;
 
     bdrv_drained_end(src);
@@ -1592,10 +1602,6 @@  static BlockJob *mirror_start_job(
                        " sync mode",
                        MirrorSyncMode_str(sync_mode));
             return NULL;
-        } else if (bitmap_mode != BITMAP_SYNC_MODE_NEVER) {
-            error_setg(errp,
-                       "Bitmap Sync Mode '%s' is not supported by Mirror",
-                       BitmapSyncMode_str(bitmap_mode));
         }
     } else if (bitmap) {
         error_setg(errp,
@@ -1612,6 +1618,12 @@  static BlockJob *mirror_start_job(
             return NULL;
         }
         granularity = bdrv_dirty_bitmap_granularity(bitmap);
+
+        if (bitmap_mode != BITMAP_SYNC_MODE_NEVER) {
+            if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_DEFAULT, errp)) {
+                return NULL;
+            }
+        }
     } else if (granularity == 0) {
         granularity = bdrv_get_default_bitmap_granularity(target);
     }
@@ -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);
         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;
     }
 
     if (!has_replaces) {