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 <[email protected]> > --- > 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
