| Message ID | 20190620010356.19164-3-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | bitmaps: introduce 'bitmap' sync mode | expand |
On 20.06.19 03:03, John Snow wrote: > We don't need or want a new sync mode for simple differences in > semantics. Create a new mode simply named "BITMAP" that is designed to > make use of the new Bitmap Sync Mode field. > > Because the only bitmap mode is 'conditional', this adds no new > functionality to the backup job (yet). The old incremental backup mode > is maintained as a syntactic sugar for sync=bitmap, mode=conditional. > > Add all of the plumbing necessary to support this new instruction. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > qapi/block-core.json | 30 ++++++++++++++++++++++-------- > include/block/block_int.h | 6 +++++- > block/backup.c | 35 ++++++++++++++++++++++++++++------- > block/mirror.c | 6 ++++-- > block/replication.c | 2 +- > blockdev.c | 8 ++++++-- > 6 files changed, 66 insertions(+), 21 deletions(-) > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index caf28a71a0..6d05ad8f47 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -1127,12 +1127,15 @@ > # > # @none: only copy data written from now on > # > -# @incremental: only copy data described by the dirty bitmap. Since: 2.4 > +# @incremental: only copy data described by the dirty bitmap. (since: 2.4) Why not deprecate this in the process and note that this is equal to sync=bitmap, bitmap-mode=conditional? (I don’t think there is a rule that forces us to actually remove deprecated stuff after two releases if it doesn’t hurt to keep it.) > +# > +# @bitmap: only copy data described by the dirty bitmap. (since: 4.1) > +# Behavior on completion is determined by the BitmapSyncMode. > # > # Since: 1.3 > ## > { 'enum': 'MirrorSyncMode', > - 'data': ['top', 'full', 'none', 'incremental'] } > + 'data': ['top', 'full', 'none', 'incremental', 'bitmap'] } > > ## > # @BitmapSyncMode: > @@ -1352,10 +1355,14 @@ > # > # @speed: the maximum speed, in bytes per second > # > -# @bitmap: the name of dirty bitmap if sync is "incremental". > -# Must be present if sync is "incremental", must NOT be present > +# @bitmap: the name of dirty bitmap if sync is "bitmap". > +# Must be present if sync is "bitmap", must NOT be present > # otherwise. (Since 2.4) Er, well, now this is too fast of a deprecation. :-) It must still also be present if sync is “incremental”. > # > +# @bitmap-mode: Specifies the type of data the bitmap should contain after > +# the operation concludes. Must be present if sync is "bitmap". > +# Must NOT be present otherwise. (Since 4.1) Do we have any rule that qemu must enforce “must not”s? :-) (No, I don’t think so. I think it’s very reasonable that you accept bitmap-mode=conditional for sync=incremental.) > # @compress: true to compress data, if the target format supports it. > # (default: false) (since 2.8) > # > @@ -1390,7 +1397,8 @@ > 'data': { '*job-id': 'str', 'device': 'str', 'target': 'str', > '*format': 'str', 'sync': 'MirrorSyncMode', > '*mode': 'NewImageMode', '*speed': 'int', > - '*bitmap': 'str', '*compress': 'bool', > + '*bitmap': 'str', '*bitmap-mode': 'BitmapSyncMode', > + '*compress': 'bool', > '*on-source-error': 'BlockdevOnError', > '*on-target-error': 'BlockdevOnError', > '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } } > @@ -1412,10 +1420,14 @@ > # @speed: the maximum speed, in bytes per second. The default is 0, > # for unlimited. > # > -# @bitmap: the name of dirty bitmap if sync is "incremental". > -# Must be present if sync is "incremental", must NOT be present > +# @bitmap: the name of dirty bitmap if sync is "bitmap". > +# Must be present if sync is "bitmap", must NOT be present > # otherwise. (Since 3.1) Same as above. > +# @bitmap-mode: Specifies the type of data the bitmap should contain after > +# the operation concludes. Must be present if sync is "bitmap". > +# Must NOT be present otherwise. (Since 4.1) > +# > # @compress: true to compress data, if the target format supports it. > # (default: false) (since 2.8) > # > @@ -1449,7 +1461,9 @@ > { 'struct': 'BlockdevBackup', > 'data': { '*job-id': 'str', 'device': 'str', 'target': 'str', > 'sync': 'MirrorSyncMode', '*speed': 'int', > - '*bitmap': 'str', '*compress': 'bool', > + '*bitmap': 'str', > + '*bitmap-mode': 'BitmapSyncMode', > + '*compress': 'bool', > '*on-source-error': 'BlockdevOnError', > '*on-target-error': 'BlockdevOnError', > '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } } > diff --git a/include/block/block_int.h b/include/block/block_int.h > index d6415b53c1..89370c1b9b 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -1132,7 +1132,9 @@ void mirror_start(const char *job_id, BlockDriverState *bs, > * @target: Block device to write to. > * @speed: The maximum speed, in bytes per second, or 0 for unlimited. > * @sync_mode: What parts of the disk image should be copied to the destination. > - * @sync_bitmap: The dirty bitmap if sync_mode is MIRROR_SYNC_MODE_INCREMENTAL. > + * @sync_bitmap: The dirty bitmap if sync_mode is 'bitmap' or 'incremental' > + * @has_bitmap_mode: true if @bitmap_sync carries a meaningful value. Hmm... If you moved the conversion of incremental/- => bitmap/conditional into blockdev.c, you could get rid of this parameter because it would be equal to (sync_bitmap != NULL). (It itches me to get rid of this parameter because there is no other has* parameter for this function yet.) > + * @bitmap_mode: The bitmap synchronization policy to use. > * @on_source_error: The action to take upon error reading from the source. > * @on_target_error: The action to take upon error writing to the target. > * @creation_flags: Flags that control the behavior of the Job lifetime. > @@ -1148,6 +1150,8 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, > BlockDriverState *target, int64_t speed, > MirrorSyncMode sync_mode, > BdrvDirtyBitmap *sync_bitmap, > + bool has_bitmap_mode, > + BitmapSyncMode bitmap_mode, > bool compress, > BlockdevOnError on_source_error, > BlockdevOnError on_target_error, > diff --git a/block/backup.c b/block/backup.c > index 715e1d3be8..c4f83d4ef7 100644 > --- a/block/backup.c > +++ b/block/backup.c [...] > @@ -584,9 +586,28 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, > } > > if (sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) { > + if (has_bitmap_mode && > + bitmap_mode != BITMAP_SYNC_MODE_CONDITIONAL) { > + error_setg(errp, "Bitmap sync mode must be 'conditional' " > + "when using sync mode '%s'", > + MirrorSyncMode_str(sync_mode)); > + return NULL; > + } > + has_bitmap_mode = true; > + bitmap_mode = BITMAP_SYNC_MODE_CONDITIONAL; > + effective_mode = MIRROR_SYNC_MODE_BITMAP; > + } > + I also just don’t quite feel like this is the correct place to put this. It’s a deprecated interface, so it should be translated in the interface code, i.e. in blockdev.c. (Sure, this gives you a central place for the translation, but you can just as well add a function to the same effect to blockdev.c.) Max
On 6/20/19 11:00 AM, Max Reitz wrote: > On 20.06.19 03:03, John Snow wrote: >> We don't need or want a new sync mode for simple differences in >> semantics. Create a new mode simply named "BITMAP" that is designed to >> make use of the new Bitmap Sync Mode field. >> >> Because the only bitmap mode is 'conditional', this adds no new >> functionality to the backup job (yet). The old incremental backup mode >> is maintained as a syntactic sugar for sync=bitmap, mode=conditional. >> >> Add all of the plumbing necessary to support this new instruction. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> qapi/block-core.json | 30 ++++++++++++++++++++++-------- >> include/block/block_int.h | 6 +++++- >> block/backup.c | 35 ++++++++++++++++++++++++++++------- >> block/mirror.c | 6 ++++-- >> block/replication.c | 2 +- >> blockdev.c | 8 ++++++-- >> 6 files changed, 66 insertions(+), 21 deletions(-) >> >> diff --git a/qapi/block-core.json b/qapi/block-core.json >> index caf28a71a0..6d05ad8f47 100644 >> --- a/qapi/block-core.json >> +++ b/qapi/block-core.json >> @@ -1127,12 +1127,15 @@ >> # >> # @none: only copy data written from now on >> # >> -# @incremental: only copy data described by the dirty bitmap. Since: 2.4 >> +# @incremental: only copy data described by the dirty bitmap. (since: 2.4) > > Why not deprecate this in the process and note that this is equal to > sync=bitmap, bitmap-mode=conditional? > > (I don’t think there is a rule that forces us to actually remove > deprecated stuff after two releases if it doesn’t hurt to keep it.) > Mostly I thought it would be fine to keep as sugar. In your replies so far I gather that "incremental" and "differential" don't mean specific backup paradigms to you, so maybe these seem like worthless words. It was my general understanding that in terms of backup paradigms/methodologies that "incremental" and "differential" mean very specific things. Incremental: Each backup contains only the delta from the last incremental backup. Differential: Each backup contains the delta from the last FULL backup. You can search "incremental vs differential backup" on your search engine of choice and find many relevant results. I took a Networking/IT vocational degree in 2007 and these terms were taught in textbooks then. So I will resist quite strongly changing them, and for this reason, felt that it was strictly a good thing to keep incremental as sugar, because I thought that people would know what it meant. (More than "conditional", anyway, which is jargon I made up.) >> +# >> +# @bitmap: only copy data described by the dirty bitmap. (since: 4.1) >> +# Behavior on completion is determined by the BitmapSyncMode. >> # >> # Since: 1.3 >> ## >> { 'enum': 'MirrorSyncMode', >> - 'data': ['top', 'full', 'none', 'incremental'] } >> + 'data': ['top', 'full', 'none', 'incremental', 'bitmap'] } >> >> ## >> # @BitmapSyncMode: >> @@ -1352,10 +1355,14 @@ >> # >> # @speed: the maximum speed, in bytes per second >> # >> -# @bitmap: the name of dirty bitmap if sync is "incremental". >> -# Must be present if sync is "incremental", must NOT be present >> +# @bitmap: the name of dirty bitmap if sync is "bitmap". >> +# Must be present if sync is "bitmap", must NOT be present >> # otherwise. (Since 2.4) > > Er, well, now this is too fast of a deprecation. :-) It must still also > be present if sync is “incremental”. > OK; I will try to phrase it better. This is reflecting too much the implementation -- I think I was trying to communicate that incremental was just sugar for "bitmap", so I was trusting that was understood here. ...But, depending on the order in which you read the docs, this could be confusing, so I guess I will change that. >> # >> +# @bitmap-mode: Specifies the type of data the bitmap should contain after >> +# the operation concludes. Must be present if sync is "bitmap". >> +# Must NOT be present otherwise. (Since 4.1) > > Do we have any rule that qemu must enforce “must not”s? :-) > > (No, I don’t think so. I think it’s very reasonable that you accept > bitmap-mode=conditional for sync=incremental.) > Right, I left this a secret wiggle room. If you specify the correct bitmap sync mode for the incremental sugar, it will actually let it slide. If you specify the wrong one, it will error out. However, I think this is perfectly correct advice from the API: Please use this mode with sync=bitmap and do not use it otherwise. Would you like me to change it to be more technically correct and document the little affordance I made? >> # @compress: true to compress data, if the target format supports it. >> # (default: false) (since 2.8) >> # >> @@ -1390,7 +1397,8 @@ >> 'data': { '*job-id': 'str', 'device': 'str', 'target': 'str', >> '*format': 'str', 'sync': 'MirrorSyncMode', >> '*mode': 'NewImageMode', '*speed': 'int', >> - '*bitmap': 'str', '*compress': 'bool', >> + '*bitmap': 'str', '*bitmap-mode': 'BitmapSyncMode', >> + '*compress': 'bool', >> '*on-source-error': 'BlockdevOnError', >> '*on-target-error': 'BlockdevOnError', >> '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } } >> @@ -1412,10 +1420,14 @@ >> # @speed: the maximum speed, in bytes per second. The default is 0, >> # for unlimited. >> # >> -# @bitmap: the name of dirty bitmap if sync is "incremental". >> -# Must be present if sync is "incremental", must NOT be present >> +# @bitmap: the name of dirty bitmap if sync is "bitmap". >> +# Must be present if sync is "bitmap", must NOT be present >> # otherwise. (Since 3.1) > > Same as above. > OK >> +# @bitmap-mode: Specifies the type of data the bitmap should contain after >> +# the operation concludes. Must be present if sync is "bitmap". >> +# Must NOT be present otherwise. (Since 4.1) >> +# >> # @compress: true to compress data, if the target format supports it. >> # (default: false) (since 2.8) >> # >> @@ -1449,7 +1461,9 @@ >> { 'struct': 'BlockdevBackup', >> 'data': { '*job-id': 'str', 'device': 'str', 'target': 'str', >> 'sync': 'MirrorSyncMode', '*speed': 'int', >> - '*bitmap': 'str', '*compress': 'bool', >> + '*bitmap': 'str', >> + '*bitmap-mode': 'BitmapSyncMode', >> + '*compress': 'bool', >> '*on-source-error': 'BlockdevOnError', >> '*on-target-error': 'BlockdevOnError', >> '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } } >> diff --git a/include/block/block_int.h b/include/block/block_int.h >> index d6415b53c1..89370c1b9b 100644 >> --- a/include/block/block_int.h >> +++ b/include/block/block_int.h >> @@ -1132,7 +1132,9 @@ void mirror_start(const char *job_id, BlockDriverState *bs, >> * @target: Block device to write to. >> * @speed: The maximum speed, in bytes per second, or 0 for unlimited. >> * @sync_mode: What parts of the disk image should be copied to the destination. >> - * @sync_bitmap: The dirty bitmap if sync_mode is MIRROR_SYNC_MODE_INCREMENTAL. >> + * @sync_bitmap: The dirty bitmap if sync_mode is 'bitmap' or 'incremental' >> + * @has_bitmap_mode: true if @bitmap_sync carries a meaningful value. > > Hmm... If you moved the conversion of incremental/- => > bitmap/conditional into blockdev.c, you could get rid of this parameter > because it would be equal to (sync_bitmap != NULL). > > (It itches me to get rid of this parameter because there is no other > has* parameter for this function yet.) > Yeah, it annoyed me too, and I believe later you do correctly guess why I did it -- it's so that the sugar conversion occurs all in one place where the logic was easiest to condense. I ran into the issue that there's no way to define a QAPI enum that has a "default"/"unset" state without also allowing that value to be entered by the user explicitly; so there was no way to pass along an "unset enum" down this far. So... (thought continued below) >> + * @bitmap_mode: The bitmap synchronization policy to use. >> * @on_source_error: The action to take upon error reading from the source. >> * @on_target_error: The action to take upon error writing to the target. >> * @creation_flags: Flags that control the behavior of the Job lifetime. >> @@ -1148,6 +1150,8 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, >> BlockDriverState *target, int64_t speed, >> MirrorSyncMode sync_mode, >> BdrvDirtyBitmap *sync_bitmap, >> + bool has_bitmap_mode, >> + BitmapSyncMode bitmap_mode, >> bool compress, >> BlockdevOnError on_source_error, >> BlockdevOnError on_target_error, >> diff --git a/block/backup.c b/block/backup.c >> index 715e1d3be8..c4f83d4ef7 100644 >> --- a/block/backup.c >> +++ b/block/backup.c > > [...] > >> @@ -584,9 +586,28 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, >> } >> >> if (sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) { >> + if (has_bitmap_mode && >> + bitmap_mode != BITMAP_SYNC_MODE_CONDITIONAL) { >> + error_setg(errp, "Bitmap sync mode must be 'conditional' " >> + "when using sync mode '%s'", >> + MirrorSyncMode_str(sync_mode)); >> + return NULL; >> + } >> + has_bitmap_mode = true; >> + bitmap_mode = BITMAP_SYNC_MODE_CONDITIONAL; >> + effective_mode = MIRROR_SYNC_MODE_BITMAP; >> + } >> + > > I also just don’t quite feel like this is the correct place to put this. > It’s a deprecated interface, so it should be translated in the > interface code, i.e. in blockdev.c. > > (Sure, this gives you a central place for the translation, but you can > just as well add a function to the same effect to blockdev.c.) > > Max > ... I can toy around with your idea of making a helper that can be called in blockdev and see if I like it. Thank you for taking a look!
On 20.06.19 18:01, John Snow wrote: > > > On 6/20/19 11:00 AM, Max Reitz wrote: >> On 20.06.19 03:03, John Snow wrote: >>> We don't need or want a new sync mode for simple differences in >>> semantics. Create a new mode simply named "BITMAP" that is designed to >>> make use of the new Bitmap Sync Mode field. >>> >>> Because the only bitmap mode is 'conditional', this adds no new >>> functionality to the backup job (yet). The old incremental backup mode >>> is maintained as a syntactic sugar for sync=bitmap, mode=conditional. >>> >>> Add all of the plumbing necessary to support this new instruction. >>> >>> Signed-off-by: John Snow <jsnow@redhat.com> >>> --- >>> qapi/block-core.json | 30 ++++++++++++++++++++++-------- >>> include/block/block_int.h | 6 +++++- >>> block/backup.c | 35 ++++++++++++++++++++++++++++------- >>> block/mirror.c | 6 ++++-- >>> block/replication.c | 2 +- >>> blockdev.c | 8 ++++++-- >>> 6 files changed, 66 insertions(+), 21 deletions(-) >>> >>> diff --git a/qapi/block-core.json b/qapi/block-core.json >>> index caf28a71a0..6d05ad8f47 100644 >>> --- a/qapi/block-core.json >>> +++ b/qapi/block-core.json >>> @@ -1127,12 +1127,15 @@ >>> # >>> # @none: only copy data written from now on >>> # >>> -# @incremental: only copy data described by the dirty bitmap. Since: 2.4 >>> +# @incremental: only copy data described by the dirty bitmap. (since: 2.4) >> >> Why not deprecate this in the process and note that this is equal to >> sync=bitmap, bitmap-mode=conditional? >> >> (I don’t think there is a rule that forces us to actually remove >> deprecated stuff after two releases if it doesn’t hurt to keep it.) >> > > Mostly I thought it would be fine to keep as sugar. In your replies so > far I gather that "incremental" and "differential" don't mean specific > backup paradigms to you, so maybe these seem like worthless words. > > It was my general understanding that in terms of backup > paradigms/methodologies that "incremental" and "differential" mean very > specific things. > > Incremental: Each backup contains only the delta from the last > incremental backup. > Differential: Each backup contains the delta from the last FULL backup. > > You can search "incremental vs differential backup" on your search > engine of choice and find many relevant results. I took a Networking/IT > vocational degree in 2007 and these terms were taught in textbooks then. > > So I will resist quite strongly changing them, and for this reason, felt > that it was strictly a good thing to keep incremental as sugar, because > I thought that people would know what it meant. :C OK. I’m happy as long as it’s all explained somewhere (i.e. bitmaps.rst). Personally, I’d also like a pointer to that documentation here. (Sure, people should just look there if they don’t understand something about bitmaps anyway, but I can’t see it hurting to just put a pointer here anyway.) > (More than "conditional", anyway, which is jargon I made up.) But you make it up in this series, which is great for me, because that means I get the definition (from the cover letter) without having to look it up. O:-) [...] >>> # >>> +# @bitmap-mode: Specifies the type of data the bitmap should contain after >>> +# the operation concludes. Must be present if sync is "bitmap". >>> +# Must NOT be present otherwise. (Since 4.1) >> >> Do we have any rule that qemu must enforce “must not”s? :-) >> >> (No, I don’t think so. I think it’s very reasonable that you accept >> bitmap-mode=conditional for sync=incremental.) >> > > Right, I left this a secret wiggle room. If you specify the correct > bitmap sync mode for the incremental sugar, it will actually let it > slide. If you specify the wrong one, it will error out. > > However, I think this is perfectly correct advice from the API: Please > use this mode with sync=bitmap and do not use it otherwise. > > Would you like me to change it to be more technically correct and > document the little affordance I made? It’s probably better not to. Better forbid as much as we can so that we can break compatibility to users that happened to use it still “because it works”. Max
20.06.2019 4:03, John Snow wrote: > We don't need or want a new sync mode for simple differences in > semantics. Create a new mode simply named "BITMAP" that is designed to > make use of the new Bitmap Sync Mode field. > > Because the only bitmap mode is 'conditional', this adds no new > functionality to the backup job (yet). The old incremental backup mode > is maintained as a syntactic sugar for sync=bitmap, mode=conditional. > > Add all of the plumbing necessary to support this new instruction. I don't follow, why you don't want to just add bitmap-mode optional parameter for incremental mode? For this all looks similar to just two separate things: 1. add bitmap-mode parameter 2. rename incremental to bitmap Why do we need [2.] ? If we do only [1.], we'll avoid creating two similar modes, syntax sugar, a bit of mess as it seems to me.. Hmm, about differential backups, as I understood, we call 'differential' an incremental backup, but which considers difference not from latest incremental backup but from some in the past.. Is it incorrect?
On 6/21/19 7:29 AM, Vladimir Sementsov-Ogievskiy wrote: > 20.06.2019 4:03, John Snow wrote: >> We don't need or want a new sync mode for simple differences in >> semantics. Create a new mode simply named "BITMAP" that is designed to >> make use of the new Bitmap Sync Mode field. >> >> Because the only bitmap mode is 'conditional', this adds no new >> functionality to the backup job (yet). The old incremental backup mode >> is maintained as a syntactic sugar for sync=bitmap, mode=conditional. >> >> Add all of the plumbing necessary to support this new instruction. > > I don't follow, why you don't want to just add bitmap-mode optional parameter > for incremental mode? > Vocabulary reasons, see below. > For this all looks similar to just two separate things: > 1. add bitmap-mode parameter > 2. rename incremental to bitmap This is exactly correct! > > Why do we need [2.] ? > If we do only [1.], we'll avoid creating two similar modes, syntax sugar, a bit > of mess as it seems to me.. > > Hmm, about differential backups, as I understood, we call 'differential' an incremental > backup, but which considers difference not from latest incremental backup but from some > in the past.. Is it incorrect? > The reason is because I have been treating "INCREMENTAL" as meaning something very specific -- I gather from you and Max that you don't consider this term to mean something specific. So, by other prominent backup vendors, they use these terms in this way: INCREMENTAL: This backup contains a delta from the last INCREMENTAL backup made. In effect, this creates a chain of backups that must be squashed together to recover data, but uses less info on copy. DIFFERENTIAL: This backup contains a delta from the last FULL backup made. In effect, each differential backup only requires a base image and a single differential. This usually wastes more data during the backup process, but makes restoration processes easier. I *always* use these terms in these *exact* ways; you can see that the bitmap behavior we use is exactly what MIRROR_SYNC_MODE_INCREMENTAL does. Even when we are using bitmap manipulation techniques to get it to do something else, the block job itself is engineered to think that it is producing an "Incremental" backup. In the early days of this feature, Fam actually proposed something like what I am proposing here: a BITMAP sync mode with an on_complete parameter for the backup job that would either roll the bitmap forward or not (like my "conditional", "never") based on the success of the job. We removed that because at the time we wanted to target a simpler feature. As part of that removal, I renamed the mode "INCREMENTAL" under the premise that if we ever wanted to add a "DIFFERENTIAL" mode like what Fam's original design allowed for, we could add MIRROR_SYNC_MODE_DIFFERENTIAL and that would differentiate the two modes. This rename was done with the specific knowledge and intent that the mode was named after the exact specific backup paradigm it was enabling. Otherwise, I would have left it "BITMAP" back then. I've had patches in my branch to add a DIFFERENTIAL mode ever since then! However, since we added bitmap merging, you'll notice that we actually CAN do "Differential" backups by playing around with the bitmaps ourselves, which has largely stopped me from wanting to introduce the new mode. You'll recall that recently Xie Changlong sent patches to add "incremental" support to mirror, but what they ACTUALLY implemented was "Differential" mode -- they didn't clear the bitmap afterwards. I actually responded as such on-list -- that if we implement a "Differential" mode that their patches would have been appropriate for that mode. As a result of that discussion, I went to add a "Differential" mode to mirror, but in the process realized that it's much easier to make the bitmap sync behavior its own parameter. However, because the new parameters no longer mean the backup is "Incremental" by that definition, I decided to rename the mode "BITMAP" again to be *less specific* and, perhaps now ironically, avoid confusion. Even given this confusion ... I actually still think that we should NOT use "Incremental" to mean something generic, and I will continue to enforce the idea that "Incremental" should mean a series of non-overlapping time-sliced deltas. --js
diff --git a/qapi/block-core.json b/qapi/block-core.json index caf28a71a0..6d05ad8f47 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1127,12 +1127,15 @@ # # @none: only copy data written from now on # -# @incremental: only copy data described by the dirty bitmap. Since: 2.4 +# @incremental: only copy data described by the dirty bitmap. (since: 2.4) +# +# @bitmap: only copy data described by the dirty bitmap. (since: 4.1) +# Behavior on completion is determined by the BitmapSyncMode. # # Since: 1.3 ## { 'enum': 'MirrorSyncMode', - 'data': ['top', 'full', 'none', 'incremental'] } + 'data': ['top', 'full', 'none', 'incremental', 'bitmap'] } ## # @BitmapSyncMode: @@ -1352,10 +1355,14 @@ # # @speed: the maximum speed, in bytes per second # -# @bitmap: the name of dirty bitmap if sync is "incremental". -# Must be present if sync is "incremental", must NOT be present +# @bitmap: the name of dirty bitmap if sync is "bitmap". +# Must be present if sync is "bitmap", must NOT be present # otherwise. (Since 2.4) # +# @bitmap-mode: Specifies the type of data the bitmap should contain after +# the operation concludes. Must be present if sync is "bitmap". +# Must NOT be present otherwise. (Since 4.1) +# # @compress: true to compress data, if the target format supports it. # (default: false) (since 2.8) # @@ -1390,7 +1397,8 @@ 'data': { '*job-id': 'str', 'device': 'str', 'target': 'str', '*format': 'str', 'sync': 'MirrorSyncMode', '*mode': 'NewImageMode', '*speed': 'int', - '*bitmap': 'str', '*compress': 'bool', + '*bitmap': 'str', '*bitmap-mode': 'BitmapSyncMode', + '*compress': 'bool', '*on-source-error': 'BlockdevOnError', '*on-target-error': 'BlockdevOnError', '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } } @@ -1412,10 +1420,14 @@ # @speed: the maximum speed, in bytes per second. The default is 0, # for unlimited. # -# @bitmap: the name of dirty bitmap if sync is "incremental". -# Must be present if sync is "incremental", must NOT be present +# @bitmap: the name of dirty bitmap if sync is "bitmap". +# Must be present if sync is "bitmap", must NOT be present # otherwise. (Since 3.1) # +# @bitmap-mode: Specifies the type of data the bitmap should contain after +# the operation concludes. Must be present if sync is "bitmap". +# Must NOT be present otherwise. (Since 4.1) +# # @compress: true to compress data, if the target format supports it. # (default: false) (since 2.8) # @@ -1449,7 +1461,9 @@ { 'struct': 'BlockdevBackup', 'data': { '*job-id': 'str', 'device': 'str', 'target': 'str', 'sync': 'MirrorSyncMode', '*speed': 'int', - '*bitmap': 'str', '*compress': 'bool', + '*bitmap': 'str', + '*bitmap-mode': 'BitmapSyncMode', + '*compress': 'bool', '*on-source-error': 'BlockdevOnError', '*on-target-error': 'BlockdevOnError', '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } } diff --git a/include/block/block_int.h b/include/block/block_int.h index d6415b53c1..89370c1b9b 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -1132,7 +1132,9 @@ void mirror_start(const char *job_id, BlockDriverState *bs, * @target: Block device to write to. * @speed: The maximum speed, in bytes per second, or 0 for unlimited. * @sync_mode: What parts of the disk image should be copied to the destination. - * @sync_bitmap: The dirty bitmap if sync_mode is MIRROR_SYNC_MODE_INCREMENTAL. + * @sync_bitmap: The dirty bitmap if sync_mode is 'bitmap' or 'incremental' + * @has_bitmap_mode: true if @bitmap_sync carries a meaningful value. + * @bitmap_mode: The bitmap synchronization policy to use. * @on_source_error: The action to take upon error reading from the source. * @on_target_error: The action to take upon error writing to the target. * @creation_flags: Flags that control the behavior of the Job lifetime. @@ -1148,6 +1150,8 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, BlockDriverState *target, int64_t speed, MirrorSyncMode sync_mode, BdrvDirtyBitmap *sync_bitmap, + bool has_bitmap_mode, + BitmapSyncMode bitmap_mode, bool compress, BlockdevOnError on_source_error, BlockdevOnError on_target_error, diff --git a/block/backup.c b/block/backup.c index 715e1d3be8..c4f83d4ef7 100644 --- a/block/backup.c +++ b/block/backup.c @@ -38,9 +38,9 @@ typedef struct CowRequest { typedef struct BackupBlockJob { BlockJob common; BlockBackend *target; - /* bitmap for sync=incremental */ BdrvDirtyBitmap *sync_bitmap; MirrorSyncMode sync_mode; + BitmapSyncMode bitmap_mode; BlockdevOnError on_source_error; BlockdevOnError on_target_error; CoRwlock flush_rwlock; @@ -452,7 +452,7 @@ static int coroutine_fn backup_run(Job *job, Error **errp) job_progress_set_remaining(job, s->len); - if (s->sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) { + if (s->sync_mode == MIRROR_SYNC_MODE_BITMAP) { backup_incremental_init_copy_bitmap(s); } else { hbitmap_set(s->copy_bitmap, 0, s->len); @@ -536,6 +536,7 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target, BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, BlockDriverState *target, int64_t speed, MirrorSyncMode sync_mode, BdrvDirtyBitmap *sync_bitmap, + bool has_bitmap_mode, BitmapSyncMode bitmap_mode, bool compress, BlockdevOnError on_source_error, BlockdevOnError on_target_error, @@ -548,6 +549,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, int ret; int64_t cluster_size; HBitmap *copy_bitmap = NULL; + MirrorSyncMode effective_mode = sync_mode; assert(bs); assert(target); @@ -584,9 +586,28 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, } if (sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) { + if (has_bitmap_mode && + bitmap_mode != BITMAP_SYNC_MODE_CONDITIONAL) { + error_setg(errp, "Bitmap sync mode must be 'conditional' " + "when using sync mode '%s'", + MirrorSyncMode_str(sync_mode)); + return NULL; + } + has_bitmap_mode = true; + bitmap_mode = BITMAP_SYNC_MODE_CONDITIONAL; + effective_mode = MIRROR_SYNC_MODE_BITMAP; + } + + if (effective_mode == MIRROR_SYNC_MODE_BITMAP) { if (!sync_bitmap) { error_setg(errp, "must provide a valid bitmap name for " - "\"incremental\" sync mode"); + "'%s' sync mode", MirrorSyncMode_str(sync_mode)); + return NULL; + } + + if (!has_bitmap_mode) { + error_setg(errp, "must provide a valid bitmap mode for " + "'%s' sync mode", MirrorSyncMode_str(sync_mode)); return NULL; } @@ -596,8 +617,8 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, } } else if (sync_bitmap) { error_setg(errp, - "a sync_bitmap was provided to backup_run, " - "but received an incompatible sync_mode (%s)", + "a bitmap was given to backup_job_create, " + "but it received an incompatible sync_mode (%s)", MirrorSyncMode_str(sync_mode)); return NULL; } @@ -639,8 +660,8 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs, job->on_source_error = on_source_error; job->on_target_error = on_target_error; job->sync_mode = sync_mode; - job->sync_bitmap = sync_mode == MIRROR_SYNC_MODE_INCREMENTAL ? - sync_bitmap : NULL; + job->sync_bitmap = sync_bitmap; + job->bitmap_mode = bitmap_mode; job->compress = compress; /* Detect image-fleecing (and similar) schemes */ diff --git a/block/mirror.c b/block/mirror.c index d17be4cdbc..42b3d9acd0 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -1717,8 +1717,10 @@ void mirror_start(const char *job_id, BlockDriverState *bs, bool is_none_mode; BlockDriverState *base; - if (mode == MIRROR_SYNC_MODE_INCREMENTAL) { - error_setg(errp, "Sync mode 'incremental' not supported"); + if ((mode == MIRROR_SYNC_MODE_INCREMENTAL) || + (mode == MIRROR_SYNC_MODE_BITMAP)) { + error_setg(errp, "Sync mode '%s' not supported", + MirrorSyncMode_str(mode)); return; } is_none_mode = mode == MIRROR_SYNC_MODE_NONE; diff --git a/block/replication.c b/block/replication.c index b41bc507c0..a62aaeb879 100644 --- a/block/replication.c +++ b/block/replication.c @@ -543,7 +543,7 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode, s->backup_job = backup_job_create( NULL, s->secondary_disk->bs, s->hidden_disk->bs, - 0, MIRROR_SYNC_MODE_NONE, NULL, false, + 0, MIRROR_SYNC_MODE_NONE, NULL, false, 0, false, BLOCKDEV_ON_ERROR_REPORT, BLOCKDEV_ON_ERROR_REPORT, JOB_INTERNAL, backup_job_completed, bs, NULL, &local_err); diff --git a/blockdev.c b/blockdev.c index 5d6a13dea9..7abbd6bbf2 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3572,7 +3572,9 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn, } job = backup_job_create(backup->job_id, bs, target_bs, backup->speed, - backup->sync, bmap, backup->compress, + backup->sync, bmap, + backup->has_bitmap_mode, backup->bitmap_mode, + backup->compress, backup->on_source_error, backup->on_target_error, job_flags, NULL, NULL, txn, &local_err); if (local_err != NULL) { @@ -3677,7 +3679,9 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, JobTxn *txn, job_flags |= JOB_MANUAL_DISMISS; } job = backup_job_create(backup->job_id, bs, target_bs, backup->speed, - backup->sync, bmap, backup->compress, + backup->sync, bmap, + backup->has_bitmap_mode, backup->bitmap_mode, + backup->compress, backup->on_source_error, backup->on_target_error, job_flags, NULL, NULL, txn, &local_err); if (local_err != NULL) {
We don't need or want a new sync mode for simple differences in semantics. Create a new mode simply named "BITMAP" that is designed to make use of the new Bitmap Sync Mode field. Because the only bitmap mode is 'conditional', this adds no new functionality to the backup job (yet). The old incremental backup mode is maintained as a syntactic sugar for sync=bitmap, mode=conditional. Add all of the plumbing necessary to support this new instruction. Signed-off-by: John Snow <jsnow@redhat.com> --- qapi/block-core.json | 30 ++++++++++++++++++++++-------- include/block/block_int.h | 6 +++++- block/backup.c | 35 ++++++++++++++++++++++++++++------- block/mirror.c | 6 ++++-- block/replication.c | 2 +- blockdev.c | 8 ++++++-- 6 files changed, 66 insertions(+), 21 deletions(-)