qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 07/12] block/backup: add 'always' bitmap sync po


From: Max Reitz
Subject: Re: [Qemu-devel] [PATCH 07/12] block/backup: add 'always' bitmap sync policy
Date: Thu, 20 Jun 2019 19:00:24 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0

On 20.06.19 03:03, 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>
> ---
>  qapi/block-core.json |  5 ++++-
>  block/backup.c       | 10 ++++++----
>  2 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 0332dcaabc..58d267f1f5 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1143,6 +1143,9 @@
>  # An enumeration of possible behaviors for the synchronization of a bitmap
>  # when used for data copy operations.
>  #
> +# @always: The bitmap is always synchronized with remaining blocks to copy,
> +#          whether or not the operation has completed successfully or not.
> +#
>  # @conditional: The bitmap is only synchronized when the operation is 
> successul.
>  #               This is useful for Incremental semantics.
>  #
> @@ -1153,7 +1156,7 @@
>  # Since: 4.1
>  ##
>  { 'enum': 'BitmapSyncMode',
> -  'data': ['conditional', 'never'] }
> +  'data': ['always', 'conditional', 'never'] }
>  
>  ##
>  # @MirrorCopyMode:
> diff --git a/block/backup.c b/block/backup.c
> index 627f724b68..beb2078696 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -266,15 +266,17 @@ static void backup_cleanup_sync_bitmap(BackupBlockJob 
> *job, int ret)
>      BlockDriverState *bs = blk_bs(job->common.blk);
>  
>      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. */
> +        /* Failure, or we don't want to synchronize the bitmap. */
> +        if (job->bitmap_mode == BITMAP_SYNC_MODE_ALWAYS) {
> +            bdrv_dirty_bitmap_claim(job->sync_bitmap, &job->copy_bitmap);

Hmm...  OK, bitmaps in backup always confuse me, so bear with me, please.

(Hi, I’m a time traveler from the end of this section and I can tell you
that everything is fine.  I was just confused.  I’ll still keep this
here, because it was so much work.)

The copy_bitmap is copied from the sync_bitmap at the beginning, so the
sync_bitmap can continue to be dirtied, but that won’t affect the job.
In normal incremental mode, this means that the sync point is always at
the beginning of the job.  (Well, naturally, because that’s how backup
is supposed to go.)

But then replacing the sync_bitmap with the copy_bitmap here means that
all of these dirtyings that happened during the job are lost.  Hmm, but
that doesn’t matter, does it?  Because whenever something was dirtied in
sync_bitmap, the corresponding area must have been copied to the backup
due to the job.

Ah, yes, it would actually be wrong to keep the new dirty bits, because
in this mode, sync_bitmap should (on failure) reflect what is left to
copy to make the backup complete.  Copying these newly dirtied sectors
would be wrong.  (Yes, I know you wrote that in the documentation of
@always.  I just tried to get a different perspective.)

Yes, yes, and copy_bitmap is always set whenever a CBW to the target
fails before the source can be updated.  Good, good.


Hi, I’m the time traveler from above.  I also left the section here so I
can give one of my trademark “Ramble, ramble,

Reviewed-by: Max Reitz <address@hidden>

”

> +        }
> +        /* Merge the successor back into the parent. */
>          bm = bdrv_reclaim_dirty_bitmap(bs, job->sync_bitmap, NULL);
> -        assert(bm);
>      } else {
>          /* Everything is fine, delete this bitmap and install the backup. */
>          bm = bdrv_dirty_bitmap_abdicate(bs, job->sync_bitmap, NULL);
> -        assert(bm);
>      }
> +    assert(bm);
>  }
>  
>  static void backup_commit(Job *job)
> 


Attachment: signature.asc
Description: OpenPGP digital signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]