| Message ID | 20190620010356.19164-4-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | bitmaps: introduce 'bitmap' sync mode | expand |
On 20.06.19 03:03, John Snow wrote: > This adds a "never" policy for bitmap synchronization. Regardless of if > the job succeeds or fails, we never update the bitmap. This can be used > to perform differential backups, or simply to avoid the job modifying a > bitmap. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > qapi/block-core.json | 6 +++++- > block/backup.c | 5 +++-- > 2 files changed, 8 insertions(+), 3 deletions(-) > > diff --git a/qapi/block-core.json b/qapi/block-core.json > index 6d05ad8f47..0332dcaabc 100644 > --- a/qapi/block-core.json > +++ b/qapi/block-core.json > @@ -1146,10 +1146,14 @@ > # @conditional: The bitmap is only synchronized when the operation is successul. > # This is useful for Incremental semantics. > # > +# @never: The bitmap is never synchronized with the operation, and is > +# treated solely as a manifest of blocks to copy. > +# This is useful for Differential semantics. > +# Again, this is too buzzword-y for my taste. I don’t find it as bad because there is not much to explain about this mode, and you do explain it above, but still. Like, I (me myself) read this and after the first sentence I think I’ve understood what this is. Then I read “for Differential semantics” and I’m confused. After a couple of seconds, I realize what you mean because I’ve described in my response to patch 1. One reason it leaves the buzzword-y taste is because “differential” is never explained anywhere. bitmaps.rst makes two mentions of it, but it too just assumes I know what you mean. Also, incremental backups are just a certain kind of differential backups. So you need to explain “differential” somewhere and how it differs from “incremental” in this regard. Why not here? “This is useful when you wish to repeatedly perform operations in reference to a constant synchronization point (when the bitmap was created).” Or something. Max > # Since: 4.1 > ## > { 'enum': 'BitmapSyncMode', > - 'data': ['conditional'] } > + 'data': ['conditional', 'never'] } > > ## > # @MirrorCopyMode:
On 6/20/19 11:25 AM, Max Reitz wrote: > On 20.06.19 03:03, John Snow wrote: >> This adds a "never" policy for bitmap synchronization. Regardless of if >> the job succeeds or fails, we never update the bitmap. This can be used >> to perform differential backups, or simply to avoid the job modifying a >> bitmap. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> qapi/block-core.json | 6 +++++- >> block/backup.c | 5 +++-- >> 2 files changed, 8 insertions(+), 3 deletions(-) >> >> diff --git a/qapi/block-core.json b/qapi/block-core.json >> index 6d05ad8f47..0332dcaabc 100644 >> --- a/qapi/block-core.json >> +++ b/qapi/block-core.json >> @@ -1146,10 +1146,14 @@ >> # @conditional: The bitmap is only synchronized when the operation is successul. >> # This is useful for Incremental semantics. >> # >> +# @never: The bitmap is never synchronized with the operation, and is >> +# treated solely as a manifest of blocks to copy. >> +# This is useful for Differential semantics. >> +# > > Again, this is too buzzword-y for my taste. I don’t find it as bad > because there is not much to explain about this mode, and you do explain > it above, but still. > Explained in my response to patch 2, I disagree. > Like, I (me myself) read this and after the first sentence I think I’ve > understood what this is. Then I read “for Differential semantics” and > I’m confused. After a couple of seconds, I realize what you mean > because I’ve described in my response to patch 1. > > One reason it leaves the buzzword-y taste is because “differential” is > never explained anywhere. bitmaps.rst makes two mentions of it, but it > too just assumes I know what you mean. Also, incremental backups are > just a certain kind of differential backups. > This, however, is a real shortcoming of the doc. You'll notice I didn't propose a doc update in this patchset, because secretly it's an RFC and I did expect a v2+. > So you need to explain “differential” somewhere and how it differs from > “incremental” in this regard. Why not here? > Too broad of a concept to explain down in qapi comment strings, or I'd have to explain it everywhere. bitmaps.rst is the correct place. > “This is useful when you wish to repeatedly perform operations in > reference to a constant synchronization point (when the bitmap was > created).” > > Or something. > > Max > >> # Since: 4.1 >> ## >> { 'enum': 'BitmapSyncMode', >> - 'data': ['conditional'] } >> + 'data': ['conditional', 'never'] } >> >> ## >> # @MirrorCopyMode: >
diff --git a/qapi/block-core.json b/qapi/block-core.json index 6d05ad8f47..0332dcaabc 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1146,10 +1146,14 @@ # @conditional: The bitmap is only synchronized when the operation is successul. # This is useful for Incremental semantics. # +# @never: The bitmap is never synchronized with the operation, and is +# treated solely as a manifest of blocks to copy. +# This is useful for Differential semantics. +# # Since: 4.1 ## { 'enum': 'BitmapSyncMode', - 'data': ['conditional'] } + 'data': ['conditional', 'never'] } ## # @MirrorCopyMode: diff --git a/block/backup.c b/block/backup.c index c4f83d4ef7..627f724b68 100644 --- a/block/backup.c +++ b/block/backup.c @@ -265,8 +265,9 @@ static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret) BdrvDirtyBitmap *bm; BlockDriverState *bs = blk_bs(job->common.blk); - if (ret < 0) { - /* Merge the successor back into the parent, delete nothing. */ + if (ret < 0 || job->bitmap_mode == BITMAP_SYNC_MODE_NEVER) { + /* Failure, or we don't want to synchronize the bitmap. + * Merge the successor back into the parent, delete nothing. */ bm = bdrv_reclaim_dirty_bitmap(bs, job->sync_bitmap, NULL); assert(bm); } else {
This adds a "never" policy for bitmap synchronization. Regardless of if the job succeeds or fails, we never update the bitmap. This can be used to perform differential backups, or simply to avoid the job modifying a bitmap. Signed-off-by: John Snow <jsnow@redhat.com> --- qapi/block-core.json | 6 +++++- block/backup.c | 5 +++-- 2 files changed, 8 insertions(+), 3 deletions(-)