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