On October 1, 2020 7:01 pm, Max Reitz wrote:
> On 22.09.20 11:14, Fabian Grünbichler wrote:
>> From: John Snow <js...@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.gruenbich...@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.
