This simplifies some interface matters; namely the initialization and
(later) merging the manifest back into the sync_bitmap if it was
provided.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/backup.c | 76 ++++++++++++++++++++++++--------------------------
 1 file changed, 37 insertions(+), 39 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index d7fdafebda..9cc5a7f6ca 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -38,7 +38,10 @@ typedef struct CowRequest {
 typedef struct BackupBlockJob {
     BlockJob common;
     BlockBackend *target;
+
     BdrvDirtyBitmap *sync_bitmap;
+    BdrvDirtyBitmap *copy_bitmap;
+
     MirrorSyncMode sync_mode;
     BitmapSyncMode bitmap_mode;
     BlockdevOnError on_source_error;
@@ -51,7 +54,6 @@ typedef struct BackupBlockJob {
     NotifierWithReturn before_write;
     QLIST_HEAD(, CowRequest) inflight_reqs;
 
-    HBitmap *copy_bitmap;
     bool use_copy_range;
     int64_t copy_range_size;
 
@@ -113,7 +115,7 @@ static int coroutine_fn backup_cow_with_bounce_buffer(BackupBlockJob *job,
     int write_flags = job->serialize_target_writes ? BDRV_REQ_SERIALISING : 0;
 
     assert(QEMU_IS_ALIGNED(start, job->cluster_size));
-    hbitmap_reset(job->copy_bitmap, start, job->cluster_size);
+    bdrv_reset_dirty_bitmap(job->copy_bitmap, start, job->cluster_size);
     nbytes = MIN(job->cluster_size, job->len - start);
     if (!*bounce_buffer) {
         *bounce_buffer = blk_blockalign(blk, job->cluster_size);
@@ -146,7 +148,7 @@ static int coroutine_fn backup_cow_with_bounce_buffer(BackupBlockJob *job,
 
     return nbytes;
 fail:
-    hbitmap_set(job->copy_bitmap, start, job->cluster_size);
+    bdrv_set_dirty_bitmap(job->copy_bitmap, start, job->cluster_size);
     return ret;
 
 }
@@ -169,12 +171,12 @@ static int coroutine_fn backup_cow_with_offload(BackupBlockJob *job,
     assert(QEMU_IS_ALIGNED(start, job->cluster_size));
     nbytes = MIN(job->copy_range_size, end - start);
     nr_clusters = DIV_ROUND_UP(nbytes, job->cluster_size);
-    hbitmap_reset(job->copy_bitmap, start, job->cluster_size * nr_clusters);
+    bdrv_reset_dirty_bitmap(job->copy_bitmap, start, job->cluster_size * nr_clusters);
     ret = blk_co_copy_range(blk, start, job->target, start, nbytes,
                             read_flags, write_flags);
     if (ret < 0) {
         trace_backup_do_cow_copy_range_fail(job, start, ret);
-        hbitmap_set(job->copy_bitmap, start, job->cluster_size * nr_clusters);
+        bdrv_set_dirty_bitmap(job->copy_bitmap, start, job->cluster_size * nr_clusters);
         return ret;
     }
 
@@ -202,7 +204,7 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
     cow_request_begin(&cow_request, job, start, end);
 
     while (start < end) {
-        if (!hbitmap_get(job->copy_bitmap, start)) {
+        if (!bdrv_dirty_bitmap_get(job->copy_bitmap, start)) {
             trace_backup_do_cow_skip(job, start);
             start += job->cluster_size;
             continue; /* already copied */
@@ -296,14 +298,16 @@ static void backup_abort(Job *job)
 static void backup_clean(Job *job)
 {
     BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
+    BlockDriverState *bs = blk_bs(s->target);
+
+    if (s->copy_bitmap) {
+        bdrv_release_dirty_bitmap(bs, s->copy_bitmap);
+        s->copy_bitmap = NULL;
+    }
+
     assert(s->target);
     blk_unref(s->target);
     s->target = NULL;
-
-    if (s->copy_bitmap) {
-        hbitmap_free(s->copy_bitmap);
-        s->copy_bitmap = NULL;
-    }
 }
 
 void backup_do_checkpoint(BlockJob *job, Error **errp)
@@ -318,7 +322,7 @@ void backup_do_checkpoint(BlockJob *job, Error **errp)
         return;
     }
 
-    hbitmap_set(backup_job->copy_bitmap, 0, backup_job->len);
+    bdrv_set_dirty_bitmap(backup_job->copy_bitmap, 0, backup_job->len);
 }
 
 static void backup_drain(BlockJob *job)
@@ -387,59 +391,49 @@ static bool bdrv_is_unallocated_range(BlockDriverState *bs,
 
 static int coroutine_fn backup_loop(BackupBlockJob *job)
 {
-    int ret;
     bool error_is_read;
     int64_t offset;
-    HBitmapIter hbi;
+    BdrvDirtyBitmapIter *bdbi;
     BlockDriverState *bs = blk_bs(job->common.blk);
+    int ret = 0;
 
-    hbitmap_iter_init(&hbi, job->copy_bitmap, 0);
-    while ((offset = hbitmap_iter_next(&hbi)) != -1) {
+    bdbi = bdrv_dirty_iter_new(job->copy_bitmap);
+    while ((offset = bdrv_dirty_iter_next(bdbi)) != -1) {
         if (job->sync_mode == MIRROR_SYNC_MODE_TOP &&
             bdrv_is_unallocated_range(bs, offset, job->cluster_size))
         {
-            hbitmap_reset(job->copy_bitmap, offset, job->cluster_size);
+            bdrv_set_dirty_bitmap(job->copy_bitmap, offset, job->cluster_size);
             continue;
         }
 
         do {
             if (yield_and_check(job)) {
-                return 0;
+                goto out;
             }
             ret = backup_do_cow(job, offset,
                                 job->cluster_size, &error_is_read, false);
             if (ret < 0 && backup_error_action(job, error_is_read, -ret) ==
                            BLOCK_ERROR_ACTION_REPORT)
             {
-                return ret;
+                goto out;
             }
         } while (ret < 0);
     }
 
-    return 0;
+ out:
+    bdrv_dirty_iter_free(bdbi);
+    return ret;
 }
 
 /* init copy_bitmap from sync_bitmap */
 static void backup_incremental_init_copy_bitmap(BackupBlockJob *job)
 {
-    uint64_t offset = 0;
-    uint64_t bytes = job->len;
-
-    while (bdrv_dirty_bitmap_next_dirty_area(job->sync_bitmap,
-                                             &offset, &bytes))
-    {
-        hbitmap_set(job->copy_bitmap, offset, bytes);
-
-        offset += bytes;
-        if (offset >= job->len) {
-            break;
-        }
-        bytes = job->len - offset;
-    }
+    bdrv_dirty_bitmap_merge_internal(job->copy_bitmap, job->sync_bitmap,
+                                     NULL, true);
 
     /* TODO job_progress_set_remaining() would make more sense */
     job_progress_update(&job->common.job,
-        job->len - hbitmap_count(job->copy_bitmap));
+                        job->len - bdrv_get_dirty_count(job->copy_bitmap));
 }
 
 static int coroutine_fn backup_run(Job *job, Error **errp)
@@ -456,7 +450,7 @@ static int coroutine_fn backup_run(Job *job, Error **errp)
     if (s->sync_mode == MIRROR_SYNC_MODE_BITMAP) {
         backup_incremental_init_copy_bitmap(s);
     } else {
-        hbitmap_set(s->copy_bitmap, 0, s->len);
+        bdrv_set_dirty_bitmap(s->copy_bitmap, 0, s->len);
     }
 
     s->before_write.notify = backup_before_write_notify;
@@ -549,7 +543,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
     BackupBlockJob *job = NULL;
     int ret;
     int64_t cluster_size;
-    HBitmap *copy_bitmap = NULL;
+    BdrvDirtyBitmap *copy_bitmap = NULL;
 
     assert(bs);
     assert(target);
@@ -619,7 +613,11 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
         goto error;
     }
 
-    copy_bitmap = hbitmap_alloc(len, ctz32(cluster_size));
+    copy_bitmap = bdrv_create_dirty_bitmap(target, cluster_size, NULL, errp);
+    if (!copy_bitmap) {
+        goto error;
+    }
+    bdrv_disable_dirty_bitmap(copy_bitmap);
 
     /* job->len is fixed, so we can't allow resize */
     job = block_job_create(job_id, &backup_job_driver, txn, bs,
@@ -670,7 +668,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
  error:
     if (copy_bitmap) {
         assert(!job || !job->copy_bitmap);
-        hbitmap_free(copy_bitmap);
+        bdrv_release_dirty_bitmap(bs, copy_bitmap);
     }
     if (sync_bitmap) {
         bdrv_reclaim_dirty_bitmap(bs, sync_bitmap, NULL);
-- 
2.21.0

On 03.07.19 23:55, John Snow wrote:
> This simplifies some interface matters; namely the initialization and
> (later) merging the manifest back into the sync_bitmap if it was
> provided.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/backup.c | 76 ++++++++++++++++++++++++--------------------------
>  1 file changed, 37 insertions(+), 39 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index d7fdafebda..9cc5a7f6ca 100644
> --- a/block/backup.c
> +++ b/block/backup.c

[...]

> @@ -202,7 +204,7 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
>      cow_request_begin(&cow_request, job, start, end);
>  
>      while (start < end) {
> -        if (!hbitmap_get(job->copy_bitmap, start)) {
> +        if (!bdrv_dirty_bitmap_get(job->copy_bitmap, start)) {
>              trace_backup_do_cow_skip(job, start);
>              start += job->cluster_size;
>              continue; /* already copied */
> @@ -296,14 +298,16 @@ static void backup_abort(Job *job)
>  static void backup_clean(Job *job)
>  {
>      BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
> +    BlockDriverState *bs = blk_bs(s->target);

I’d prefer to call it target_bs, because “bs” is usually the source node.

Which brings me to a question: Why is the copy bitmap assigned to the
target in the first place?  Wouldn’t it make more sense on the source?

> +
> +    if (s->copy_bitmap) {
> +        bdrv_release_dirty_bitmap(bs, s->copy_bitmap);
> +        s->copy_bitmap = NULL;
> +    }
> +
>      assert(s->target);
>      blk_unref(s->target);
>      s->target = NULL;
> -
> -    if (s->copy_bitmap) {
> -        hbitmap_free(s->copy_bitmap);
> -        s->copy_bitmap = NULL;
> -    }
>  }
>  
>  void backup_do_checkpoint(BlockJob *job, Error **errp)

[...]

> @@ -387,59 +391,49 @@ static bool bdrv_is_unallocated_range(BlockDriverState *bs,
>  
>  static int coroutine_fn backup_loop(BackupBlockJob *job)
>  {
> -    int ret;
>      bool error_is_read;
>      int64_t offset;
> -    HBitmapIter hbi;
> +    BdrvDirtyBitmapIter *bdbi;
>      BlockDriverState *bs = blk_bs(job->common.blk);
> +    int ret = 0;
>  
> -    hbitmap_iter_init(&hbi, job->copy_bitmap, 0);
> -    while ((offset = hbitmap_iter_next(&hbi)) != -1) {
> +    bdbi = bdrv_dirty_iter_new(job->copy_bitmap);
> +    while ((offset = bdrv_dirty_iter_next(bdbi)) != -1) {
>          if (job->sync_mode == MIRROR_SYNC_MODE_TOP &&
>              bdrv_is_unallocated_range(bs, offset, job->cluster_size))
>          {
> -            hbitmap_reset(job->copy_bitmap, offset, job->cluster_size);
> +            bdrv_set_dirty_bitmap(job->copy_bitmap, offset, job->cluster_size);

Should be reset.

>              continue;
>          }
>  
>          do {
>              if (yield_and_check(job)) {
> -                return 0;
> +                goto out;
>              }
>              ret = backup_do_cow(job, offset,
>                                  job->cluster_size, &error_is_read, false);
>              if (ret < 0 && backup_error_action(job, error_is_read, -ret) ==
>                             BLOCK_ERROR_ACTION_REPORT)
>              {
> -                return ret;
> +                goto out;
>              }
>          } while (ret < 0);
>      }
>  
> -    return 0;
> + out:
> +    bdrv_dirty_iter_free(bdbi);
> +    return ret;
>  }
>  
>  /* init copy_bitmap from sync_bitmap */
>  static void backup_incremental_init_copy_bitmap(BackupBlockJob *job)
>  {
> -    uint64_t offset = 0;
> -    uint64_t bytes = job->len;
> -
> -    while (bdrv_dirty_bitmap_next_dirty_area(job->sync_bitmap,
> -                                             &offset, &bytes))
> -    {
> -        hbitmap_set(job->copy_bitmap, offset, bytes);
> -
> -        offset += bytes;
> -        if (offset >= job->len) {
> -            break;
> -        }
> -        bytes = job->len - offset;
> -    }
> +    bdrv_dirty_bitmap_merge_internal(job->copy_bitmap, job->sync_bitmap,
> +                                     NULL, true);

How about asserting that this was successful?

>  
>      /* TODO job_progress_set_remaining() would make more sense */
>      job_progress_update(&job->common.job,
> -        job->len - hbitmap_count(job->copy_bitmap));
> +                        job->len - bdrv_get_dirty_count(job->copy_bitmap));
>  }
>  
>  static int coroutine_fn backup_run(Job *job, Error **errp)

[...]

> @@ -670,7 +668,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
>   error:
>      if (copy_bitmap) {
>          assert(!job || !job->copy_bitmap);
> -        hbitmap_free(copy_bitmap);
> +        bdrv_release_dirty_bitmap(bs, copy_bitmap);

If you decide to keep the copy_bitmap on the target, s/bs/target/.

Max

On 7/4/19 1:29 PM, Max Reitz wrote:
> On 03.07.19 23:55, John Snow wrote:
>> This simplifies some interface matters; namely the initialization and
>> (later) merging the manifest back into the sync_bitmap if it was
>> provided.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  block/backup.c | 76 ++++++++++++++++++++++++--------------------------
>>  1 file changed, 37 insertions(+), 39 deletions(-)
>>
>> diff --git a/block/backup.c b/block/backup.c
>> index d7fdafebda..9cc5a7f6ca 100644
>> --- a/block/backup.c
>> +++ b/block/backup.c
> 
> [...]
> 
>> @@ -202,7 +204,7 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
>>      cow_request_begin(&cow_request, job, start, end);
>>  
>>      while (start < end) {
>> -        if (!hbitmap_get(job->copy_bitmap, start)) {
>> +        if (!bdrv_dirty_bitmap_get(job->copy_bitmap, start)) {
>>              trace_backup_do_cow_skip(job, start);
>>              start += job->cluster_size;
>>              continue; /* already copied */
>> @@ -296,14 +298,16 @@ static void backup_abort(Job *job)
>>  static void backup_clean(Job *job)
>>  {
>>      BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
>> +    BlockDriverState *bs = blk_bs(s->target);
> 
> I’d prefer to call it target_bs, because “bs” is usually the source node.
> 

Sure;

> Which brings me to a question: Why is the copy bitmap assigned to the
> target in the first place?  Wouldn’t it make more sense on the source?
> 

*cough*;

the idea was that the target is *most likely* to be the temporary node
created for the purpose of this backup, even though backup does not
explicitly create the node.

So ... by creating it on the target, we avoid cluttering up the results
in block query; and otherwise it doesn't actually matter what node we
created it on, really.

I can move it over to the source, but the testing code has to get a
little smarter in order to find the "right" anonymous bitmap, which is
not impossible; but I thought this would actually be a convenience for
people.

>> +
>> +    if (s->copy_bitmap) {
>> +        bdrv_release_dirty_bitmap(bs, s->copy_bitmap);
>> +        s->copy_bitmap = NULL;
>> +    }
>> +
>>      assert(s->target);
>>      blk_unref(s->target);
>>      s->target = NULL;
>> -
>> -    if (s->copy_bitmap) {
>> -        hbitmap_free(s->copy_bitmap);
>> -        s->copy_bitmap = NULL;
>> -    }
>>  }
>>  
>>  void backup_do_checkpoint(BlockJob *job, Error **errp)
> 
> [...]
> 
>> @@ -387,59 +391,49 @@ static bool bdrv_is_unallocated_range(BlockDriverState *bs,
>>  
>>  static int coroutine_fn backup_loop(BackupBlockJob *job)
>>  {
>> -    int ret;
>>      bool error_is_read;
>>      int64_t offset;
>> -    HBitmapIter hbi;
>> +    BdrvDirtyBitmapIter *bdbi;
>>      BlockDriverState *bs = blk_bs(job->common.blk);
>> +    int ret = 0;
>>  
>> -    hbitmap_iter_init(&hbi, job->copy_bitmap, 0);
>> -    while ((offset = hbitmap_iter_next(&hbi)) != -1) {
>> +    bdbi = bdrv_dirty_iter_new(job->copy_bitmap);
>> +    while ((offset = bdrv_dirty_iter_next(bdbi)) != -1) {
>>          if (job->sync_mode == MIRROR_SYNC_MODE_TOP &&
>>              bdrv_is_unallocated_range(bs, offset, job->cluster_size))
>>          {
>> -            hbitmap_reset(job->copy_bitmap, offset, job->cluster_size);
>> +            bdrv_set_dirty_bitmap(job->copy_bitmap, offset, job->cluster_size);
> 
> Should be reset.
> 

Whoa, oops! I got through testing FULL but, clearly, not TOP. This also
doesn't trigger any other failures in our test suite, so I need to make
sure this is being covered.

Thank you.

>>              continue;
>>          }
>>  
>>          do {
>>              if (yield_and_check(job)) {
>> -                return 0;
>> +                goto out;
>>              }
>>              ret = backup_do_cow(job, offset,
>>                                  job->cluster_size, &error_is_read, false);
>>              if (ret < 0 && backup_error_action(job, error_is_read, -ret) ==
>>                             BLOCK_ERROR_ACTION_REPORT)
>>              {
>> -                return ret;
>> +                goto out;
>>              }
>>          } while (ret < 0);
>>      }
>>  
>> -    return 0;
>> + out:
>> +    bdrv_dirty_iter_free(bdbi);
>> +    return ret;
>>  }
>>  
>>  /* init copy_bitmap from sync_bitmap */
>>  static void backup_incremental_init_copy_bitmap(BackupBlockJob *job)
>>  {
>> -    uint64_t offset = 0;
>> -    uint64_t bytes = job->len;
>> -
>> -    while (bdrv_dirty_bitmap_next_dirty_area(job->sync_bitmap,
>> -                                             &offset, &bytes))
>> -    {
>> -        hbitmap_set(job->copy_bitmap, offset, bytes);
>> -
>> -        offset += bytes;
>> -        if (offset >= job->len) {
>> -            break;
>> -        }
>> -        bytes = job->len - offset;
>> -    }
>> +    bdrv_dirty_bitmap_merge_internal(job->copy_bitmap, job->sync_bitmap,
>> +                                     NULL, true);
> 
> How about asserting that this was successful?
> 

Sure.

>>  
>>      /* TODO job_progress_set_remaining() would make more sense */
>>      job_progress_update(&job->common.job,
>> -        job->len - hbitmap_count(job->copy_bitmap));
>> +                        job->len - bdrv_get_dirty_count(job->copy_bitmap));
>>  }
>>  
>>  static int coroutine_fn backup_run(Job *job, Error **errp)
> 
> [...]
> 
>> @@ -670,7 +668,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
>>   error:
>>      if (copy_bitmap) {
>>          assert(!job || !job->copy_bitmap);
>> -        hbitmap_free(copy_bitmap);
>> +        bdrv_release_dirty_bitmap(bs, copy_bitmap);
> 
> If you decide to keep the copy_bitmap on the target, s/bs/target/.

Ah, heck. Clearly I didn't test this error pathway either. I'll have to
add some more tests to make sure the error recovery works right.

> 
> Max
> 

On 05.07.19 18:52, John Snow wrote:
> 
> 
> On 7/4/19 1:29 PM, Max Reitz wrote:

[...]

>> Which brings me to a question: Why is the copy bitmap assigned to the
>> target in the first place?  Wouldn’t it make more sense on the source?
>>
> 
> *cough*;
> 
> the idea was that the target is *most likely* to be the temporary node
> created for the purpose of this backup, even though backup does not
> explicitly create the node.
> 
> So ... by creating it on the target, we avoid cluttering up the results
> in block query; and otherwise it doesn't actually matter what node we
> created it on, really.
> 
> I can move it over to the source, but the testing code has to get a
> little smarter in order to find the "right" anonymous bitmap, which is
> not impossible; but I thought this would actually be a convenience for
> people.

You didn’t really say whether you think it makes more sense on the
source or on the target.

This is an internal bitmap, so from my understanding, the user better
not sees it at all.  It should be easy for them to ignore it, regardless
of which node it is on.  (I don’t consider “clutter” a strong point,
because, well, most of our current query interfaces are nothing but
clutter.)  Sure, that makes it a bit difficult for testing, but testing
shouldn’t really be the focus.

So for me, it comes down to where it makes more sense.  And I think it
makes more sense on the source, because it flags source clusters to copy.

Max

On 7/8/19 8:02 AM, Max Reitz wrote:
> On 05.07.19 18:52, John Snow wrote:
>>
>>
>> On 7/4/19 1:29 PM, Max Reitz wrote:
> 
> [...]
> 
>>> Which brings me to a question: Why is the copy bitmap assigned to the
>>> target in the first place?  Wouldn’t it make more sense on the source?
>>>
>>
>> *cough*;
>>
>> the idea was that the target is *most likely* to be the temporary node
>> created for the purpose of this backup, even though backup does not
>> explicitly create the node.
>>
>> So ... by creating it on the target, we avoid cluttering up the results
>> in block query; and otherwise it doesn't actually matter what node we
>> created it on, really.
>>
>> I can move it over to the source, but the testing code has to get a
>> little smarter in order to find the "right" anonymous bitmap, which is
>> not impossible; but I thought this would actually be a convenience for
>> people.
> 
> You didn’t really say whether you think it makes more sense on the
> source or on the target.
> 

Yeah, a bitmap that isn't recording writes seems to make about equal
sense on either to me; I chose the destination because it was more
likely to be temporary (in the drive-backup case) and I considered this
a temporary bitmap for use by the job.

Organizationally I felt that made sense. I realize it's also not
strictly true for the blockdev-backup case, but it also doesn't matter
terribly.

Semantically, it's a toss-up. Another coder could conceivably one day
decided to re-enable this bitmap and then it would make more sense on
the source. (That coder would be wrong to do that.)

> This is an internal bitmap, so from my understanding, the user better
> not sees it at all.  It should be easy for them to ignore it, regardless
> of which node it is on.  (I don’t consider “clutter” a strong point,
> because, well, most of our current query interfaces are nothing but
> clutter.)  Sure, that makes it a bit difficult for testing, but testing
> shouldn’t really be the focus.

We don't have an API to truly hide bitmaps. Vladimir wanted to add one
but I felt it complicated too much. I still don't really want one, I
don't think it's worth the time.

At the moment, anonymous bitmaps can be seen but because they have no
name, cannot be addressed. They have an implicit permission protection
that way.

(The complication is that Vladimir wanted to hide *named* bitmaps, which
has implications for the namespace that I didn't want to deal with. We
can hide anonymous bitmaps, but we can't do so automatically because
mirror and backup already use anonymous bitmaps that aren't hidden. I
hope people don't rely on seeing them in the query output, but they might.)

> 
> So for me, it comes down to where it makes more sense.  And I think it
> makes more sense on the source, because it flags source clusters to copy.
> 
> Max
> 

If you insist.

On 08.07.19 20:32, John Snow wrote:
> 
> 
> On 7/8/19 8:02 AM, Max Reitz wrote:
>> On 05.07.19 18:52, John Snow wrote:
>>>
>>>
>>> On 7/4/19 1:29 PM, Max Reitz wrote:
>>
>> [...]
>>
>>>> Which brings me to a question: Why is the copy bitmap assigned to the
>>>> target in the first place?  Wouldn’t it make more sense on the source?
>>>>
>>>
>>> *cough*;
>>>
>>> the idea was that the target is *most likely* to be the temporary node
>>> created for the purpose of this backup, even though backup does not
>>> explicitly create the node.
>>>
>>> So ... by creating it on the target, we avoid cluttering up the results
>>> in block query; and otherwise it doesn't actually matter what node we
>>> created it on, really.
>>>
>>> I can move it over to the source, but the testing code has to get a
>>> little smarter in order to find the "right" anonymous bitmap, which is
>>> not impossible; but I thought this would actually be a convenience for
>>> people.
>>
>> You didn’t really say whether you think it makes more sense on the
>> source or on the target.
>>
> 
> Yeah, a bitmap that isn't recording writes seems to make about equal
> sense on either to me; I chose the destination because it was more
> likely to be temporary (in the drive-backup case) and I considered this
> a temporary bitmap for use by the job.
> 
> Organizationally I felt that made sense. I realize it's also not
> strictly true for the blockdev-backup case, but it also doesn't matter
> terribly.
> 
> Semantically, it's a toss-up. Another coder could conceivably one day
> decided to re-enable this bitmap and then it would make more sense on
> the source. (That coder would be wrong to do that.)

Hm.  If we had a filter node like we do for mirror, it should be there,
clearly.

...is what I wanted to say.  But then I looked it up and found out that
mirror actually still puts its bitmap on the source node.

Sure, one of the differences between backup and mirror is that backup’s
bitmap does not record writes and thus it functionally doesn’t matter
where it’s placed.  But why make mirror and backup behave more
differently than necessary?  They should be a single block job anyway.

>> So for me, it comes down to where it makes more sense.  And I think it
>> makes more sense on the source, because it flags source clusters to copy.
>>
>> Max
>>
> 
> If you insist.

I guess I kind of do, yes.

Max