Re: [Qemu-block] [PATCH v2 12/18] block/backup: add 'always' bitmap sync policy

[Max Reitz]

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 <address@hidden>
> ---
>  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 <address@hidden>

>      }
>  }
>

signature.asc
Description: OpenPGP digital signature