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 <vsement...@virtuozzo.com> > --- > 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