| Message ID | 20190703215542.16123-13-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | bitmaps: introduce 'bitmap' sync mode | expand |
On 03.07.19 23:55, John Snow wrote: > This adds an "always" policy for bitmap synchronization. Regardless of if > the job succeeds or fails, the bitmap is *always* synchronized. This means > that for backups that fail part-way through, the bitmap retains a record of > which sectors need to be copied out to accomplish a new backup using the > old, partial result. > > In effect, this allows us to "resume" a failed backup; however the new backup > will be from the new point in time, so it isn't a "resume" as much as it is > an "incremental retry." This can be useful in the case of extremely large > backups that fail considerably through the operation and we'd like to not waste > the work that was already performed. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > block/backup.c | 25 +++++++++++++++++-------- > qapi/block-core.json | 5 ++++- > 2 files changed, 21 insertions(+), 9 deletions(-) > > diff --git a/block/backup.c b/block/backup.c > index 9cc5a7f6ca..495d8f71aa 100644 > --- a/block/backup.c > +++ b/block/backup.c > @@ -266,16 +266,25 @@ static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret) > { > BdrvDirtyBitmap *bm; > BlockDriverState *bs = blk_bs(job->common.blk); > + bool sync = (((ret == 0) || (job->bitmap_mode == BITMAP_SYNC_MODE_ALWAYS)) \ > + && (job->bitmap_mode != BITMAP_SYNC_MODE_NEVER)); > > - 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 { > - /* Everything is fine, delete this bitmap and install the backup. */ > + if (sync) { > + /* We succeeded, or we always intended to sync the bitmap. > + * Delete this bitmap and install the child. */ > bm = bdrv_dirty_bitmap_abdicate(bs, job->sync_bitmap, NULL); > - assert(bm); > + } else { > + /* We failed, or we never intended to sync the bitmap anyway. > + * Merge the successor back into the parent, keeping all data. */ > + bm = bdrv_reclaim_dirty_bitmap(bs, job->sync_bitmap, NULL); > + } > + > + assert(bm); > + > + if (ret < 0 && job->bitmap_mode == BITMAP_SYNC_MODE_ALWAYS) { “ret < 0 && sync” would be simpler – your choice. > + /* If we failed and synced, merge in the bits we didn't copy: */ > + bdrv_dirty_bitmap_merge_internal(bm, job->copy_bitmap, > + NULL, true); I presume this is for sync=full? If so: Reviewed-by: Max Reitz <mreitz@redhat.com> > } > } >
On 04.07.19 19:43, Max Reitz wrote: > On 03.07.19 23:55, John Snow wrote: >> This adds an "always" policy for bitmap synchronization. Regardless of if >> the job succeeds or fails, the bitmap is *always* synchronized. This means >> that for backups that fail part-way through, the bitmap retains a record of >> which sectors need to be copied out to accomplish a new backup using the >> old, partial result. >> >> In effect, this allows us to "resume" a failed backup; however the new backup >> will be from the new point in time, so it isn't a "resume" as much as it is >> an "incremental retry." This can be useful in the case of extremely large >> backups that fail considerably through the operation and we'd like to not waste >> the work that was already performed. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> block/backup.c | 25 +++++++++++++++++-------- >> qapi/block-core.json | 5 ++++- >> 2 files changed, 21 insertions(+), 9 deletions(-) >> >> diff --git a/block/backup.c b/block/backup.c >> index 9cc5a7f6ca..495d8f71aa 100644 >> --- a/block/backup.c >> +++ b/block/backup.c >> @@ -266,16 +266,25 @@ static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret) [...] >> + /* If we failed and synced, merge in the bits we didn't copy: */ >> + bdrv_dirty_bitmap_merge_internal(bm, job->copy_bitmap, >> + NULL, true); > > I presume this is for sync=full? Ah, no. This is necessary because the original sync bitmap was discarded by bdrv_dirty_bitmap_abdicate(). So yep, these bits need to go back into the sync bitmap then. Max
On 7/4/19 2:05 PM, Max Reitz wrote: > On 04.07.19 19:43, Max Reitz wrote: >> On 03.07.19 23:55, John Snow wrote: >>> This adds an "always" policy for bitmap synchronization. Regardless of if >>> the job succeeds or fails, the bitmap is *always* synchronized. This means >>> that for backups that fail part-way through, the bitmap retains a record of >>> which sectors need to be copied out to accomplish a new backup using the >>> old, partial result. >>> >>> In effect, this allows us to "resume" a failed backup; however the new backup >>> will be from the new point in time, so it isn't a "resume" as much as it is >>> an "incremental retry." This can be useful in the case of extremely large >>> backups that fail considerably through the operation and we'd like to not waste >>> the work that was already performed. >>> >>> Signed-off-by: John Snow <jsnow@redhat.com> >>> --- >>> block/backup.c | 25 +++++++++++++++++-------- >>> qapi/block-core.json | 5 ++++- >>> 2 files changed, 21 insertions(+), 9 deletions(-) >>> >>> diff --git a/block/backup.c b/block/backup.c >>> index 9cc5a7f6ca..495d8f71aa 100644 >>> --- a/block/backup.c >>> +++ b/block/backup.c >>> @@ -266,16 +266,25 @@ static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret) > > [...] > >>> + /* If we failed and synced, merge in the bits we didn't copy: */ >>> + bdrv_dirty_bitmap_merge_internal(bm, job->copy_bitmap, >>> + NULL, true); >> >> I presume this is for sync=full? > Well, we don't allow bitmaps for sync=full at this point in the series. > Ah, no. This is necessary because the original sync bitmap was > discarded by bdrv_dirty_bitmap_abdicate(). So yep, these bits need to > go back into the sync bitmap then. > > Max > Yuh -- we actually want to clear the original bitmap for the 'always' case, which the "abdicate" handles for us. This ought to trigger only for the always case, so I think the conditional here is correct and as simple as it can be. Does your R-B stand? --js
On 05.07.19 18:59, John Snow wrote: > > > On 7/4/19 2:05 PM, Max Reitz wrote: >> On 04.07.19 19:43, Max Reitz wrote: >>> On 03.07.19 23:55, John Snow wrote: >>>> This adds an "always" policy for bitmap synchronization. Regardless of if >>>> the job succeeds or fails, the bitmap is *always* synchronized. This means >>>> that for backups that fail part-way through, the bitmap retains a record of >>>> which sectors need to be copied out to accomplish a new backup using the >>>> old, partial result. >>>> >>>> In effect, this allows us to "resume" a failed backup; however the new backup >>>> will be from the new point in time, so it isn't a "resume" as much as it is >>>> an "incremental retry." This can be useful in the case of extremely large >>>> backups that fail considerably through the operation and we'd like to not waste >>>> the work that was already performed. >>>> >>>> Signed-off-by: John Snow <jsnow@redhat.com> >>>> --- >>>> block/backup.c | 25 +++++++++++++++++-------- >>>> qapi/block-core.json | 5 ++++- >>>> 2 files changed, 21 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/block/backup.c b/block/backup.c >>>> index 9cc5a7f6ca..495d8f71aa 100644 >>>> --- a/block/backup.c >>>> +++ b/block/backup.c >>>> @@ -266,16 +266,25 @@ static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret) >> >> [...] >> >>>> + /* If we failed and synced, merge in the bits we didn't copy: */ >>>> + bdrv_dirty_bitmap_merge_internal(bm, job->copy_bitmap, >>>> + NULL, true); >>> >>> I presume this is for sync=full? >> > > Well, we don't allow bitmaps for sync=full at this point in the series. > >> Ah, no. This is necessary because the original sync bitmap was >> discarded by bdrv_dirty_bitmap_abdicate(). So yep, these bits need to >> go back into the sync bitmap then. >> >> Max >> > > Yuh -- we actually want to clear the original bitmap for the 'always' > case, which the "abdicate" handles for us. This ought to trigger only > for the always case, so I think the conditional here is correct and as > simple as it can be. > > Does your R-B stand? Sure. Max
diff --git a/block/backup.c b/block/backup.c index 9cc5a7f6ca..495d8f71aa 100644 --- a/block/backup.c +++ b/block/backup.c @@ -266,16 +266,25 @@ static void backup_cleanup_sync_bitmap(BackupBlockJob *job, int ret) { BdrvDirtyBitmap *bm; BlockDriverState *bs = blk_bs(job->common.blk); + bool sync = (((ret == 0) || (job->bitmap_mode == BITMAP_SYNC_MODE_ALWAYS)) \ + && (job->bitmap_mode != BITMAP_SYNC_MODE_NEVER)); - 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 { - /* Everything is fine, delete this bitmap and install the backup. */ + if (sync) { + /* We succeeded, or we always intended to sync the bitmap. + * Delete this bitmap and install the child. */ bm = bdrv_dirty_bitmap_abdicate(bs, job->sync_bitmap, NULL); - assert(bm); + } else { + /* We failed, or we never intended to sync the bitmap anyway. + * Merge the successor back into the parent, keeping all data. */ + bm = bdrv_reclaim_dirty_bitmap(bs, job->sync_bitmap, NULL); + } + + assert(bm); + + if (ret < 0 && job->bitmap_mode == BITMAP_SYNC_MODE_ALWAYS) { + /* If we failed and synced, merge in the bits we didn't copy: */ + bdrv_dirty_bitmap_merge_internal(bm, job->copy_bitmap, + NULL, true); } } diff --git a/qapi/block-core.json b/qapi/block-core.json index 1007192655..b527306d19 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1149,10 +1149,13 @@ # @never: The bitmap is never synchronized with the operation, and is # treated solely as a read-only manifest of blocks to copy. # +# @always: The bitmap is always synchronized with the operation, +# regardless of whether or not the operation was successful. +# # Since: 4.2 ## { 'enum': 'BitmapSyncMode', - 'data': ['conditional', 'never'] } + 'data': ['conditional', 'never', 'always'] } ## # @MirrorCopyMode:
This adds an "always" policy for bitmap synchronization. Regardless of if the job succeeds or fails, the bitmap is *always* synchronized. This means that for backups that fail part-way through, the bitmap retains a record of which sectors need to be copied out to accomplish a new backup using the old, partial result. In effect, this allows us to "resume" a failed backup; however the new backup will be from the new point in time, so it isn't a "resume" as much as it is an "incremental retry." This can be useful in the case of extremely large backups that fail considerably through the operation and we'd like to not waste the work that was already performed. Signed-off-by: John Snow <jsnow@redhat.com> --- block/backup.c | 25 +++++++++++++++++-------- qapi/block-core.json | 5 ++++- 2 files changed, 21 insertions(+), 9 deletions(-)