Re: [Qemu-devel] [PATCH v10 04/14] block/backup: introduce BlockCopyState

[Max Reitz]

On 30.08.19 18:12, Vladimir Sementsov-Ogievskiy wrote:
> Split copying code part from backup to "block-copy", including separate
> state structure and function renaming. This is needed to share it with
> backup-top filter driver in further commits.
> 
> Notes:
> 
> 1. As BlockCopyState keeps own BlockBackend objects, remaining
> job->common.blk users only use it to get bs by blk_bs() call, so clear
> job->commen.blk permissions set in block_job_create and add
> job->source_bs to be used instead of blk_bs(job->common.blk), to keep
> it more clear which bs we use when introduce backup-top filter in
> further commit.
> 
> 2. Rename s/initializing_bitmap/skip_unallocated/ to sound a bit better
> as interface to BlockCopyState
> 
> 3. Split is not very clean: there left some duplicated fields, backup
> code uses some BlockCopyState fields directly, let's postpone it for
> further improvements and keep this comment simpler for review.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  block/backup.c     | 357 ++++++++++++++++++++++++++++-----------------
>  block/trace-events |  12 +-
>  2 files changed, 231 insertions(+), 138 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index abb5099fa3..002dee4d7f 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -35,12 +35,43 @@ typedef struct CowRequest {
>      CoQueue wait_queue; /* coroutines blocked on this request */
>  } CowRequest;
>  
> +typedef void (*ProgressBytesCallbackFunc)(int64_t bytes, void *opaque);
> +typedef void (*ProgressResetCallbackFunc)(void *opaque);
> +typedef struct BlockCopyState {
> +    BlockBackend *source;
> +    BlockBackend *target;
> +    BdrvDirtyBitmap *copy_bitmap;
> +    int64_t cluster_size;
> +    bool use_copy_range;
> +    int64_t copy_range_size;
> +    uint64_t len;
> +
> +    BdrvRequestFlags write_flags;
> +
> +    /*
> +     * skip_unallocated: if true, on copy operation firstly reset areas
> +     * unallocated in top layer of source (and then of course don't copy
> +     * corresponding clusters). If some bytes reset, call
> +     * progress_reset_callback.
> +     */

It isn’t quite clear that this refers to the copy_bitmap.  Maybe
something like

“If true, the copy operation prepares a sync=top job: It scans the
source's top layer to find all unallocated areas and resets them in the
copy_bitmap (so they will not be copied).  Whenever any such area is
cleared, progress_reset_callback will be invoked.
Once the whole top layer has been scanned, skip_unallocated is cleared
and the actual copying begins.”

instead?

> +    bool skip_unallocated;
> +
> +    /* progress_bytes_callback called when some copying progress is done. */

Maybe add a colon after the attribute name?  (Or drop the name altogether)

> +    ProgressBytesCallbackFunc progress_bytes_callback;
> +
> +    /*
> +     * progress_reset_callback called when some bytes reset from copy_bitmap
> +     * (see @skip_unallocated above)

Maybe you should keep the note you before on what to do then, i.e. that
the callee should probably recalculate how many bytes remain based on
the copy_bitmap’s dirty bit count.

> +     */
> +    ProgressResetCallbackFunc progress_reset_callback;
> +    void *progress_opaque;
> +} BlockCopyState;

[...]

> @@ -415,16 +535,16 @@ static void backup_abort(Job *job)
>  static void backup_clean(Job *job)
>  {
>      BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
> -    BlockDriverState *bs = blk_bs(s->common.blk);
> +    BlockCopyState *bcs = s->bcs;
>  
> -    if (s->copy_bitmap) {
> -        bdrv_release_dirty_bitmap(bs, s->copy_bitmap);
> -        s->copy_bitmap = NULL;
> -    }
> +    /*
> +     * Zero pointer first, to not interleave with backup_drain during some
> +     * yield. TODO: just block_copy_state_free(s->bcs) after backup_drain
> +     * dropped.
> +     */

I suppose that‘s now. :-)

> +    s->bcs = NULL;
>  
> -    assert(s->target);
> -    blk_unref(s->target);
> -    s->target = NULL;
> +    block_copy_state_free(bcs);
>  }

[...]

> @@ -449,8 +569,8 @@ static void backup_drain(BlockJob *job)
>      /* Need to keep a reference in case blk_drain triggers execution
>       * of backup_complete...
>       */
> -    if (s->target) {
> -        BlockBackend *target = s->target;
> +    if (s->bcs) {
> +        BlockBackend *target = s->bcs->target;
>          blk_ref(target);
>          blk_drain(target);
>          blk_unref(target);

(And this hunk can go away now.)

[...]

> diff --git a/block/trace-events b/block/trace-events
> index 04209f058d..453792ed87 100644
> --- a/block/trace-events
> +++ b/block/trace-events
> @@ -40,12 +40,12 @@ mirror_yield_in_flight(void *s, int64_t offset, int 
> in_flight) "s %p offset %" P
>  # backup.c
>  backup_do_cow_enter(void *job, int64_t start, int64_t offset, uint64_t 
> bytes) "job %p start %" PRId64 " offset %" PRId64 " bytes %" PRIu64
>  backup_do_cow_return(void *job, int64_t offset, uint64_t bytes, int ret) 
> "job %p offset %" PRId64 " bytes %" PRIu64 " ret %d"
> -backup_do_cow_skip(void *job, int64_t start) "job %p start %"PRId64
> -backup_do_cow_skip_range(void *job, int64_t start, uint64_t bytes) "job %p 
> start %"PRId64" bytes %"PRId64
> -backup_do_cow_process(void *job, int64_t start) "job %p start %"PRId64
> -backup_do_cow_read_fail(void *job, int64_t start, int ret) "job %p start 
> %"PRId64" ret %d"
> -backup_do_cow_write_fail(void *job, int64_t start, int ret) "job %p start 
> %"PRId64" ret %d"
> -backup_do_cow_copy_range_fail(void *job, int64_t start, int ret) "job %p 
> start %"PRId64" ret %d"
> +block_copy_skip(void *bcs, int64_t start) "job %p start %"PRId64
> +block_copy_skip_range(void *bcs, int64_t start, uint64_t bytes) "job %p 
> start %"PRId64" bytes %"PRId64
> +block_copy_process(void *bcs, int64_t start) "job %p start %"PRId64
> +block_copy_with_bounce_buffer_read_fail(void *bcs, int64_t start, int ret) 
> "job %p start %"PRId64" ret %d"
> +block_copy_with_bounce_buffer_write_fail(void *bcs, int64_t start, int ret) 
> "job %p start %"PRId64" ret %d"
> +block_copy_with_offload_fail(void *bcs, int64_t start, int ret) "job %p 
> start %"PRId64" ret %d"

The messages probably should stop calling it a “job”, too.

Max

signature.asc
Description: OpenPGP digital signature