| Message ID | 20190716000117.25219-11-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | bitmaps: allow bitmaps to be used with full and top | expand |
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 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.)
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
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.
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(-)