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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

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.