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 e28fd23f6a..47628aca24 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -686,7 +686,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)) {
@@ -697,12 +697,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 3c76c85cb5..29c6c6044a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3565,6 +3565,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 16.07.19 02:01, 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(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
(I’ve seen Markus’s concern, but I think management applications can
just see whether specifying sync={full,top} + bitmap works if they want
to use it.)
John Snow <jsnow@redhat.com> writes: > 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> > --- [...] > 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. Do you expect management applications will want to know about the presence of this patch?
On 7/16/19 1:18 AM, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: > >> 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> >> --- > [...] >> 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. > > Do you expect management applications will want to know about the > presence of this patch? > It's all going to be queued up for 4.2, so management can gate on the presence of the "bitmap-mode" field. The text being replaced here is intermediate only. Bitmaps can't be used with traditional backup modes without the bitmap-mode parameter, so I believe this is OK. --js
© 2016 - 2026 Red Hat, Inc.