Accept bitmaps and sync policies for the other backup modes.
This allows us to do things like create a bitmap synced to a full backup
without a transaction, or start a resumable backup process.

Some combinations don't make sense, though:

- NEVER policy combined with any non-BITMAP mode doesn't do anything,
  because the bitmap isn't used for input or output.
  It's harmless, but is almost certainly never what the user wanted.

- sync=NONE is more questionable. It can't use on-success because this
  job never completes with success anyway, and the resulting artifact
  of 'always' is suspect: because we start with a full bitmap and only
  copy out segments that get written to, the final output bitmap will
  always be ... a fully set bitmap.

  Maybe there's contexts in which bitmaps make sense for sync=none,
  but not without more severe changes to the current job, and omitting
  it here doesn't prevent us from adding it later.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/backup.c       |  8 +-------
 blockdev.c           | 22 ++++++++++++++++++++++
 qapi/block-core.json |  6 ++++--
 3 files changed, 27 insertions(+), 9 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 38c4a688c6..b94ed595ba 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -602,7 +602,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
         return NULL;
     }
 
-    if (sync_mode == MIRROR_SYNC_MODE_BITMAP) {
+    if (sync_bitmap) {
         /* If we need to write to this bitmap, check that we can: */
         if (bitmap_mode != BITMAP_SYNC_MODE_NEVER &&
             bdrv_dirty_bitmap_check(sync_bitmap, BDRV_BITMAP_DEFAULT, errp)) {
@@ -613,12 +613,6 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
         if (bdrv_dirty_bitmap_create_successor(bs, sync_bitmap, errp) < 0) {
             return NULL;
         }
-    } else if (sync_bitmap) {
-        error_setg(errp,
-                   "a bitmap was given to backup_job_create, "
-                   "but it received an incompatible sync_mode (%s)",
-                   MirrorSyncMode_str(sync_mode));
-        return NULL;
     }
 
     len = bdrv_getlength(bs);
diff --git a/blockdev.c b/blockdev.c
index f0b7da53b0..bc152f8e0d 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3502,6 +3502,28 @@ static BlockJob *do_backup_common(BackupCommon *backup,
         if (bdrv_dirty_bitmap_check(bmap, BDRV_BITMAP_ALLOW_RO, errp)) {
             return NULL;
         }
+
+        /* This does not produce a useful bitmap artifact: */
+        if (backup->sync == MIRROR_SYNC_MODE_NONE) {
+            error_setg(errp, "sync mode '%s' does not produce meaningful bitmap"
+                       " outputs", MirrorSyncMode_str(backup->sync));
+            return NULL;
+        }
+
+        /* If the bitmap isn't used for input or output, this is useless: */
+        if (backup->bitmap_mode == BITMAP_SYNC_MODE_NEVER &&
+            backup->sync != MIRROR_SYNC_MODE_BITMAP) {
+            error_setg(errp, "Bitmap sync mode '%s' has no meaningful effect"
+                       " when combined with sync mode '%s'",
+                       BitmapSyncMode_str(backup->bitmap_mode),
+                       MirrorSyncMode_str(backup->sync));
+            return NULL;
+        }
+    }
+
+    if (!backup->has_bitmap && backup->has_bitmap_mode) {
+        error_setg(errp, "Cannot specify Bitmap sync mode without a bitmap");
+        return NULL;
     }
 
     if (!backup->auto_finalize) {
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 5a578806c5..099e4f37b2 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1352,13 +1352,15 @@
 # @speed: the maximum speed, in bytes per second. The default is 0,
 #         for unlimited.
 #
-# @bitmap: the name of a dirty bitmap if sync is "bitmap" or "incremental".
+# @bitmap: The name of a dirty bitmap to use.
 #          Must be present if sync is "bitmap" or "incremental".
+#          Can be present if sync is "full" or "top".
 #          Must not be present otherwise.
 #          (Since 2.4 (drive-backup), 3.1 (blockdev-backup))
 #
 # @bitmap-mode: Specifies the type of data the bitmap should contain after
-#               the operation concludes. Must be present if sync is "bitmap".
+#               the operation concludes.
+#               Must be present if a bitmap was provided,
 #               Must NOT be present otherwise. (Since 4.2)
 #
 # @compress: true to compress data, if the target format supports it.
-- 
2.21.0

On 10.07.19 03:05, John Snow wrote:
> Accept bitmaps and sync policies for the other backup modes.
> This allows us to do things like create a bitmap synced to a full backup
> without a transaction, or start a resumable backup process.
> 
> Some combinations don't make sense, though:
> 
> - NEVER policy combined with any non-BITMAP mode doesn't do anything,
>   because the bitmap isn't used for input or output.
>   It's harmless, but is almost certainly never what the user wanted.
> 
> - sync=NONE is more questionable. It can't use on-success because this
>   job never completes with success anyway, and the resulting artifact
>   of 'always' is suspect: because we start with a full bitmap and only
>   copy out segments that get written to, the final output bitmap will
>   always be ... a fully set bitmap.
> 
>   Maybe there's contexts in which bitmaps make sense for sync=none,
>   but not without more severe changes to the current job, and omitting
>   it here doesn't prevent us from adding it later.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/backup.c       |  8 +-------
>  blockdev.c           | 22 ++++++++++++++++++++++
>  qapi/block-core.json |  6 ++++--
>  3 files changed, 27 insertions(+), 9 deletions(-)

[...]

> diff --git a/blockdev.c b/blockdev.c
> index f0b7da53b0..bc152f8e0d 100644
> --- a/blockdev.c
> +++ b/blockdev.c

[...]

> +    if (!backup->has_bitmap && backup->has_bitmap_mode) {
> +        error_setg(errp, "Cannot specify Bitmap sync mode without a bitmap");

Any reason for capitalizing the first “Bitmap”?

With a reason or it fixed:

Reviewed-by: Max Reitz <mreitz@redhat.com>

On 7/10/19 12:48 PM, Max Reitz wrote:
> On 10.07.19 03:05, John Snow wrote:
>> Accept bitmaps and sync policies for the other backup modes.
>> This allows us to do things like create a bitmap synced to a full backup
>> without a transaction, or start a resumable backup process.
>>
>> Some combinations don't make sense, though:
>>
>> - NEVER policy combined with any non-BITMAP mode doesn't do anything,
>>   because the bitmap isn't used for input or output.
>>   It's harmless, but is almost certainly never what the user wanted.
>>
>> - sync=NONE is more questionable. It can't use on-success because this
>>   job never completes with success anyway, and the resulting artifact
>>   of 'always' is suspect: because we start with a full bitmap and only
>>   copy out segments that get written to, the final output bitmap will
>>   always be ... a fully set bitmap.
>>
>>   Maybe there's contexts in which bitmaps make sense for sync=none,
>>   but not without more severe changes to the current job, and omitting
>>   it here doesn't prevent us from adding it later.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  block/backup.c       |  8 +-------
>>  blockdev.c           | 22 ++++++++++++++++++++++
>>  qapi/block-core.json |  6 ++++--
>>  3 files changed, 27 insertions(+), 9 deletions(-)
> 
> [...]
> 
>> diff --git a/blockdev.c b/blockdev.c
>> index f0b7da53b0..bc152f8e0d 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
> 
> [...]
> 
>> +    if (!backup->has_bitmap && backup->has_bitmap_mode) {
>> +        error_setg(errp, "Cannot specify Bitmap sync mode without a bitmap");
> 
> Any reason for capitalizing the first “Bitmap”?
> 
> With a reason or it fixed:
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> 

Hanging around Germans too much?

Actually, I can explain why: because a "bitmap" is a generic term, but
whenever I capitalize it as "Bitmap" I am referring to a Block Dirty
Bitmap which is a specific sort of thing. I do this unconsciously.

But in that case, I should actually be consistent in the interface (and
docstrings and docs and error strings) and always call it that specific
thing, which I don't.

"bitmap" is probably more correct for now, but I ought to go through all
the interface and make it consistent in some way or another.


(Actually: I'd like to see if I can't rename the "BdrvDirtyBitmap"
object to something more generic and shorter so I can rename many of the
functions we have something shorter.

Because the structure is "BdrvDirtyBitmap", there's some confusion when
we name functions like bdrv_dirty_bitmap_{verb} because it's not clear
if this is a bdrv function that does something with dirty bitmaps, or if
it's a "BdrvDirtyBitmap" function that does something with that object.

I guess that seems like a subtle point, but it's why the naming
conventions in dirty-bitmap.c are all over the place. I think at one
point, the idea was that:

bdrv_{verb}_dirty_bitmap was an action applied to a BDS that happened to
do something with dirty bitmaps. bdrv_dirty_bitmap_{verb} was an action
that applied to a "BdrvDirtyBitmap". Crystal clear and not confusing at
all, right?

It'd be nice to have functions that operate on a node be named
bdrv_dbitmap_foo() and functions that operate on the bitmap structure
itself named just dbitmap_bar().

Would it be okay if I named them such a thing, I wonder?

we have "bitmap" and "hbitmap" already, I could do something like
"dbitmap" for "dirty bitmap" or some such. Kind of an arbitrary change I
admit, but I'm itching to do a big spring cleaning in dirty-bitmap.c
right after this series is done.)

On 10.07.19 20:32, John Snow wrote:
> 
> 
> On 7/10/19 12:48 PM, Max Reitz wrote:
>> On 10.07.19 03:05, John Snow wrote:
>>> Accept bitmaps and sync policies for the other backup modes.
>>> This allows us to do things like create a bitmap synced to a full backup
>>> without a transaction, or start a resumable backup process.
>>>
>>> Some combinations don't make sense, though:
>>>
>>> - NEVER policy combined with any non-BITMAP mode doesn't do anything,
>>>   because the bitmap isn't used for input or output.
>>>   It's harmless, but is almost certainly never what the user wanted.
>>>
>>> - sync=NONE is more questionable. It can't use on-success because this
>>>   job never completes with success anyway, and the resulting artifact
>>>   of 'always' is suspect: because we start with a full bitmap and only
>>>   copy out segments that get written to, the final output bitmap will
>>>   always be ... a fully set bitmap.
>>>
>>>   Maybe there's contexts in which bitmaps make sense for sync=none,
>>>   but not without more severe changes to the current job, and omitting
>>>   it here doesn't prevent us from adding it later.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>  block/backup.c       |  8 +-------
>>>  blockdev.c           | 22 ++++++++++++++++++++++
>>>  qapi/block-core.json |  6 ++++--
>>>  3 files changed, 27 insertions(+), 9 deletions(-)
>>
>> [...]
>>
>>> diff --git a/blockdev.c b/blockdev.c
>>> index f0b7da53b0..bc152f8e0d 100644
>>> --- a/blockdev.c
>>> +++ b/blockdev.c
>>
>> [...]
>>
>>> +    if (!backup->has_bitmap && backup->has_bitmap_mode) {
>>> +        error_setg(errp, "Cannot specify Bitmap sync mode without a bitmap");
>>
>> Any reason for capitalizing the first “Bitmap”?
>>
>> With a reason or it fixed:
>>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
>>
> 
> Hanging around Germans too much?

You should know then that the korrekt way to write it would be:

„Specifying Binarydigitmapsynchronizationmode without a Binarydigitmap
is absolutely verboten!“

> Actually, I can explain why: because a "bitmap" is a generic term, but
> whenever I capitalize it as "Bitmap" I am referring to a Block Dirty
> Bitmap which is a specific sort of thing. I do this unconsciously.
> 
> But in that case, I should actually be consistent in the interface (and
> docstrings and docs and error strings) and always call it that specific
> thing, which I don't.
> 
> "bitmap" is probably more correct for now, but I ought to go through all
> the interface and make it consistent in some way or another.
> 
> 
> (Actually: I'd like to see if I can't rename the "BdrvDirtyBitmap"
> object to something more generic and shorter so I can rename many of the
> functions we have something shorter.

Well, BDB is free.  That path has worked fine for BlockDriverStates.

Or what I said on IRC, but you know.

> Because the structure is "BdrvDirtyBitmap", there's some confusion when
> we name functions like bdrv_dirty_bitmap_{verb} because it's not clear
> if this is a bdrv function that does something with dirty bitmaps, or if
> it's a "BdrvDirtyBitmap" function that does something with that object.
> 
> I guess that seems like a subtle point, but it's why the naming
> conventions in dirty-bitmap.c are all over the place. I think at one
> point, the idea was that:
> 
> bdrv_{verb}_dirty_bitmap was an action applied to a BDS that happened to
> do something with dirty bitmaps. bdrv_dirty_bitmap_{verb} was an action
> that applied to a "BdrvDirtyBitmap". Crystal clear and not confusing at
> all, right?

I just thought people named their functions whatever they felt like at
the time.

> It'd be nice to have functions that operate on a node be named
> bdrv_dbitmap_foo() and functions that operate on the bitmap structure
> itself named just dbitmap_bar().
> 
> Would it be okay if I named them such a thing, I wonder?
> 
> we have "bitmap" and "hbitmap" already, I could do something like
> "dbitmap" for "dirty bitmap" or some such. Kind of an arbitrary change I
> admit, but I'm itching to do a big spring cleaning in dirty-bitmap.c
> right after this series is done.)

HBitmaps are generally used to track dirty areas, so I’d find this a
misnomer.  BDBitmap would be OK.  The “block” part should be in there
somewhere.

Max